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: make zfs_strerror really thread-safe and portable #16923

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

rkojedzinszky
Copy link
Contributor

Fixes #15793
Fixes #16640
Closes #16916

Motivation and Context

#15793 wanted to make zfs_strerror threadsafe, unfortunately, it turned out that strerror_l() usage was wrong, and also, some libc implementations dont have strerror_l().

Description

zfs_strerror() now simply calls original strerror() and copies the result to a thread-local buffer, then returns that.

How Has This Been Tested?

Code did compile.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

include/libzutil.h Outdated Show resolved Hide resolved
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jan 3, 2025
@rkojedzinszky
Copy link
Contributor Author

May it happen that strerror_r() is available in most libc implementations, and the mutex could be dropped?

@amotin
Copy link
Member

amotin commented Jan 3, 2025

May it happen that strerror_r() is available in most libc implementations, and the mutex could be dropped?

I can't speak about other OS'es, but in FreeBSD I see it for 20+ years since FreeBSD 4.x.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 3, 2025
@rkojedzinszky
Copy link
Contributor Author

Did a quick check, found that uClibc and Musl do have strerror_r(). Glibc and FreeBSD have it too, so maybe, we could remove that piece of code?

@amotin
Copy link
Member

amotin commented Jan 3, 2025

From FreeBSD's man page: "The strerror_r() function conforms to IEEE Std 1003.1-2001 (“POSIX.1”)". Sounds like quite a while ago.

@github-actions github-actions bot removed the Status: Accepted Ready to integrate (reviewed, tested) label Jan 3, 2025
@rkojedzinszky
Copy link
Contributor Author

uClibc and Musl are known to be small libc implementations, perhaps with missing functions. Howewer, I found that both have strerror_r(), and it seems not to be configurable/optional.

@behlendorf
Copy link
Contributor

There appears to be two versions of strerror_r(), an "XSI-compliant version specified in POSIX.1-2001" and a "GNU-specific version". So that's something to be careful about.

From https://manpages.ubuntu.com/manpages/bionic/man3/strerror.3.html

int strerror_r(int errnum, char *buf, size_t buflen); /* XSI-compliant */
char *strerror_r(int errnum, char *buf, 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.

It's also not clear to me from the documentation if strerror_r() honors the locale. That's something we'd need to test.

@rkojedzinszky
Copy link
Contributor Author

Good catch, the GNU version may not write to the supplied buffer. Oh.

@rkojedzinszky
Copy link
Contributor Author

Regarding locale, it should behave as strerror().

@rkojedzinszky
Copy link
Contributor Author

Unfortunately, openzfs has AM_CPPFLAGS += -D_GNU_SOURCE in config/Rules.am, which definitely makes the GNU version available with glibc. FreeBSD on the other hand definitely provides the POSIX variant and behavior, and there are the other libcs too. So, we must stick back to simple strerror() guarder by the pthread mutex.

Signed-off-by: Richard Kojedzinszky <[email protected]>
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jan 3, 2025
@behlendorf behlendorf merged commit dc0324b into openzfs:master Jan 4, 2025
22 of 25 checks passed
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 4, 2025
openzfs#15793 wanted to make zfs_strerror threadsafe, unfortunately, it
turned out that strerror_l() usage was wrong, and also, some libc 
implementations dont have strerror_l().

zfs_strerror() now simply calls original strerror() and copies the 
result to a thread-local buffer, then returns that.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
Closes openzfs#16640
Closes openzfs#16923
rkojedzinszky added a commit to dravanet/openzfs-zfs that referenced this pull request Jan 5, 2025
openzfs#15793 wanted to make zfs_strerror threadsafe, unfortunately, it
turned out that strerror_l() usage was wrong, and also, some libc 
implementations dont have strerror_l().

zfs_strerror() now simply calls original strerror() and copies the 
result to a thread-local buffer, then returns that.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
Closes openzfs#16640
Closes openzfs#16923
rkojedzinszky added a commit to dravanet/truenas-middleware that referenced this pull request Jan 5, 2025
rkojedzinszky added a commit to dravanet/openzfs-zfs that referenced this pull request Jan 6, 2025
turned out that strerror_l() usage was wrong, and also, some libc
implementations dont have strerror_l().

zfs_strerror() now simply calls original strerror() and copies the
result to a thread-local buffer, then returns that.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Kojedzinszky <[email protected]>
Closes openzfs#15793
Closes openzfs#16640
Closes openzfs#16923

Upstream commit dc0324b
rkojedzinszky added a commit to dravanet/truenas-middleware that referenced this pull request Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants