As per C89 standard (https://www.open-std.org/JTC1/sc22/wg14/www/docs/n1256.pdf, section 6.5.3 "Unary operators") we are only required to parenthesize a sizeof() call if the argument is a type name, which is not the case here:
sizeof unary-expression
sizeof ( type-name )
However, it is less surprising to err on the side of caution, and uniformly enclose in parentheses every sizeof() invocation across the code base.
No functional changes.
The upstream function secp256k1_ge_set_xo_var() in src/group_impl.h returns an
int signalling an error condition, but it is not marked with
SECP256K1_WARN_UNUSED_RESULT.
Thus, when compiling the Frost module, no warning was generated when
deserialize_frost_signature() had no check covering the case that
secp256k1_ge_set_xo_var() could fail.
The GCC static analyzer found two potential execution paths in
secp256k1_frost_verify() that would lead to undefined behaviour. For brevity,
the issues are not reported here, but only in the Github PR associated with
this commit (https://github.com/bancaditalia/secp256k1-frost/pull/5).
As of today (2023-05-29), in bitcoin-core secp256k1, secp256k1_ge_set_xo_var()
is still not marked SECP256K1_WARN_UNUSED_RESULT, see:
https://github.com/bitcoin-core/secp256k1/blob/908e02d596b66203788e8945b1f9c93ff28a4536/src/group_impl.h#L280
It may make sense to propose upstream to add that annotation.
In order to replicate the issues fixed by this commit, compile with:
./configure SECP_CFLAGS="-fanalyzer -fanalyzer-transitivity" --disable-tests --disable-exhaustive-tests --disable-benchmark --enable-experimental --enable-module-frost
The leak presented if compute_binding_factors() failed. In that case the error
reporting path would not deallocate the binding factor that were allocated just
above.
Found with gcc 13.1 via:
./configure SECP_CFLAGS="-fanalyzer -fanalyzer-transitivity" --disable-tests --disable-exhaustive-tests --disable-benchmark --enable-experimental --enable-module-frost
The analyzer found 3 leaks at main_impl.h:1403, all of them solved by this
change. The branch analysis of the first leak was:
```
In file included from src/secp256k1.c:767:
src/modules/frost/main_impl.h: In function 'secp256k1_frost_aggregate':
src/modules/frost/main_impl.h:1454:1: warning: leak of 'binding_factors.binding_factors' [CWE-401] [-Wanalyzer-malloc-leak]
1454 | }
| ^
'secp256k1_frost_aggregate': events 1-2
|
| 1365 | SECP256K1_API int secp256k1_frost_aggregate(const secp256k1_context *ctx,
| | ^~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (1) entry to 'secp256k1_frost_aggregate'
[...]
| 1394 | binding_factors.binding_factors = (secp256k1_scalar *)
| 1395 | checked_malloc(&default_error_callback, num_signers * sizeof(secp256k1_scalar));
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (10) calling 'checked_malloc' from 'secp256k1_frost_aggregate'
|
+--> 'checked_malloc': events 11-15
|
|src/util.h:118:31:
| 118 | static SECP256K1_INLINE void *checked_malloc(const secp256k1_callback* cb, size_t size) {
| | ^~~~~~~~~~~~~~
| | |
| | (11) entry to 'checked_malloc'
| 119 | void *ret = malloc(size);
| | ~~~~~~~~~~~~
| | |
| | (12) allocated here
[...]
'secp256k1_frost_aggregate': events 29-30
|
| 1403 | if (compute_binding_factors(&binding_factors, msg32, 32, num_signers, commitments) == 0) {
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (29) ...to here
|......
| 1454 | }
| | ~
| | |
| | (30) 'binding_factors.binding_factors' leaks here; was allocated at (12)
|
```
In the next commit, this will allow us to call it from inside
secp256k1_frost_sign().
No functional changes.
In this way we have a single way of clearing binding factors across the code
base. Before this change, secp256k1_frost_sign() was the only function that
rolled its own code.
No functional changes.
A reference per the pkg-config sytax can be found in "man 5 pc", or online at
https://man.freebsd.org/cgi/man.cgi?query=pc&sektion=5&n=1
EXAMPLES
An example .pc file:
# This is a comment
prefix=/home/kaniini/pkg # this defines a variable
exec_prefix=${prefix} # defining another variable with a substitution
libdir=${exec_prefix}/lib
includedir=${prefix}/include
Name: libfoo # human-readable name
Description: an example library called libfoo # human-readable description
Version: 1.0
URL: http://www.pkgconf.org
Requires: libbar > 2.0.0
Conflicts: libbaz <= 3.0.0
Libs: -L${libdir} -lfoo
Libs.private: -lm
Cflags: -I${includedir}/libfoo
As per C89 standard (https://www.open-std.org/JTC1/sc22/wg14/www/docs/n1256.pdf,
section 6.5.3 "Unary operators") we are only required to parenthesize a sizeof()
call if the arguent is a type name, which is not the case here.
However, it is less surprising to err on the side of caution, and uniformly
enclose in parentheses every sizeof() invocation across the code base.
No functional changes.