Re: [Mingw-w64-public] [PATCH] Use rand_s in mkstemp function
Dnia środa, 6 marzec 2024 10:09:30 (+01:00), Martin Storsjö napisał(a):
> On Mon, 4 Mar 2024, Martin Storsjö wrote:
>
> > Looking closer at our mkstemp implementation, we have this loop:
> >
> >/*
> >Like OpenBSD, mkstemp() will try at least 2 ** 31 combinations before
> >giving up.
> > */
> >for (i = 0; i >= 0; i++) {
> >for(j = index; j < len; j++) {
> >template_name[j] = letters[rand () % 62];
> >}
> >fd = _sopen(template_name,
> >_O_RDWR | _O_CREAT | _O_EXCL | _O_BINARY,
> >_SH_DENYNO, _S_IREAD | _S_IWRITE);
> >if (fd != -1) return fd;
> >if (fd == -1 && errno != EEXIST) return -1;
> >}
> >
> > This should retry an absolutely insane number of times, so as long as one
> > process finds a unique file name and stops iterating, the other parallel
> > process should also find a unique one soon after, one would expect.
> >
> > So if this fails, it looks like something is fishy here; if we have this
> > clash, do we hit the "if (fd == -1 && errno != EEXIST) return -1;" case
> > directly on the first iteration?
>
> I tried reproducing this myself. I was able to hit the error, but only very
> very rarely (I only reproduced it twice while running these loops for an hour
> or two)).
>
> The loop does work as expected, normally - in most cases of collisions, we do
> continue iterating. Only in a very very small fraction of cases, we end up
> with errno set to something else than EEXIST, e.g. EACCES. And overall,
> erroring out on EACCES is the right thing to do, otherwise we'd loop (near)
> infinitely if trying to create a temp file in a directory without write
> permission.
>
> So that clarifies the mystery to me. And the suggested fix of using rand_s()
> sounds good to me, but we should keep a fallback to rand().
>
> I'll post a new patch with that behaviour implemented, and with a slightly
> updated commit message.
>
> // Martin
>
> ___
> Mingw-w64-public mailing list
> Mingw-w64-public@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
>
For some mysterious reason I was unable to reproduce the problem with
toolchains provided by MSYS2.
However using https://github.com/niXman/mingw-builds-binaries or my own
toolchain built with crosstool-ng it took less than a minute for me using
provided loops.
Even GitHub actions reproduced the issue within 5 minutes:
https://github.com/mati865/ehuss_test/actions/runs/7291015249/job/19869073666
Thank you for picking this up though.
___
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] Use rand_s in mkstemp function
On Mon, 4 Mar 2024, Martin Storsjö wrote:
Looking closer at our mkstemp implementation, we have this loop:
/*
Like OpenBSD, mkstemp() will try at least 2 ** 31 combinations before
giving up.
*/
for (i = 0; i >= 0; i++) {
for(j = index; j < len; j++) {
template_name[j] = letters[rand () % 62];
}
fd = _sopen(template_name,
_O_RDWR | _O_CREAT | _O_EXCL | _O_BINARY,
_SH_DENYNO, _S_IREAD | _S_IWRITE);
if (fd != -1) return fd;
if (fd == -1 && errno != EEXIST) return -1;
}
This should retry an absolutely insane number of times, so as long as one
process finds a unique file name and stops iterating, the other parallel
process should also find a unique one soon after, one would expect.
So if this fails, it looks like something is fishy here; if we have this
clash, do we hit the "if (fd == -1 && errno != EEXIST) return -1;" case
directly on the first iteration?
I tried reproducing this myself. I was able to hit the error, but only
very very rarely (I only reproduced it twice while running these loops for
an hour or two)).
The loop does work as expected, normally - in most cases of collisions, we
do continue iterating. Only in a very very small fraction of cases, we end
up with errno set to something else than EEXIST, e.g. EACCES. And overall,
erroring out on EACCES is the right thing to do, otherwise we'd loop
(near) infinitely if trying to create a temp file in a directory without
write permission.
So that clarifies the mystery to me. And the suggested fix of using
rand_s() sounds good to me, but we should keep a fallback to rand().
I'll post a new patch with that behaviour implemented, and with a slightly
updated commit message.
// Martin
___
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] Use rand_s in mkstemp function
On Mon, 4 Mar 2024, Martin Storsjö wrote:
Hi,
On Mon, 4 Mar 2024, Mateusz Mikuła wrote:
rand is not random enough and may lead to clashing temporary directories
with multiple parallel link processes as it was observed on Rust's CI.
It can be reproduced with these commands (run them all in without long
pauses):
for n in {1..15000}; do rm -f lib/libLLVMAVRAsmParser.a && \
ar qc lib/libLLVMAVRAsmParser.a
lib/Target/AVR/AsmParser/CMakeFiles/LLVMAVRAsmParser.dir/AVRAsmParser.cpp.obj
&& \
ranlib.exe lib/libLLVMAVRAsmParser.a; done &
for n in {1..15000}; do rm -f lib/libLLVMSparcCodeGen.a && \
ar qc lib/libLLVMSparcCodeGen.a
lib/Target/Sparc/CMakeFiles/LLVMSparcCodeGen.dir/*.obj && \
ranlib.exe lib/libLLVMSparcCodeGen.a; done
echo "done"
fg
Before the patch it will fail with an error: ranlib.exe: could not create
temporary file whilst writing archive: no more archived files.
Thanks, I've run into this issue occasionally when building LLVM on msys2 as
well, but I've failed to reproduce it when I've tried to look closer at it
(as I've missed the issue that one needs to build two archives at the same
time in order to trigger it).
If the issue is that the randomness clashes, shouldn't that be something
that, as part of the contract of mkstemp, the function should retry until it
finds a non-conflicting combination? But, thinking further, is the issue that
two processes end up trying the same sequence of pseudo random files, which
all then end up clashing, and mkstemp returns an error as it was unable to
find a unique file name? I guess that's plausible. In that case, I guess this
patch is fine (with Liu Hao's suggestion), as a way to reduce the risk of
running into this.
Looking closer at our mkstemp implementation, we have this loop:
/*
Like OpenBSD, mkstemp() will try at least 2 ** 31 combinations before
giving up.
*/
for (i = 0; i >= 0; i++) {
for(j = index; j < len; j++) {
template_name[j] = letters[rand () % 62];
}
fd = _sopen(template_name,
_O_RDWR | _O_CREAT | _O_EXCL | _O_BINARY,
_SH_DENYNO, _S_IREAD | _S_IWRITE);
if (fd != -1) return fd;
if (fd == -1 && errno != EEXIST) return -1;
}
This should retry an absolutely insane number of times, so as long as one
process finds a unique file name and stops iterating, the other parallel
process should also find a unique one soon after, one would expect.
So if this fails, it looks like something is fishy here; if we have this
clash, do we hit the "if (fd == -1 && errno != EEXIST) return -1;" case
directly on the first iteration?
(Separately, it looks like the loop relies on undefined behaviour, signed
wraparound, in order to exit the loop.)
// Martin
___
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] Use rand_s in mkstemp function
Hi,
On Mon, 4 Mar 2024, Mateusz Mikuła wrote:
rand is not random enough and may lead to clashing temporary directories
with multiple parallel link processes as it was observed on Rust's CI.
It can be reproduced with these commands (run them all in without long pauses):
for n in {1..15000}; do rm -f lib/libLLVMAVRAsmParser.a && \
ar qc lib/libLLVMAVRAsmParser.a
lib/Target/AVR/AsmParser/CMakeFiles/LLVMAVRAsmParser.dir/AVRAsmParser.cpp.obj
&& \
ranlib.exe lib/libLLVMAVRAsmParser.a; done &
for n in {1..15000}; do rm -f lib/libLLVMSparcCodeGen.a && \
ar qc lib/libLLVMSparcCodeGen.a lib/Target/Sparc/CMakeFiles/LLVMSparcCodeGen.dir/*.obj
&& \
ranlib.exe lib/libLLVMSparcCodeGen.a; done
echo "done"
fg
Before the patch it will fail with an error: ranlib.exe: could not create
temporary file whilst writing archive: no more archived files.
Thanks, I've run into this issue occasionally when building LLVM on msys2
as well, but I've failed to reproduce it when I've tried to look closer at
it (as I've missed the issue that one needs to build two archives at the
same time in order to trigger it).
If the issue is that the randomness clashes, shouldn't that be something
that, as part of the contract of mkstemp, the function should retry until
it finds a non-conflicting combination? But, thinking further, is the
issue that two processes end up trying the same sequence of pseudo random
files, which all then end up clashing, and mkstemp returns an error as it
was unable to find a unique file name? I guess that's plausible. In that
case, I guess this patch is fine (with Liu Hao's suggestion), as a way to
reduce the risk of running into this.
// Martin
___
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] Use rand_s in mkstemp function
在 2024-03-04 08:04, Mateusz Mikuła 写道: -template_name[j] = letters[rand () % 62]; +if (rand_s ()) return -1; +template_name[j] = letters[r % 62]; Does it make sense that if `rand_s()` fails, use `rand()` as a fallback? We have a custom implementation of `rand_s()` in 'secapi/rand_s.c' which may return `EINVAL` on Windows 2000. -- Best regards, LIU Hao OpenPGP_signature.asc Description: OpenPGP digital signature ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
[Mingw-w64-public] [PATCH] Use rand_s in mkstemp function
rand is not random enough and may lead to clashing temporary directories
with multiple parallel link processes as it was observed on Rust's CI.
It can be reproduced with these commands (run them all in without long pauses):
for n in {1..15000}; do rm -f lib/libLLVMAVRAsmParser.a && \
ar qc lib/libLLVMAVRAsmParser.a
lib/Target/AVR/AsmParser/CMakeFiles/LLVMAVRAsmParser.dir/AVRAsmParser.cpp.obj
&& \
ranlib.exe lib/libLLVMAVRAsmParser.a; done &
for n in {1..15000}; do rm -f lib/libLLVMSparcCodeGen.a && \
ar qc lib/libLLVMSparcCodeGen.a
lib/Target/Sparc/CMakeFiles/LLVMSparcCodeGen.dir/*.obj && \
ranlib.exe lib/libLLVMSparcCodeGen.a; done
echo "done"
fg
Before the patch it will fail with an error: ranlib.exe: could not create
temporary file whilst writing archive: no more archived files.
The changes in this patch were proposed by Josh Stone on Rust's Zulip server
and I was asked to forward it.
Co-Authored-By: Josh Stone
Signed-off-by: Mateusz Mikuła
---
mingw-w64-crt/misc/mkstemp.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/mingw-w64-crt/misc/mkstemp.c b/mingw-w64-crt/misc/mkstemp.c
index 6b327f2fc..1698f49f4 100644
--- a/mingw-w64-crt/misc/mkstemp.c
+++ b/mingw-w64-crt/misc/mkstemp.c
@@ -1,3 +1,4 @@
+#define _CRT_RAND_S
#include
#include
#include
@@ -25,6 +26,7 @@
int __cdecl mkstemp (char *template_name)
{
int i, j, fd, len, index;
+unsigned int r;
/* These are the (62) characters used in temporary filenames. */
static const char letters[] =
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
@@ -45,7 +47,8 @@ int __cdecl mkstemp (char *template_name)
*/
for (i = 0; i >= 0; i++) {
for(j = index; j < len; j++) {
-template_name[j] = letters[rand () % 62];
+if (rand_s ()) return -1;
+template_name[j] = letters[r % 62];
}
fd = _sopen(template_name,
_O_RDWR | _O_CREAT | _O_EXCL | _O_BINARY,
--
2.43.0.windows.1
___
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
