Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix format-truncation warning #1663

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

michaelortmann
Copy link
Member

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);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Member

@skralg skralg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@thommey
Copy link
Member

thommey commented Aug 8, 2024

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.

and reset this branch to implement a new solution
…strerror_r() due to GNU / POSIX portability complexity
@michaelortmann
Copy link
Member Author

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.

src/dns.c Outdated Show resolved Hide resolved
@thommey
Copy link
Member

thommey commented Aug 8, 2024

I like the new approach, very reasonable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants