util: Replace non-threadsafe strerror #24933
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
laanwj
commented
Apr 20, 2022
laanwj
force-pushed
the
2022-04-strerror-threadsafe
branch
4 times, most recently
from
fa7449f
to
009331a
Compare
laanwj
force-pushed
the
2022-04-strerror-threadsafe
branch
from
ac117bf
to
9d5fa88
Compare
|
To test on your favorite platform: $ c++ error-test.cpp util/syserror.cpp -o error-test -DHAVE_CONFIG_H -I.(make sure to define error-test.cpp: #include <util/syserror.h>
#include <iostream>
int main()
{
for (int i = -200; i < 200; ++i) {
std::cout << SysErrorString(i) << std::endl;
}
} |
|
Concept ACK on removing |
build options
error_test output (debian testing)
|
jonatack
reviewed
Apr 21, 2022
There was a problem hiding this comment.
ACK 9d5fa88 code review, at each commit built and tested running error_test on debian testing 5.16.18-1 (2022-03-29) with -DHAVE_CONFIG_H defined, verified in man strerror that the strerror_r() function used here is similar to strerror(), but is thread safe. Tested that both the gnu and posix paths in SysErrorString() work for me locally. Looked up strerror_s in https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strerror-s-strerror-s-wcserror-s-wcserror-s and that it is thread-safe as well.
laanwj
force-pushed
the
2022-04-strerror-threadsafe
branch
2 times, most recently
from
fdc18af
to
e391567
Compare
|
ACK e391567 |
|
Approach ACK |
|
What about: diff --git a/test/lint/lint-locale-dependence.py b/test/lint/lint-locale-dependence.py
index 2abf1be6b3..837224a1c4 100755
--- a/test/lint/lint-locale-dependence.py
+++ b/test/lint/lint-locale-dependence.py
@@ -144,7 +144,7 @@ LOCALE_DEPENDENT_FUNCTIONS = [
"strcasecmp",
"strcasestr",
"strcoll", # LC_COLLATE
- #"strerror",
+ "strerror",
"strfmon",
"strftime", # LC_TIME
"strncasecmp", |
laanwj
force-pushed
the
2022-04-strerror-threadsafe
branch
from
e391567
to
c1da6ca
Compare
|
Rebased (to get the Python port of the linter) and added a commit that updates the linter.
|
laanwj
force-pushed
the
2022-04-strerror-threadsafe
branch
2 times, most recently
from
9703754
to
f739e65
Compare
Some uses of non-threadsafe `strerror` have snuck into the code since they were removed in bitcoin#4152. Add a wrapper `SysErrorString` for thread-safe strerror alternatives and replace all uses of `strerror` with this.
Deduplicate code and error checks by making sure `s` stays `nullptr` in case of error. Return "Unknown error" instead of an empty string in this case.
Increase the error message buffer to 1024 as recommended in the manual page (Thanks Jon Atack)
Add `strerror` to the locale-dependence linter to catch its use. Add exemptions for bdb interface code (false positive) and strerror.cpp (the only allowed use). Also fix a bug in the regexp so that `_r` and `_s` variants are detected again.
laanwj
force-pushed
the
2022-04-strerror-threadsafe
branch
from
f739e65
to
e3a06a3
Compare
|
ACK e3a06a3 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 4, 2022
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
May 21, 2022
Some uses of non-threadsafe `strerror` have snuck into the code since they were removed in bitcoin#4152. Add a wrapper `SysErrorString` for thread-safe strerror alternatives and replace all uses of `strerror` with this. Github-Pull: bitcoin#24933 Rebased-From: 46971c6
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
May 21, 2022
Github-Pull: bitcoin#24933 Rebased-From: e7f2f77
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
May 21, 2022
Deduplicate code and error checks by making sure `s` stays `nullptr` in case of error. Return "Unknown error" instead of an empty string in this case. Github-Pull: bitcoin#24933 Rebased-From: 718da30
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
May 21, 2022
Increase the error message buffer to 1024 as recommended in the manual page (Thanks Jon Atack) Github-Pull: bitcoin#24933 Rebased-From: f00fb12
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
May 21, 2022
Add `strerror` to the locale-dependence linter to catch its use. Add exemptions for bdb interface code (false positive) and strerror.cpp (the only allowed use). Also fix a bug in the regexp so that `_r` and `_s` variants are detected again. Github-Pull: bitcoin#24933 Rebased-From: e3a06a3
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
May 22, 2022
Add `strerror` to the locale-dependence linter to catch its use. Add exemptions for bdb interface code (false positive) and strerror.cpp (the only allowed use). Also fix a bug in the regexp so that `_r` and `_s` variants are detected again. Github-Pull: bitcoin#24933 Rebased-From: e3a06a3
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some uses of non-threadsafe
strerrorhave snuck into the code since they were removed in #4152. Add a wrapperSysErrorStringfor thread-safe strerror alternatives (with code fromNetworkErrorString) and replace all uses ofstrerrorwith this.Edit: I've also added a commit that refactors the code so that buf[] is never read at all if the function fails, making some fragile-looking code unnecessary.
Edit2: from the linux manpage:
As the function can be called from any thread at any time, using a non-thread-safe function is unacceptable.