eggdrop icon indicating copy to clipboard operation
eggdrop copied to clipboard

Fix format-truncation warning

Open michaelortmann opened this issue 1 year ago • 3 comments

Found by: Geo Patch by: michaelortmann Fixes:

One-line summary: Fix format-truncation warning

Additional description (if needed): This PR truncates the return of strerror_r() to a reasonable len. Better, than to raise the dtn->strerror buffer size, because struct dns is per dns thread, so it should be small in size to avoid any memory bloat.

Test cases demonstrating functionality (if applicable): Fixes:

gcc -g -O2 -pipe -Wall -I.. -I..  -DHAVE_CONFIG_H -I/usr/include -Og -g3 -DDEBUG -fsanitize=address -DDEBUG_ASSERT -DDEBUG_MEM -DDEBUG_DNS  -c dns.c
dns.c: In function ‘thread_dns_ipbyhost’:
dns.c:571:99: warning: ‘%s’ directive output may be truncated writing up to 2047 bytes into a region of size 147 [-Wformat-truncation=]
  571 |     snprintf(dtn->strerror, sizeof dtn->strerror, "dns: thread_dns_ipbyhost(): getaddrinfo(): %s: %s", gai_strerror(error), ebuf);
      |                                                                                                   ^~                        ~~~~
dns.c:571:5: note: ‘snprintf’ output 46 or more bytes (assuming 2093) into a destination of size 192
  571 |     snprintf(dtn->strerror, sizeof dtn->strerror, "dns: thread_dns_ipbyhost(): getaddrinfo(): %s: %s", gai_strerror(error), ebuf);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

michaelortmann avatar Jul 30 '24 22:07 michaelortmann

I have reverted to strerror for now, this needs a larger discussion.

According to man strerror_r glibc recommends 1024 bytes (because that's what they internally allocate). Furthermode, there's strerror_r from GNU and strerror_r in general, both are incompatible with each other and we do enable _GNU_SOURCE in config.h automatically - so I am not sure we can rule out the char* returning version is used. If we use the char* returning version, the return value matters and should be used instead of the buffer because it might not be used. What a mess.

thommey avatar Aug 08 '24 01:08 thommey

I have reverted to strerror for now, this needs a larger discussion.

After discussion on IRC, here is a new fix, that keeps things stupid simple, by just printing the raw errno.

Dont use strerror() in thread and dont use strerror_r() due to GNU / POSIX portability complexity.

The errno number could be resolved manually in rare case this line is hit.

Ready for review again.

michaelortmann avatar Aug 08 '24 23:08 michaelortmann

I like the new approach, very reasonable

thommey avatar Aug 08 '24 23:08 thommey