msan: use of uninitialized value in secp256k1_scalar_mul_shift_var #1511

Building master (05bfab69aef3622f77f754cfb01220108a109c91) in the following way (same flags as we use in our MSAN CI), results in the following failure:

Ubuntu clang version 17.0.6 (5build1) (x86_64)
./autogen.sh
./configure CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls" CC=clang
Build Options:
  with external callbacks = no
  with benchmarks         = yes
  with tests              = yes
  with ctime tests        = yes
  with coverage           = no
  with examples           = no
  module ecdh             = yes
  module recovery         = no
  module extrakeys        = yes
  module schnorrsig       = yes
  module ellswift         = yes

  asm                     = x86_64
  ecmult window size      = 15
  ecmult gen prec. bits   = 4

  valgrind                = yes
  CC                      = clang
  CPPFLAGS                = 
  SECP_CFLAGS             = -O2  -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wconditional-uninitialized -Wreserved-identifier -fvisibility=hidden 
  CFLAGS                  = -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls
  LDFLAGS                 = 
make check -j17
<snip>
cat test-suite.log 
==============================================
   libsecp256k1 0.4.2-dev: ./test-suite.log
==============================================

# TOTAL: 3
# PASS:  2
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: tests
===========

test count = 64
random seed = 799c333349f126d5ee67e7ac48fa9dc8
==3962699==WARNING: MemorySanitizer: use-of-uninitialized-value
    [#0](/bitcoin-core-secp256k1/0/) 0x556d1c78cb8d in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:909:5
    [#1](/bitcoin-core-secp256k1/1/) 0x556d1c7894d8 in secp256k1_scalar_split_lambda /root/secp256k1/src/scalar_impl.h:162:5
    [#2](/bitcoin-core-secp256k1/2/) 0x556d1c793be0 in secp256k1_ecmult_strauss_wnaf /root/secp256k1/src/ecmult_impl.h:256:9
    [#3](/bitcoin-core-secp256k1/3/) 0x556d1c674a50 in secp256k1_ecmult /root/secp256k1/src/ecmult_impl.h:355:5
    [#4](/bitcoin-core-secp256k1/4/) 0x556d1c674a50 in secp256k1_ecdsa_sig_verify /root/secp256k1/src/ecdsa_impl.h:212:5
    [#5](/bitcoin-core-secp256k1/5/) 0x556d1c70daba in run_proper_context_tests /root/secp256k1/src/tests.c:460:5
    [#6](/bitcoin-core-secp256k1/6/) 0x556d1c6b4257 in main /root/secp256k1/src/tests.c:7554:5
    [#7](/bitcoin-core-secp256k1/7/) 0x7f0ab902a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    [#8](/bitcoin-core-secp256k1/8/) 0x7f0ab902a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    [#9](/bitcoin-core-secp256k1/9/) 0x556d1c5dc2a4 in _start (/root/secp256k1/tests+0x322a4) (BuildId: c02b23f447c54427d446d0e1ed03077d0b65a6a6)

  Uninitialized value was stored to memory at
    [#0](/bitcoin-core-secp256k1/0/) 0x556d1c78cb86 in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:909:38
    [#1](/bitcoin-core-secp256k1/1/) 0x556d1c7894d8 in secp256k1_scalar_split_lambda /root/secp256k1/src/scalar_impl.h:162:5
    [#2](/bitcoin-core-secp256k1/2/) 0x556d1c793be0 in secp256k1_ecmult_strauss_wnaf /root/secp256k1/src/ecmult_impl.h:256:9
    [#3](/bitcoin-core-secp256k1/3/) 0x556d1c674a50 in secp256k1_ecmult /root/secp256k1/src/ecmult_impl.h:355:5
    [#4](/bitcoin-core-secp256k1/4/) 0x556d1c674a50 in secp256k1_ecdsa_sig_verify /root/secp256k1/src/ecdsa_impl.h:212:5
    [#5](/bitcoin-core-secp256k1/5/) 0x556d1c70daba in run_proper_context_tests /root/secp256k1/src/tests.c:460:5
    [#6](/bitcoin-core-secp256k1/6/) 0x556d1c6b4257 in main /root/secp256k1/src/tests.c:7554:5
    [#7](/bitcoin-core-secp256k1/7/) 0x7f0ab902a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    [#8](/bitcoin-core-secp256k1/8/) 0x7f0ab902a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    [#9](/bitcoin-core-secp256k1/9/) 0x556d1c5dc2a4 in _start (/root/secp256k1/tests+0x322a4) (BuildId: c02b23f447c54427d446d0e1ed03077d0b65a6a6)

  Uninitialized value was created by an allocation of 'l' in the stack frame
    [#0](/bitcoin-core-secp256k1/0/) 0x556d1c78c151 in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:893:5

SUMMARY: MemorySanitizer: use-of-uninitialized-value /root/secp256k1/src/scalar_4x64_impl.h:909:5 in secp256k1_scalar_mul_shift_var
Exiting
FAIL tests (exit status: 1)

Related to https://github.com/bitcoin/bitcoin/pull/29742.

I guess it does not happen in valgrind, dropping -fsanitize=memory first in the CFLAGS?

The accessed value is created by asm in secp256k1_scalar_mul_512. #1496 added annotations to secp256k1_scalar_reduce_512, but not to secp256k1_scalar_mul_512. So we'll simply need to add those. I'm not sure why this wasn't noticed in #1496, i.e., why MSAN was happy without the annotation. (Perhaps differences in clang versions etc?) cc @theuni

Huh, yeah, I'm not sure either. I never bumped into this with my testing. Will have a look and try to PR a fix.

It took me a while to reproduce... indeed clang-15 does not complain, but clang-17 does. Seeing as it detected something that clang-15 missed, but smarter tracking could potentially understand vars set in asm, it's hard to say if newer clang is smarter or dumber here :p

Either way, I agree with @real-or-random that this needs annotations. Will PR a fix.

Closing as fixed now that #1512 is merged.