Closed mattdm closed 1 year ago
lcn2
commented
1 year ago Thank you very much for your clear bug report, @mattdm
We will make this a priority .. although right now we have a regional internet service outage .. this reply is being send thru a satellite phone ($$$-ive). Nevertheless we wanted to let you know that we will address this issue, work offline on the matter, and when our server is restored (in a few days is the ETA) we will get back to you.
lcn2
commented
1 year ago -Wmaybe-uninitialized
gcc -DCALC_SRC -UCUSTOM -Wall -Wextra -pedantic -UUNBAN -fPIC -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -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 -fs tack-clash-protection -fcf-protection -c -o byteswap.o byteswap.c In file included from qmath.h:32, from cmath.h:32, from byteswap.c:28: zmath.h: In function 'swap_HALF_in_ZVALUE': zmath.h:581:36: warning: 'dest_40->v' may be used uninitialized [-Wmaybe-uninitialized] 581 | #define zcopyval(z1,z2) memcpy((z2).v, (z1).v, (z1).len * sizeof(HALF)) | ^I think this might just be because the compiler is not understanding/trusting what's going on with the not_reached() in the check for NULL, but... maybe there's something else going on?
It is not clear from the above text that this warning is caused by line 587 of byteswap.c.
FYI: We do not (yet?) have a compiler that understands
-Wmaybe-uninitialized(we do have a clang compiler that has-Wuninitializedand this line does not trigger a warning from that compiler).
Assuming the root cause of the warning is the compiler's concern that *dest in 587 of byteswap.c could generate a NULL pointer deference, as you suggested line 581 of byteswap.c.
The call to
not_reached()according toattribute.hline 63 will be__builtin_unreachable()if that builtin macro exists orabort()according toattribute.hline 65 if builtin macro does not exist. Our guess is that your gcc compiler understands__builtin_unreachable(), BTW.
Our guess is that your gcc compiler understands __builtin_unreachable() and so it should have dismissed the NULL pointer deference possibility in line 587 of byteswap.c.
QUESTION: If you change line 578 of byteswap.c to use calloc() as in:
dest = calloc(1, sizeof(ZVALUE));Does you compiler trust the action of calloc() and not generate the warning?
Please advise, @mattdm ... thanks!
See the new top of the master branch ... commit 74b833977ba41d00145f60ca1b02276ef2309ec6 or better.
lcn2
commented
1 year ago -Wuse-after-free
gcc -DCALC_SRC -UCUSTOM -Wall -Wextra -pedantic -UUNBAN -fPIC -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -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 -fs tack-clash-protection -fcf-protection -c -o lib_calc.o lib_calc.c lib_calc.c: In function 'find_tty_state': lib_calc.c:805:36: warning: pointer may be used after 'realloc' [-Wuse-after-free] 805 | new_fd_orig = (ttystruct *)realloc(fd_setup, sizeof(fd_orig[0]) * | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 806 | (fd_setup_len+1)); | ~~~~~~~~~~~~~~~~~ lib_calc.c:799:31: note: call to 'realloc' here 799 | new_fd_setup = (int *)realloc(fd_setup, sizeof(fd_setup[0]) * | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 800 | (fd_setup_len+1)); | ~~~~~~~~~~~~~~~~~I think this is is a bug —
reallocoffd_setupon both https://github.com/lcn2/calc/blob/master/lib_calc.c#L799 and https://github.com/lcn2/calc/blob/master/lib_calc.c#L805, and if the memory moves in the first call,fd_setupwill have been freed, right?
This indeed is a bug (not a feature :) )! The code at the end of find_tty_state() should read something like this:
/*
* no empty slots exist, realloc another slot
*/
/* expand fd_setup */
new_fd_setup = (int *)realloc(fd_setup, sizeof(fd_setup[0]) *
(fd_setup_len+1));
if (new_fd_setup == NULL) {
return -1;
}
fd_setup = new_fd_setup;
new_fd_setup[fd_setup_len] = -1;
/* expand fd_orig */
new_fd_orig = (ttystruct *)realloc(fd_orig, sizeof(fd_orig[0]) *
(fd_setup_len+1));
if (new_fd_orig == NULL) {
return -1;
}
fd_orig = new_fd_orig;
/* expand fd_cur */
new_fd_cur = (ttystruct *)realloc(fd_cur, sizeof(fd_cur[0]) *
(fd_setup_len+1));
if (new_fd_cur == NULL) {
return -1;
}
fd_cur = new_fd_cur;
/* expand fd setup length */
++fd_setup_len;
/* return the new slot */
return fd_setup_len-1;
}or even better:
/*
* no empty slots exist, realloc another slot
*/
/* expand fd_orig as an original pre-modified copy of fd_setup */
new_fd_orig = (ttystruct *)realloc(fd_orig, sizeof(fd_orig[0]) *
(fd_setup_len+1));
if (new_fd_orig == NULL) {
return -1;
}
fd_orig = new_fd_orig;
memcpy(fd_orig, fd_setup, sizeof(fd_orig[0]) * (fd_setup_len+1));
/* expand fd_setup */
new_fd_setup = (int *)realloc(fd_setup, sizeof(fd_setup[0]) *
(fd_setup_len+1));
if (new_fd_setup == NULL) {
return -1;
}
fd_setup = new_fd_setup;
new_fd_setup[fd_setup_len] = -1;
/* expand fd_cur */
new_fd_cur = (ttystruct *)realloc(fd_cur, sizeof(fd_cur[0]) *
(fd_setup_len+1));
if (new_fd_cur == NULL) {
return -1;
}
fd_cur = new_fd_cur;
/* expand fd setup length */
++fd_setup_len;
/* return the new slot */
return fd_setup_len-1;
}See the new top of the master branch ... commit 3aaad95443e9c059898e135f61838b2eb4fcb01d and perhaps commit ef6a30c9c92f72ed81c584e68ed52b7aa3d84aa8
Please advise, @mattdm, if that fixes the warning and addresses the bug.
lcn2
commented
1 year ago -Wclobbered
gcc -DCALC_SRC -UCUSTOM -Wall -Wextra -pedantic -UUNBAN -fPIC -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -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 -fs tack-clash-protection -fcf-protection -Wno-implicit -Wno-error=long-long \ -Wno-long-long -c seed.c seed.c: In function 'pseudo_seed': seed.c:481:12: warning: variable 'hash_val' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered] 481 | hash64 hash_val; /* fnv64 hash of sdata */ | ^~~~~~~~I have no idea what's going on here, actually. :)
Well there was a call to setjmp() in pseudo_seed():
sdata.time = time(NULL);
sdata.size = sizeof(sdata);
sdata.prev_hash64_copy = prev_hash64; /* load previous hash */
sdata.call_count_copy = ++call_count; /* update call count */
(void) setjmp(sdata.env);
#if defined(HAVE_ENVIRON)
sdata.environ_copy = environ;
#endif /* HAVE_ENVIRON */
sdata.sdata_p = (char *)&sdata;
/*
* seed the generator with the above sdata
*/
hash_val = private_hash64_buf(hash_val, (char *)&sdata, sizeof(sdata));The gcc compiler with -Wclobbered (our compiler does not yet have that warning) has nil ways of knowing that some later longjmp(sdata.env)) won't be called and thus will reset the hash_val value on the local stack back to the previous state.
There is not a later longjmp(sdata.env)) call. The call to setjmp() was there to add more stuff into building data for a hash value. In fact we wouldn't care if that happened, but the gcc compiler does not know that.
Since that call to setjmp() was only there to add more stuff to hash, and since it not critical to the pseudo_seed(), and since it had the potential to alarm compilers, we removed it.
See new top of the master branch ...commit 83adfaa720ae445084b36aab3ad17532a68870d7
Please advise, @mattdm, if this removes the warning. Thanks!
mattdm
commented
1 year ago Yeah, with those patches cherry-picked, builds without warnings. (In fact, so does f4f19f2.)
lcn2
commented
1 year ago Thank you @mattdm for reporting this issue and for your help in resolving them!
-Wmaybe-uninitialized
I think this might just be because the compiler is not understanding/trusting what's going on with the not_reached() in the check for NULL, but... maybe there's something else going on?
-Wuse-after-free
I think this is is a bug —
reallocoffd_setupon both https://github.com/lcn2/calc/blob/master/lib_calc.c#L799 and https://github.com/lcn2/calc/blob/master/lib_calc.c#L805, and if the memory moves in the first call,fd_setupwill have been freed, right?-Wclobbered
I have no idea what's going on here, actually. :)