tests: really test the non-var scalar inverse

niooss-ledger commented at 1:33 pm on June 28, 2021: contributor

Function test_inverse_scalar contains:

(var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse_var)(&l, x);  /* l = 1/x */

The two sides of the condition are the same function. This seems to be an error, as there also exists a non-var function, named secp256k1_scalar_inverse.

Make test_inverse_scalar use this other function when var is false.

This issue was found using clang’s static analyzer, which reported a “Logic error: Identical expressions in conditional expression” (with checker alpha.core.IdenticalExpr).

tests: really test the non-var scalar inverse

Function `test_inverse_scalar` contains:

    (var ? secp256k1_scalar_inverse_var : secp256k1_scalar_inverse_var)(&l, x);  /* l = 1/x */

The two sides of the condition are the same function. This seems to be
an error, as there also exists a non-var function, named
`secp256k1_scalar_inverse`.

Make `test_inverse_scalar` use this other function when `var` is false.

This issue was found using clang's static analyzer, which reported a
"Logic error: Identical expressions in conditional expression" (with
checker `alpha.core.IdenticalExpr`).

41ed13942b

real-or-random approved

real-or-random commented at 1:54 pm on June 28, 2021: contributor

Whoa, nice catch!

ACK 41ed13942bd722ff0ee4d2eb8bef55155549afd6

real-or-random cross-referenced this on Jun 28, 2021 from issue Improve precision of code coverage and add report to CI by real-or-random

real-or-random commented at 2:10 pm on June 28, 2021: contributor

This issue was found using clang’s static analyzer, which reported a “Logic error: Identical expressions in conditional expression” (with checker alpha.core.IdenticalExpr).

Can you elaborate on how you used/called the tool? Maybe we could add it to our arsenal in CI if it doesn’t give a gazillion false positives.

niooss-ledger commented at 2:28 pm on June 28, 2021: contributor

Can you elaborate on how you used/called the tool? Maybe we could add it to our arsenal in CI if it doesn’t give a gazillion false positives.

I used the Ubuntu package clang-tools to install this tool. Then I ran this command line (on Ubuntu):

 0scan-build \
 1    --force-analyze-debug-code \
 2    -enable-checker security \
 3    -enable-checker unix \
 4    -enable-checker valist \
 5    -enable-checker alpha.core.BoolAssignment \
 6    -enable-checker alpha.core.CastSize \
 7    -enable-checker alpha.core.CastToStruct \
 8    -enable-checker alpha.core.DynamicTypeChecker \
 9    -enable-checker alpha.core.FixedAddr \
10    -enable-checker alpha.core.IdenticalExpr \
11    -enable-checker alpha.core.PointerArithm \
12    -enable-checker alpha.core.PointerSub \
13    -enable-checker alpha.core.SizeofPtr \
14    -enable-checker alpha.core.TestAfterDivZero \
15    -enable-checker alpha.security.ArrayBoundV2 \
16    -enable-checker alpha.security.MallocOverflow \
17    -enable-checker alpha.security.ReturnPtrRange \
18    -enable-checker alpha.security.taint.TaintPropagation \
19    -enable-checker alpha.unix.PthreadLock \
20    -enable-checker alpha.unix.SimpleStream \
21    -enable-checker alpha.unix.Stream \
22    -enable-checker alpha.unix.cstring.BufferOverlap \
23    -enable-checker alpha.unix.cstring.OutOfBounds \
24    -enable-checker alpha.unix.cstring.NotNullTerminated \
25    -maxloop 257 \
26    -analyze-headers \
27    -o "$(pwd)/scan-build-output" \
28    sh -c 'export CFLAGS=-DVERIFY ; ./configure --enable-coverage=no --with-asm=no && make'

This command produces several HTML files in a new directory scan-build-output.

CFLAGS=-DVERIFY is used to greatly reduce the number of “NULL pointer dereference” reports. By the way, it would be nice if there was a configure switch for this ;)
--enable-coverage=no (which is the default value) is used to ensure that VERIFY_CHECK are compiled, in order to avoid “NULL pointer dereference” reports.
--with-asm=no is used to avoid using the ASM implementation of some functions, that the static analyzer cannot use.

At first there were ~100 warnings, but many of them were duplicates because they occur in C header files.

real-or-random commented at 3:19 pm on June 28, 2021: contributor

Thanks, that’s helpful. Now I remember I used this before. I believe it’s possible to eliminate most of the false positives.

* `CFLAGS=-DVERIFY` is used to greatly reduce the number of "NULL pointer dereference" reports. By the way, it would be nice if there was a `configure` switch for this ;)

Not a switch so far but the easiest way to set environment variables is to pass them to configure, e.g., ./configure CPPFLAGS=-DVERIFY. (Strictly speaking, -DVERIFY is a C preprocessor flag, so it should go to CPPFLAGS even though CFLAGS will also work in this case.)

jonasnick commented at 3:30 pm on June 28, 2021: contributor

ACK 41ed13942bd722ff0ee4d2eb8bef55155549afd6

jonasnick merged this on Jun 28, 2021

jonasnick closed this on Jun 28, 2021

jonasnick commented at 3:31 pm on June 28, 2021: contributor

Thank you @niooss-ledger

niooss-ledger deleted the branch on Jun 28, 2021

sipa commented at 3:55 pm on June 28, 2021: contributor

Posthumous ACK

real-or-random cross-referenced this on Jun 28, 2021 from issue General CI discussion by real-or-random

sipa referenced this in commit c020cbaa5c on Jul 14, 2021

apoelstra cross-referenced this on Jul 27, 2021 from issue Upstream PRs 879, 959, 955, 944, 951, 960, 844, 963, 965 by apoelstra

apoelstra referenced this in commit 196c993d1f on Jul 28, 2021

janus referenced this in commit 62075fc9aa on Nov 5, 2021

patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022

patricklodder referenced this in commit 03002a9013 on Jul 28, 2022

str4d referenced this in commit 7648f6bf55 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

tests: really test the non-var scalar inverse #959