Conversation
jblomer
requested review from
ferdymercury
and removed request for
bellenot,
dpiparo and
pcanal
Use UUIDv4 in TDirectory (hence in TDirectoryFile and TFile) and in the RMiniFile. Note that TDirectory is used without gSystem (needed for GetCryptoRandom()) in the context of rootcling. In principle, GetCryptoRandom() should work without a TSystem object. That may be for a later commit. The UUIDv4, unlike the current UUIDv1, is (with high enough probability) globally unique.
jblomer
requested review from
dpiparo,
hahnjo,
pcanal and
silverweed
and removed request for
ferdymercury and
hahnjo
| // In the context of rootcling, we don't have gSystem | ||
| if (gSystem) |
There was a problem hiding this comment.
| // In the context of rootcling, we don't have gSystem | |
| if (gSystem) | |
| // Fallback to the default constructed TUUID if we | |
| // get here before gSystem is initialized. | |
| if (gSystem) |
Test Results 22 files 22 suites 3d 11h 29m 1s ⏱️ For more details on these failures, see this check. Results for commit f17534b. |
|
Impressive PR: thanks for it. It should be backported until 6.36. |
| const char *TUUID::AsString() const | ||
| { | ||
| static char uuid[40]; | ||
| TTHREAD_TLS(char) uuid[40]; |
There was a problem hiding this comment.
Why can't we use thread_local?
There was a problem hiding this comment.
That's what it resolves to. I can use thread_local but this file uses TTHREAD_TLS in other places so I used this for consistency.
Factor out calling the OS crypto random number generator into a free function in the Base package. In turn, TSystem::GetCryptoRandom() can be static and does not rely in gSystem being initialized.
vepadulano
left a comment
vepadulano
left a comment
There was a problem hiding this comment.
LGTM, let's wait for the CI. Just a minor consideration.
| #elif defined(R__GETRANDOM_CLIB) | ||
| return getrandom(buf, len, GRND_NONBLOCK) == len; | ||
| #elif defined(R__USE_URANDOM) | ||
| std::ifstream urandom{"/dev/urandom"}; | ||
| if (!urandom) | ||
| return false; | ||
| urandom.read(reinterpret_cast<char *>(buf), len); | ||
| return true; | ||
| #else |
There was a problem hiding this comment.
Here GRND_NONBLOCK should be fine since we're only interested in the case where we can immediately fill the buffer with the right lenght I believe. OTOH, are we fine with the possibility of partially filled buffer for lengths greater than 256 bytes https://man7.org/linux/man-pages/man2/getrandom.2.html ?
There was a problem hiding this comment.
That's a good point, this was already present before. Perhaps we restrict the input length to 256. We don't use it for anything longer currently.
| Error("GetCryptoRandom", "Not implemented"); | ||
| return -1; | ||
| if (len < 0) { | ||
| return false; |
There was a problem hiding this comment.
Should this be:
| return false; | |
| return -1; |
6 participants
Use UUIDv4 in TDirectory (hence in TDirectoryFile and TFile) and in the RMiniFile. Note that TDirectory is used without gSystem (needed for GetCryptoRandom()) in the context of rootcling. In principle, GetCryptoRandom() should work without a TSystem object. That may be for a later PR.
The UUIDv4, unlike the current UUIDv1, is (with high enough probability) globally unique.
Also fixes a race in TUUID::AsString().
EDIT: The last two commits factor out the
GetCryptoRandom()into a free function, so that it can be used unconditionally in the I/O.Fixes #22015
Replaces #22016