Overflow logging.c)nwipe_log_sysinfo) #202
Comments
|
Hi Martyn, I'll take a look at that today. |
|
I'm not seeing this warning when i compile under sid. How are you compiling it with ? |
|
Im building with hardening (all) options see https://wiki.debian.org/Hardening |
|
I don't see any issues with the code, it generates a command with a maximum length of 37 characters including the NULL so fits easily into the string cmd[50]. Maybe if I used snprintf where you can specify a limit, i.e 50 then it would be get rid of the warning. Irrespective of that I don't think there is an issue with the code, I think it's just a warning thats there to inform you that there is a safer way to do this using snprinf as i can set bounds. Are you able to let it go through as is and I'll make the change to use snprintf in the next release ? Let me know what you want me to do. :-) ps I'll check out the link above and try and reproduce and see if using snprintf instead clears the warning. |
|
Try this export CFLAGS="-g -O2 -fstack-protector-strong -Wformat -Werror=format-security" |
|
Still not seeing it using the then |
|
@martijnvanbrummelen OK !, got it, under Ubuntu it doesn't show the warning using the CFLAGS you provided but under SID it does. I'll do a patch using snprintf and see if it clears the warning. |
|
@martijnvanbrummelen So using snprintf didn't fix the warning. However the fix was actually easier. I changed: The compiler was happy with that !. It was very specific i.e. 540 produced a warning that the NULL terminator would over write the buffer. 541 bytes was what it required. Do you want to try that on your system ? |
|
@martijnvanbrummelen In the warning it says "directive writing upto 527 bytes" and in the second part of the warning it says 541 bytes. The compiler has basically looked at the total size of the char array and assuming the entire 504 byte array could be written to the cmd buffer. Of course it fails to recognise the array is full of null terminated strings with a maximum size of 24 bytes. Even if you made the array const char the warning still comes up. So basically the fix to get rid of the warning is to make the destination string the same size as the array containing all the strings, i.e 504+13(dmidecode -s )+24(the maximum length of one string. = 541 bytes. |
|
Fixed by #204 |
|
@martijnvanbrummelen I've applied the overflow warning patch to the master (not 0.27 release). I've done it a better way than hard coding the array size. I've used 'sizeof' to determine what the size of the character array that we construct the dmidecode command in should be. This is a better way to determine the array size as it's determined by the compiler. It also means that if somebody adds extra dmidecode keywords to the keyword array, the destination array size is automatically adjusted by the compiler. Maybe it would be better to skip 0.27 and just jump to 0.28 ? The current master has some nice fixes for the GUI when resizing the terminal. Or we could just ignore the overflow warning in 0.27 as it's not a 'real' bug. Let me know what you would prefer me to do. |
|
I think we should drop 0.27 and release 0.28 which comes with at least this fix and the man page fix. |
No problem, I'll update the various version files etc and do a release on 0.28, run the checks on all the various distros as before and then I'll let you know when 0.28 is ready to upload to debian. It will probably be ready by end of business tomorrow. 0.28 will come with the following plus all the 0.27 updates. CHANGELOG.md v0.28
v0.27
Proposed for 0.29
|
While compiling a overflow issue(warning) is found:
logging.c: In function ‘nwipe_log_sysinfo’:
logging.c:438:37: warning: ‘%s’ directive writing up to 527 bytes into a region of size 37 [-Wformat-overflow=]
438 | sprintf( cmd, "dmidecode -s %s", &dmidecode_keywords[keywords_idx][0] );
| ^~
In file included from /usr/include/stdio.h:867,
from logging.c:29:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:36:10: note: ‘__builtin___sprintf_chk’ output between 14 and 541 bytes into a destination of size 50
36 | return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
37 | __bos (__s), __fmt, __va_arg_pack ());
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
char cmd[50];
The text was updated successfully, but these errors were encountered: