search

pqov / pqov-paper

Creative Commons Zero v1.0 Universal
7 stars 2 forks source link

Undefined behavior warning #4

Open thomwiggers opened 1 year ago

thomwiggers commented 1 year ago

gcc shows the following warning when compiling:

src/ov_publicmap.c: In function ‘accu_eval_quad_gf256.constprop’:                                              
src/ov_publicmap.c:278:30: warning: iteration 212 invokes undefined behavior [-Waggressive-loop-optimizations] 
  278 |          unsigned idx = _xixj[j];                                                                      
      |                         ~~~~~^~~                                                                       
src/ov_publicmap.c:277:13: note: within this loop                                                              
  277 |       for(;j<o;j++) {                                                                                  
      |            ~^~
thomwiggers commented 1 year ago

Valgrind, using --track-origins=yes also complains about loads of uninitialized reads around that code:

 Use of uninitialised value of size 8
    at 0x116AC8: accu_eval_quad_gf256.constprop.0 (in /home/thom/git/phd/PQClean/pqov-paper/sign_api-benchmark)
    by 0x1180A6: ov_publicmap (in /home/thom/git/phd/PQClean/pqov-paper/sign_api-benchmark)
    by 0x11233C: ov_verify (in /home/thom/git/phd/PQClean/pqov-paper/sign_api-benchmark)
    by 0x11ACF7: crypto_sign_open (in /home/thom/git/phd/PQClean/pqov-paper/sign_api-benchmark)
    by 0x10977E: main (in /home/thom/git/phd/PQClean/pqov-paper/sign_api-benchmark)
  Uninitialised value was created by a stack allocation
    at 0x10D284: gf256mat_gaussian_elim_ref (in /home/thom/git/phd/PQClean/pqov-paper/sign_api-benchmark)

It seems that not every part of mat is properly initialized.

devillegna commented 1 year ago

I believe it is caused by gcc for optimization. If you try gcc -O2, then all these warnings and uninitialized reads are gone. clang uses the same optimization and loads uninitialized data with -O3 flags but shows on warning while compiling.

thomwiggers commented 1 year ago

By definition, GCC can not cause undefined behaviour, because that is a property of the C code. The compiler is simply making assumptions that it can safely perform certain optimizations...

devillegna commented 1 year ago

It might be the false positive alarm from the valgrind instead of GCC. Please see https://valgrind.org/docs/manual/quick-start.html#quick-start.caveats . It suggest to run valgrind Memcheck using -O1 optimization.

thomwiggers commented 1 year ago

The compiler warning originates in GCC, not Valgrind

devillegna commented 1 year ago

There are 2 ongoing issues in this issue thread.

  1. gcc warning: 
    src/ov_publicmap.c: In function ‘accu_eval_quad_gf256.constprop’:                                              
src/ov_publicmap.c:278:30: warning: iteration 212 invokes undefined behavior [-Waggressive-loop-optimizations] 
278 |          unsigned idx = _xixj[j];                                                                      
  |                         ~~~~~^~~                                                                       
src/ov_publicmap.c:277:13: note: within this loop                                                              
277 |       for(;j<o;j++) {                                                                                  
  |            ~^~

I don't know what to do about the warning. Maybe you can simply remove the -Werror flag or use -O1 while using gcc. The warning doesn't appear with gcc -O1 optimization in my laptop.

  1. valgrind complaints: valgrind suggests to use -O1 for the memory test (https://valgrind.org/docs/manual/quick-start.html#quick-start.caveats). It might give false positive errors with higher optimization levels. With -O1 optimization, valgrind complains nothing.