util: Fix UB in SetStdinEcho when ENOTTY by maflcko · Pull Request #34597 · bitcoin/bitcoin

maflcko · 2026-02-16T11:37:29Z

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:

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

DrahtBot · 2026-02-16T11:37:48Z

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  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.

l0rinc

concept ACK

Does this enable the removal of

bitcoin/test/sanitizer_suppressions/ubsan

Line 58 in d7c9d6c

implicit-integer-sign-change:SetStdinEcho

?

maflcko · 2026-02-16T13:08:44Z

No, that is just a harmless warning. I guess I can refactor it a bit while touching this...

maflcko · 2026-02-16T13:53:18Z

Pushed that as well. Needs compilation with the integer sanitizer (-DSANITIZERS=integer, ref: https://godbolt.org/z/YGjWa89vs) and 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

luke-jr · 2026-02-18T16:01:29Z

src/compat/stdin.cpp

 // https://stackoverflow.com/questions/1413445/reading-a-password-from-stdcin
 void SetStdinEcho(bool enable)
 {
+    if (!StdinTerminal()) {


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

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

actually, done with a simple fputs and a string literal for each case

luke-jr · 2026-02-18T16:02:27Z

src/compat/stdin.cpp

+    }
    if (!enable) {
-        tty.c_lflag &= ~ECHO;
+        tty.c_lflag &= static_cast<decltype(tty.c_lflag)>(~ECHO);


Probably should do the case before the bitwise negation?

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.

…r 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

achow101 · 2026-02-23T23:33:15Z

ACK fa6af85

sedited

ACK fa6af85

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) .

maflcko · 2026-02-26T16:58:20Z

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.

l0rinc · 2026-02-27T09:44:02Z

lightly tested code review ACK fa6af85

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

Github-Pull: bitcoin#34597 Rebased-From: fa69297

…r 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 Github-Pull: bitcoin#34597 Rebased-From: fa6af85

fanquake · 2026-02-27T12:06:51Z

Backported to 30.x in #34689.

Github-Pull: bitcoin#34597 Rebased-From: fa69297

…r 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 Github-Pull: bitcoin#34597 Rebased-From: fa6af85

49a777d doc: update release notes for v30.x (fanquake) 0f9e08f doc: update build guides pre v31 (fanquake) 597ac36 doc: Fix `fee` field in `getblock` RPC result (nervana21) 47ed306 depends: Allow building Qt packages after interruption (Hennadii Stepanov) d221d1c psbt: validate pubkeys in MuSig2 pubnonce/partial sig deserialization (tboy1337) e1210ac doc: Improve dependencies.md IPC documentation (Ryan Ofsky) c17a5cd test: Add missing timeout_factor to zmq socket (MarcoFalke) 3042509 netif: fix compilation warning in QueryDefaultGatewayImpl() (MarcoFalke) 475a5b0 refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke) 7220ee3 util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke) Pull request description: Backports: * #34093 * #34219 * #34597 * #34690 * #34702 * #34706 * #34713 * #34789 ACKs for top commit: marcofleon: ACK 49a777d Tree-SHA512: b4ce54860b7306b22de75bb093ad574110875253e4ea3ca96a736809c8291dea1144a617c8791f36618d8e367022709ba5cf84ca0e450ef6d76394ab80f22e2f

DrahtBot added the Utils/log/libs label Feb 16, 2026

l0rinc reviewed Feb 16, 2026

View reviewed changes

maflcko mentioned this pull request Feb 16, 2026

ci: Temporarily use clang in valgrind tasks #34589

Open

maflcko added this to the 31.0 milestone Feb 17, 2026

maflcko mentioned this pull request Feb 18, 2026

test: Fix broken --valgrind handling after bitcoin wrapper #34608

Merged

luke-jr reviewed Feb 18, 2026

View reviewed changes

MarcoFalke added 2 commits February 18, 2026 21:28

util: Fix UB in SetStdinEcho when ENOTTY

fa69297

maflcko force-pushed the 2602-less-ub-stdin branch from faff796 to fa6af85 Compare February 18, 2026 20:32

DrahtBot mentioned this pull request Feb 19, 2026

Implementation of SwiftSync #34004

Draft

DrahtBot requested a review from l0rinc February 23, 2026 23:33

maflcko requested a review from luke-jr February 24, 2026 10:09

sedited approved these changes Feb 26, 2026

View reviewed changes

fanquake merged commit 05cd3b0 into bitcoin:master Feb 27, 2026
26 checks passed

maflcko deleted the 2602-less-ub-stdin branch February 27, 2026 11:10

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 27, 2026

util: Fix UB in SetStdinEcho when ENOTTY

24fcbe1

Github-Pull: bitcoin#34597 Rebased-From: fa69297

fanquake mentioned this pull request Feb 27, 2026

[30.x] Backports #34689

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Mar 9, 2026

util: Fix UB in SetStdinEcho when ENOTTY

7220ee3

Github-Pull: bitcoin#34597 Rebased-From: fa69297

Conversation

maflcko commented Feb 16, 2026

Uh oh!

DrahtBot commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 16, 2026

Uh oh!

maflcko commented Feb 16, 2026

Uh oh!

luke-jr Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

luke-jr Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 23, 2026

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 26, 2026

Uh oh!

l0rinc commented Feb 27, 2026

Uh oh!

Uh oh!

fanquake commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Feb 16, 2026 •

edited

Loading