util: Fix UB in SetStdinEcho when ENOTTY #34597

1. 
   maflcko commented at 11:37 am on February 16, 2026: member

   The call to tcgetattr may fail with ENOTTY, leaving the struct possibly uninitialized (UB).

   Fix this UB by returning early when isatty fails, or when tcgetattr fails. (Same for Windows)

   This can be tested by a command that fails valgrind before the change and passes after:

   0echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime
   

2. DrahtBot added the label Utils/log/libs on Feb 16, 2026
3. 
   DrahtBot commented at 11:37 am on February 16, 2026: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, sedited, l0rinc

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #34589 (ci: Temporarily use clang in valgrind tasks by maflcko)
   • #34004 (Implementation of SwiftSync by rustaceanrob)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. 
   l0rinc commented at 12:46 pm on February 16, 2026: contributor

   concept ACK

   Does this enable the removal of https://github.com/bitcoin/bitcoin/blob/d7c9d6c2914aadd711544908d0fad8857a809c72/test/sanitizer_suppressions/ubsan#L58?

5. 
   maflcko commented at 1:08 pm on February 16, 2026: member

   No, that is just a harmless warning. I guess I can refactor it a bit while touching this…
6. 
   maflcko commented at 1:53 pm on February 16, 2026: member

   Pushed that as well. Needs compilation with the integer sanitizer (-DSANITIZERS=integer, ref: https://godbolt.org/z/YGjWa89vs) and can be tested via:

   0UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime
   

7. maflcko added this to the milestone 31.0 on Feb 17, 2026
8. in src/compat/stdin.cpp:21 in faff79629c outdated

   17@@ -18,21 +18,28 @@
   18 // https://stackoverflow.com/questions/1413445/reading-a-password-from-stdcin
   19 void SetStdinEcho(bool enable)
   20 {
   21+    if (!StdinTerminal()) {
   

   ---

   ---

   

   luke-jr commented at 4:01 pm on February 18, 2026:

   Since this is now checked here, should failure below log an error? (Maybe visibly so the user knows to expect echo?)

   ---

   

   maflcko commented at 4:16 pm on February 18, 2026:

   I don’t think an error can happen, so this seems a bit too much effort for basically dead/untested code

   ---

   

   maflcko commented at 7:49 am on February 19, 2026:

   actually, done with a simple fputs and a string literal for each case
9. util: Fix UB in SetStdinEcho when ENOTTY fa692974ac
10. refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning

    This refactor does not change any behavior, except for the integer
    sanitizer warning.
    
    Can be tested via:
    
    UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime

    fa6af85634
11. in src/compat/stdin.cpp:42 in faff79629c outdated

    41+    if (tcgetattr(STDIN_FILENO, &tty) != 0) {
    42+        return;
    43+    }
    44     if (!enable) {
    45-        tty.c_lflag &= ~ECHO;
    46+        tty.c_lflag &= static_cast<decltype(tty.c_lflag)>(~ECHO);
    

    ---

    ---

    

    luke-jr commented at 4:02 pm on February 18, 2026:

    Probably should do the case before the bitwise negation?

    ---

    

    maflcko commented at 4:19 pm on February 18, 2026:

    I don’t think it matters here, because everything will be compiled down to the same executable anyway and this is just to silence a harmless ubsan warning.
12. maflcko force-pushed on Feb 18, 2026
13. 
    achow101 commented at 11:33 pm on February 23, 2026: member

    ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c
14. DrahtBot requested review from l0rinc on Feb 23, 2026
15. maflcko requested review from luke-jr on Feb 24, 2026
16. sedited approved
17. 
    sedited commented at 4:42 pm on February 26, 2026: contributor

    ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c

    Was thinking why we don’t just throw on these errors, but I think printing something to stderr is the correct thing to do, and introducing exceptions was discussed in the original PR: #13716 (review) .

18. 
    maflcko commented at 4:58 pm on February 26, 2026: member

    Was thinking why we don’t just throw on these errors, but I think printing something to stderr is the correct thing to do, and introducing exceptions was discussed in the original PR: #13716 (comment) .

    I think that was referring to the test/lint/lint-locale-dependence.py exception. But yeah, printing to stderr seems sufficient and aborting the whole program may not be needed.

19. 
    l0rinc commented at 9:44 am on February 27, 2026: contributor

    lightly tested code review ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c

    Thanks for fixing these and removing the suppressions - they always make me nervous.

20. fanquake merged this on Feb 27, 2026
21. fanquake closed this on Feb 27, 2026

    ---

22. maflcko deleted the branch on Feb 27, 2026
23. fanquake referenced this in commit 24fcbe18e3 on Feb 27, 2026
24. fanquake referenced this in commit 88e071667b on Feb 27, 2026
25. 
    fanquake commented at 12:06 pm on February 27, 2026: member

    Backported to 30.x in #34689.

Contributors
maflcko DrahtBot l0rinc luke-jr achow101 sedited fanquake

Review Requested
luke-jr l0rinc

Labels
Utils/log/libs

Milestone
31.0