build: warn on potentially uninitialized reads

vasild commented at 4:10 pm on May 1, 2020: member

Enable conditional-uninitialized warning class to show potentially uninitialized reads.
Fix the sole such warning in Bitcoin Core in GetRdRand(): r1 would be set to 0 on rdrand failure, so initializing it to 0 is a non-functional change.

brakmic commented at 4:44 pm on May 1, 2020: contributor

Code review ACK

DrahtBot added the label Build system on May 1, 2020

practicalswift commented at 6:10 pm on May 1, 2020: contributor

Concept ACK

DrahtBot commented at 6:46 pm on May 1, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
#18216 (test, build: Enable -Werror=sign-compare by Empact)
#16710 (build: Enable -Wsuggest-override if available by hebasto)

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.

fanquake commented at 1:01 am on May 2, 2020: member

Please don’t bundle unrelated changes like https://github.com/bitcoin/bitcoin/pull/18843/commits/fe466a4fdc705d6a368a4babc1c3b44537edf1c9 into a PR like this.

r1 would be set to 0 on rdrand failure, so initializing it to 0 is a non-functional change.

Can you at least link to documentation that says this is the case.

vasild force-pushed on May 3, 2020

vasild commented at 2:50 pm on May 3, 2020: member

Please don’t bundle unrelated changes like fe466a4 into a PR like this.

Sorry for that. I considered it would be ok as long as it is a separate commit and the main change in this PR steps on top of it. Now I submitted that dedup as a separate PR at #18857, removed it from this PR and made this PR to use the old AX_CHECK_COMPILE_FLAG() so that there is no dependency between the two PRs and either one can be merged first. As a result they conflict but it is trivial to resolve - once one of them gets merged (if that happens!) I will adjust the other one.

r1 would be set to 0 on rdrand failure, so initializing it to 0 is a non-functional change.

Can you at least link to documentation that says this is the case.

Done in the commit message:

From “Intel 64 and IA-32 ArchitecturesSoftware Developer’s Manual” [1], page 1711: “CF=1 indicates that the data in the destination is valid. Otherwise CF=0 and the data in the destination operand will be returned as zeros for the specified width.”

[1] https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf

Thanks for looking into this!

build: warn on potentially uninitialized reads

Enable -Wconditional-uninitialized to warn on potentially uninitialized
reads.

Fix the sole such warning in Bitcoin Core in GetRdRand(): r1 would be
set to 0 on rdrand failure, so initializing it to 0 is a non-functional
change.

From "Intel 64 and IA-32 ArchitecturesSoftware Developer's Manual" [1],
page 1711: "CF=1 indicates that the data in the destination is valid.
Otherwise CF=0 and the data in the destination operand will be returned
as zeros for the specified width."

[1] https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf

71f183a49b

in src/random.cpp:122 in 23faed094e outdated

115@@ -116,7 +116,7 @@ static uint64_t GetRdRand() noexcept
116     // RdRand may very rarely fail. Invoke it up to 10 times in a loop to reduce this risk.
117 #ifdef __i386__
118     uint8_t ok;
119-    uint32_t r1, r2;
120+    uint32_t r1 = 0, r2 = 0;

MarcoFalke commented at 2:56 pm on May 3, 2020:

0    uint32_t r1 = 0, r2 = 0; // ...

Mabye also add an inline comment to explain why setting to zero is not a bug?

vasild commented at 3:22 pm on May 3, 2020:

Done: https://github.com/bitcoin/bitcoin/compare/23faed094ecf5f77fa250e57464bab2fb089730c..71f183a49b714a28622277fa668d8f9f3dac0aae

vasild force-pushed on May 3, 2020

vasild commented at 3:31 pm on May 3, 2020: member

It would be more readable to use _rdrand64_step() from immintrin.h or __builtin_ia32_rdrand64_step() instead of __asm__ volatile (".byte 0x48, 0x0f, 0xc7, 0xf0; setc %1" : "=a"(r1), "=q"(ok) :: "cc"); reference.

Out of the scope of this PR.

sipa commented at 6:31 pm on May 3, 2020: member

@vasild The annoying part about using intrinsics is that they require moving the code to a separate module and compiling it separately. That’s because in order to get access to _rdrand64_step(), you need to compile with -mrdrnd, which makes the compiler assume that rdrand is available throughout the entire module (which could in theory cause the compiler to emit such instructions even in cases where it’s running on a system that doesn’t have that instruction).

practicalswift commented at 6:56 pm on May 3, 2020: contributor

ACK 71f183a49b714a28622277fa668d8f9f3dac0aae

laanwj commented at 11:49 am on May 6, 2020: member

ACK 71f183a49b714a28622277fa668d8f9f3dac0aae

laanwj merged this on May 6, 2020

laanwj closed this on May 6, 2020

vasild deleted the branch on May 6, 2020

sidhujag referenced this in commit 8d2a25c69e on May 12, 2020

ComputerCraftr referenced this in commit b27501567f on Jun 10, 2020

deadalnix referenced this in commit 9c7bed4ea4 on Apr 8, 2021

kittywhiskers referenced this in commit 2a26b115f8 on Jun 17, 2021

kittywhiskers referenced this in commit 4c492b0c4b on Jun 17, 2021

kittywhiskers referenced this in commit 638fdf25fd on Jun 17, 2021

UdjinM6 referenced this in commit 988fa6a235 on Jun 23, 2021

ogabrielides referenced this in commit a8191cb3b7 on Sep 15, 2021

DrahtBot locked this on Feb 15, 2022

build: warn on potentially uninitialized reads #18843

Conflicts