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

Race fix: tdns: strerror() -> strerror_r() #1623

Merged
merged 3 commits into from
Jul 28, 2024

Conversation

michaelortmann
Copy link
Member

@michaelortmann michaelortmann commented Jun 16, 2024

Found by: thommey and michaelortmann
Patch by: michaelortmann
Fixes:

One-line summary:
Fix locking / race for tdns error logging

Additional description (if needed):
Non main threads like tdns must use strerror_r() instead of strerror().

Test cases demonstrating functionality (if applicable):

valgrind --tool=helgrind ./eggdrop -nt BotA.conf
[...]
.tcl dnslookup google.de foo

Without this patch applied, valgrind will report race

@michaelortmann michaelortmann changed the title tdns: strerror() -> strerror_r() Race fix: tdns: strerror() -> strerror_r() Jul 9, 2024
@thommey
Copy link
Member

thommey commented Jul 27, 2024

       int strerror_r(int errnum, char buf[.buflen], size_t buflen);
                      /* XSI-compliant */
       char *strerror_r(int errnum, char buf[.buflen], size_t buflen);
                      /* GNU-specific */
       strerror_r():
           The XSI-compliant version is provided if:
               (_POSIX_C_SOURCE >= 200112L) && ! _GNU_SOURCE
           Otherwise, the GNU-specific version is provided.

I have no words, but both work in our context, LGTM

@vanosg vanosg added this to the v1.10.0 milestone Jul 28, 2024
@vanosg vanosg merged commit 36e0034 into eggheads:develop Jul 28, 2024
23 checks passed
@michaelortmann michaelortmann deleted the tdns.strerror branch July 30, 2024 22:18
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