Skip to content

Commit 05cd3b0

Browse files
committed
Merge bitcoin#34597: util: Fix UB in SetStdinEcho when ENOTTY
fa6af85 refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke) fa69297 util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke) Pull request description: 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 ACKs for top commit: achow101: ACK fa6af85 l0rinc: lightly tested code review ACK fa6af85 sedited: ACK fa6af85 Tree-SHA512: 76e2fbcb6c323b17736ee057dbd5e932b2e8cbb7d9fe4488c1dc7ab6ea928a3cde7e72ca0a63f8c8c78871ccb8b669263b712c0e1b530d88f2d45ea41f071201
2 parents 3c7b0f9 + fa6af85 commit 05cd3b0

3 files changed

Lines changed: 18 additions & 12 deletions

File tree

contrib/valgrind.supp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,6 @@
1414
# Note that suppressions may depend on OS and/or library versions.
1515
# Tested on aarch64 and x86_64 with Ubuntu Noble system libs, using clang-16
1616
# and GCC, without gui.
17-
{
18-
Suppress uninitialized bytes warning in compat code
19-
Memcheck:Param
20-
ioctl(TCSET{S,SW,SF})
21-
fun:tcsetattr
22-
}
2317
{
2418
Suppress leaks on shutdown
2519
Memcheck:Leak

src/compat/stdin.cpp

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,25 +18,38 @@
1818
// https://stackoverflow.com/questions/1413445/reading-a-password-from-stdcin
1919
void SetStdinEcho(bool enable)
2020
{
21+
if (!StdinTerminal()) {
22+
return;
23+
}
2124
#ifdef WIN32
2225
HANDLE hStdin = GetStdHandle(STD_INPUT_HANDLE);
2326
DWORD mode;
24-
GetConsoleMode(hStdin, &mode);
27+
if (!GetConsoleMode(hStdin, &mode)) {
28+
fputs("GetConsoleMode failed\n", stderr);
29+
return;
30+
}
2531
if (!enable) {
2632
mode &= ~ENABLE_ECHO_INPUT;
2733
} else {
2834
mode |= ENABLE_ECHO_INPUT;
2935
}
30-
SetConsoleMode(hStdin, mode);
36+
if (!SetConsoleMode(hStdin, mode)) {
37+
fputs("SetConsoleMode failed\n", stderr);
38+
}
3139
#else
3240
struct termios tty;
33-
tcgetattr(STDIN_FILENO, &tty);
41+
if (tcgetattr(STDIN_FILENO, &tty) != 0) {
42+
fputs("tcgetattr failed\n", stderr);
43+
return;
44+
}
3445
if (!enable) {
35-
tty.c_lflag &= ~ECHO;
46+
tty.c_lflag &= static_cast<decltype(tty.c_lflag)>(~ECHO);
3647
} else {
3748
tty.c_lflag |= ECHO;
3849
}
39-
(void)tcsetattr(STDIN_FILENO, TCSANOW, &tty);
50+
if (tcsetattr(STDIN_FILENO, TCSANOW, &tty) != 0) {
51+
fputs("tcsetattr failed\n", stderr);
52+
}
4053
#endif
4154
}
4255

test/sanitizer_suppressions/ubsan

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ unsigned-integer-overflow:TxConfirmStats::EstimateMedianVal
5555
unsigned-integer-overflow:InsecureRandomContext::rand64
5656
unsigned-integer-overflow:InsecureRandomContext::SplitMix64
5757
unsigned-integer-overflow:bitset_detail::PopCount
58-
implicit-integer-sign-change:SetStdinEcho
5958
implicit-integer-sign-change:compressor.h
6059
implicit-integer-sign-change:crypto/
6160
implicit-integer-sign-change:TxConfirmStats::removeTx

0 commit comments

Comments
 (0)