1826650 - Dangling pointer in cmd/ecperf/ecperf.c (breaks compilation with recent gcc and -Wall)

Status: RESOLVED FIXED

(NSS :: Build, defect, P3)

Description

[Adam Williamson]

Steps to reproduce:

Build current nss with a very recent GCC 13 snapshot, and -Wall.

Actual results:

The build fails with these errors:

ecperf.c: In function 'genericThread':
ecperf.c:99:24: error: storing the address of local variable 'sig' in '*threadData.p2' [-Werror=dangling-pointer=]
99 | threadData->p2 = (void *)&sig;
| ^
ecperf.c:91:13: note: 'sig' declared here
91 | SECItem sig;
| ^~~
ecperf.c:91:13: note: 'data' declared here
ecperf.c: In function 'PKCS11Thread':
ecperf.c:71:24: error: storing the address of local variable 'sig' in '*threadData.p2' [-Werror=dangling-pointer=]
71 | threadData->p2 = (void *)&sig;
| ^
ecperf.c:53:13: note: 'sig' declared here
53 | SECItem sig;
| ^~~
ecperf.c:53:13: note: 'data' declared here
cc1: all warnings being treated as errors

Expected results:

The build should succeed (i.e. the code shouldn't create dangling pointers).

Comment 1

[Adam Williamson]

I'm not enough of a C expert to guess at an appropriate fix here. Don't sig and threadData both go out of scope at the end of the function so there isn't really a problem, exactly? Does threadData need to be explicitly destroyed at the end?

Comment 2

[Adam Williamson]

Note, this error doesn't happen when building with a GCC snapshot from 2023-02-21 (commit 467eb8130c9dab8ce72a4f6f39437c1ff382a90c ). It does happen when building with a GCC snapshot from 2023-04-01 (commit 21536f033a889b9c0aa53f7888469bab3d9296bd ). So it seems like gcc only started complaining about this very recently.

Comment 3

[Adam Williamson]

The most obvious relevant commit upstream, I guess, is fdac2bea53bf5e7214352e2afd5542254c3156cb - "-Wdangling-pointer: don't mark SSA lhs sets as stores". I don't understand 80% of that commit message, but it's in the right area and timeframe.

The commit IDs from my last comment are from https://gcc.gnu.org/git/?p=gcc.git;a=log;h=refs/vendors/redhat/heads/gcc-13-branch , btw, not from upstream master , sorry if that was confusing.

Comment 4

[John Schanck [:jschanck]]

Attachment: Bug 1826650 - cmd/ecperf: fix dangling pointer warning on gcc 13. r=djackson

Comment 5

[John Schanck [:jschanck]]

I don't have GCC 13 readily available. Could you test the attached patch?

Comment 6

[Adam Williamson]

Thanks! Will do.

Comment 7

[Adam Williamson]

That exchanges one problem for another, it seems :D

ecperf.c: In function 'genericThread':
ecperf.c:120:24: error: 'tmp' may be used uninitialized [-Werror=maybe-uninitialized]
  120 |         threadData->p2 = tmp;
      |         ~~~~~~~~~~~~~~~^~~~~
ecperf.c:98:11: note: 'tmp' was declared here
   98 |     void *tmp;
      |           ^~~
ecperf.c: In function 'PKCS11Thread':
ecperf.c:86:24: error: 'tmp' may be used uninitialized [-Werror=maybe-uninitialized]
   86 |         threadData->p2 = tmp;
      |         ~~~~~~~~~~~~~~~^~~~~
ecperf.c:56:11: note: 'tmp' was declared here
   56 |     void *tmp;
      |           ^~~
cc1: all warnings being treated as errors

Comment 8

[John Schanck [:jschanck]]

Oops. I've updated the patch. Thanks for your help!

Comment 9

[Adam Williamson]

OK, it looks like a scratch build with the updated patch got past ecperf.c successfully:

make[3]: Entering directory '/builddir/build/BUILD/nss-3.89/nss/cmd/ecperf'
gcc -o Linux6.2_x86_64_gcc_glibc_PTH_64_OPT.OBJ/ecperf.o -c -std=c99 -O2 -fPIC  -m64 -pipe -ffunction-sections -fdata-sections -DHAVE_STRERROR -DLINUX -Dlinux -Wall -Wshadow -Werror -DXP_UNIX -DXP_UNIX -UDEBUG -DNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -DSDB_MEASURE_USE_TEMP_DIR -D_REENTRANT -UDEBUG -DNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -DSDB_MEASURE_USE_TEMP_DIR -D_REENTRANT -DNSS_DISABLE_DBM -DSEED_ONLY_DEV_URANDOM -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_STATIC_LIBS -I../../nss/lib/softoken -I/builddir/build/BUILD/nss-3.89/dist/include/nspr -iquote ../../../dist/Linux6.2_x86_64_gcc_glibc_PTH_64_OPT.OBJ/../public/nss -iquote ../../../dist/Linux6.2_x86_64_gcc_glibc_PTH_64_OPT.OBJ/../private/nss -I../../../dist/Linux6.2_x86_64_gcc_glibc_PTH_64_OPT.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -O2  -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64   -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -Wno-array-parameter ecperf.c
rm -f Linux6.2_x86_64_gcc_glibc_PTH_64_OPT.OBJ/ecperf

so I think that version looks good.

Comment 10

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/9659929e7dff1065ffcc708f9abeb82f5937236b