30 Jan
2024
30 Jan
'24
10:44 a.m.
Hi,
I stumbled upon the following code in tempname.c:
switch (kind) {
case __GT_NOCREATE:
{
struct stat st;
if (stat (tmpl, &st) < 0) {
if (errno == ENOENT) {
fd = 0;
goto restore_and_ret;
} else
/* Give up now. */
return -1;
} else
fd = 0;
}
case __GT_FILE:
fd = open (tmpl, O_RDWR | O_CREAT | O_EXCL | flags,
mode);
break;
I am baffled by the 'else' branch of the check of 'stat (tmpl, &st) <
0'.
If I understand the whole thing correctly, this is an error case,
where
a file with the temporary name already exists. But instead, the whole
execution falls through to the __GT_FILE branch, and actually opens the
file.
If this happens, this file never seems to be closed again (see for
example
the usage in tmpnam_r.c:30:
char * tmpnam_r (char *s)
{
if (s == NULL)
return NULL;
if (__path_search (s, L_tmpnam, NULL, NULL, 0))
return NULL;
if (__gen_tempname (s, __GT_NOCREATE, 0, 0, 0))
return NULL;
return s;
}
I would suppose that instead of falling to the next switch-case, we
should return with a non-zero value (maybe '1' to distinguish it from
the general error case?).
Am I missing something basic?
Cheers,
Sven
--
Sven Linker
Tel.: +49 351 41883243
Kernkonzept GmbH at Dresden, Germany,
HRB 31129, CEO Dr.-Ing. Michael Hohmuth
31 Jan
31 Jan
10:08 a.m.
Hi Sven,
do you have a small C test case which might show the failure?
It would be a nice addition to uClibc-ng testsuite if it show the
failure.
best regards
Waldemar
Sven Linker wrote,
Hi,
I stumbled upon the following code in tempname.c:
switch (kind) {
case __GT_NOCREATE:
{
struct stat st;
if (stat (tmpl, &st) < 0) {
if (errno == ENOENT) {
fd = 0;
goto restore_and_ret;
} else
/* Give up now. */
return -1;
} else
fd = 0;
}
case __GT_FILE:
fd = open (tmpl, O_RDWR | O_CREAT | O_EXCL | flags,
mode);
break;
I am baffled by the 'else' branch of the check of 'stat (tmpl, &st) <
0'.
If I understand the whole thing correctly, this is an error case,
where
a file with the temporary name already exists. But instead, the whole
execution falls through to the __GT_FILE branch, and actually opens the
file.
If this happens, this file never seems to be closed again (see for
example
the usage in tmpnam_r.c:30:
char * tmpnam_r (char *s)
{
if (s == NULL)
return NULL;
if (__path_search (s, L_tmpnam, NULL, NULL, 0))
return NULL;
if (__gen_tempname (s, __GT_NOCREATE, 0, 0, 0))
return NULL;
return s;
}
I would suppose that instead of falling to the next switch-case, we
should return with a non-zero value (maybe '1' to distinguish it from
the general error case?).
Am I missing something basic?
Cheers,
Sven
--
Sven Linker
Tel.: +49 351 41883243
Kernkonzept GmbH at Dresden, Germany,
HRB 31129, CEO Dr.-Ing. Michael Hohmuth
_______________________________________________
devel mailing list -- devel(a)uclibc-ng.org
To unsubscribe send an email to devel-leave(a)uclibc-ng.org
6 Feb
6 Feb
2:13 p.m.
Hi,
On 31.01.24 11:08, Waldemar Brodkorb wrote:
do you have a small C test case which might show the
failure?
I unfortunately currently don't have a proper setup to test this, but I
would expect that this simple loop would deplete all available file
descriptors so that at some point, one can't call "open" anymore:
#include <stdlib.h>
do {
puts("Still fds available\n");
} while(!strcmp(mktemp("test"),""))
if(open("somefile.txt") < 0) {
puts("fds are depleted. This should not have happened.\n");
}
Best regards,
Marius
--
Marius Melzer, marius.melzer(a)kernkonzept.com
Kernkonzept GmbH, Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth
7 Feb
7 Feb
9:37 a.m.
Hi,
I'm not sure if this really tests the branch where the issue is, but I
am also not able to come up with a simple test myself (since it
basically relies on predicting the random instantiation of the
template).
But this patch should avoid opening the file, if it already exists.
Cheers,
Sven
On Tue, 2024-02-06 at 15:13 +0100, Marius Melzer wrote:
Hi,
On 31.01.24 11:08, Waldemar Brodkorb wrote:
--
Sven Linker
Tel.: +49 351 41883243
Kernkonzept GmbH at Dresden, Germany,
HRB 31129, CEO Dr.-Ing. Michael Hohmuth
do you have a small C test case which might show
the failure?
I unfortunately currently don't have a proper setup to test this, but
I
would expect that this simple loop would deplete all available file
descriptors so that at some point, one can't call "open" anymore:
#include <stdlib.h>
do {
puts("Still fds available\n");
} while(!strcmp(mktemp("test"),""))
if(open("somefile.txt") < 0) {
puts("fds are depleted. This should not have happened.\n");
}
Best regards,
Marius
11 Feb
11 Feb
6:04 a.m.
Hi Sven,
okay. Can you resent the patch with git format-patch -s origin?
Please add a commit message.
Thanks a lot in advance.
Waldemar
Sven Linker wrote,
Hi,
I'm not sure if this really tests the branch where the issue is, but I
am also not able to come up with a simple test myself (since it
basically relies on predicting the random instantiation of the
template).
But this patch should avoid opening the file, if it already exists.
Cheers,
Sven
On Tue, 2024-02-06 at 15:13 +0100, Marius Melzer wrote:
Hi,
On 31.01.24 11:08, Waldemar Brodkorb wrote:
--
Sven Linker
Tel.: +49 351 41883243
Kernkonzept GmbH at Dresden, Germany,
HRB 31129, CEO Dr.-Ing. Michael Hohmuth
do you have a small C test case which might show
the failure?
I unfortunately currently don't have a proper setup to test this, but
I
would expect that this simple loop would deplete all available file
descriptors so that at some point, one can't call "open" anymore:
#include <stdlib.h>
do {
puts("Still fds available\n");
} while(!strcmp(mktemp("test"),""))
if(open("somefile.txt") < 0) {
puts("fds are depleted. This should not have happened.\n");
}
Best regards,
Marius
diff --git a/libc/misc/internals/tempname.c
b/libc/misc/internals/tempname.c
index d3a8ccbd8..f9b714a68 100644
--- a/libc/misc/internals/tempname.c
+++ b/libc/misc/internals/tempname.c
@@ -218,7 +218,8 @@ int attribute_hidden __gen_tempname (char *tmpl, int kind, int
flags,
/* Give up now. */
return -1;
} else
- fd = 0;
+ /* File already exists, so return with non-zero value */
+ return -1;
}
case __GT_FILE:
fd = open (tmpl, O_RDWR | O_CREAT | O_EXCL | flags, mode);
_______________________________________________
devel mailing list -- devel(a)uclibc-ng.org
To unsubscribe send an email to devel-leave(a)uclibc-ng.org
13 Feb
13 Feb
7:54 a.m.
Hi Waldemar,
sure, please find the patch attached.
Cheers,
Sven
On Sun, 2024-02-11 at 07:04 +0100, Waldemar Brodkorb wrote:
Hi Sven,
okay. Can you resent the patch with git format-patch -s origin?
Please add a commit message.
Thanks a lot in advance.
Waldemar
Sven Linker wrote,
--
Sven Linker
Tel.: +49 351 41883243
Kernkonzept GmbH at Dresden, Germany,
HRB 31129, CEO Dr.-Ing. Michael Hohmuth
Hi,
I'm not sure if this really tests the branch where the issue is,
but I
am also not able to come up with a simple test myself (since it
basically relies on predicting the random instantiation of the
template).
But this patch should avoid opening the file, if it already exists.
Cheers,
Sven
On Tue, 2024-02-06 at 15:13 +0100, Marius Melzer wrote:
Hi,
On 31.01.24 11:08, Waldemar Brodkorb wrote:
--
Sven Linker
Tel.: +49 351 41883243
Kernkonzept GmbH at Dresden, Germany,
HRB 31129, CEO Dr.-Ing. Michael Hohmuth
do you have a small C test case which might show
the failure?
I unfortunately currently don't have a proper setup to test this,
but
I
would expect that this simple loop would deplete all available
file
descriptors so that at some point, one can't call "open" anymore:
#include <stdlib.h>
do {
puts("Still fds available\n");
} while(!strcmp(mktemp("test"),""))
if(open("somefile.txt") < 0) {
puts("fds are depleted. This should not have happened.\n");
}
Best regards,
Marius
diff --git a/libc/misc/internals/tempname.c
b/libc/misc/internals/tempname.c
index d3a8ccbd8..f9b714a68 100644
--- a/libc/misc/internals/tempname.c
+++ b/libc/misc/internals/tempname.c
@@ -218,7 +218,8 @@ int attribute_hidden __gen_tempname (char
*tmpl, int kind, int flags,
/* Give up now. */
return -1;
} else
- fd = 0;
+ /* File already exists, so return with
non-zero value */
+ return -1;
}
case __GT_FILE:
fd = open (tmpl, O_RDWR | O_CREAT | O_EXCL |
flags, mode);
_______________________________________________
devel mailing list -- devel(a)uclibc-ng.org
To unsubscribe send an email to devel-leave(a)uclibc-ng.org
18 Feb
18 Feb
6:24 a.m.
Hi Sven,
thanks, applied and pushed.
best regards
Waldemar
Sven Linker wrote,
Hi Waldemar,
sure, please find the patch attached.
Cheers,
Sven
On Sun, 2024-02-11 at 07:04 +0100, Waldemar Brodkorb wrote:
Hi Sven,
okay. Can you resent the patch with git format-patch -s origin?
Please add a commit message.
Thanks a lot in advance.
Waldemar
Sven Linker wrote,
--
Sven Linker
Tel.: +49 351 41883243
Kernkonzept GmbH at Dresden, Germany,
HRB 31129, CEO Dr.-Ing. Michael Hohmuth
Hi,
I'm not sure if this really tests the branch where the issue is,
but I
am also not able to come up with a simple test myself (since it
basically relies on predicting the random instantiation of the
template).
But this patch should avoid opening the file, if it already exists.
Cheers,
Sven
On Tue, 2024-02-06 at 15:13 +0100, Marius Melzer wrote:
Hi,
On 31.01.24 11:08, Waldemar Brodkorb wrote:
> do you have a small C test case which might show the failure?
I unfortunately currently don't have a proper setup to test this,
but
I
would expect that this simple loop would deplete all available
file
descriptors so that at some point, one can't call "open" anymore:
#include <stdlib.h>
do {
puts("Still fds available\n");
} while(!strcmp(mktemp("test"),""))
if(open("somefile.txt") < 0) {
puts("fds are depleted. This should not have happened.\n");
}
Best regards,
Marius
--
Sven Linker
Tel.: +49 351 41883243
Kernkonzept GmbH at Dresden, Germany,
HRB 31129, CEO Dr.-Ing. Michael Hohmuth
diff --git a/libc/misc/internals/tempname.c
b/libc/misc/internals/tempname.c
index d3a8ccbd8..f9b714a68 100644
--- a/libc/misc/internals/tempname.c
+++ b/libc/misc/internals/tempname.c
@@ -218,7 +218,8 @@ int attribute_hidden __gen_tempname (char
*tmpl, int kind, int flags,
/* Give up now. */
return -1;
} else
- fd = 0;
+ /* File already exists, so return with
non-zero value */
+ return -1;
}
case __GT_FILE:
fd = open (tmpl, O_RDWR | O_CREAT | O_EXCL |
flags, mode);
_______________________________________________
devel mailing list -- devel(a)uclibc-ng.org
To unsubscribe send an email to devel-leave(a)uclibc-ng.org
From fb56d9874751c51f493a3ce63080c8c320ef614a Mon Sep
17 00:00:00 2001
From: Sven Linker <sven.linker(a)kernkonzept.com>
Date: Wed, 7 Feb 2024 10:27:58 +0100
Subject: [PATCH] Avoid fall-through if file matching temporary name exists
During checking whether a temporary name can be created, it may happen
that a file with this name already exists. Avoid falling through to
opening the file name in this case, and return with an error instead.
Signed-off-by: Sven Linker <sven.linker(a)kernkonzept.com>
---
libc/misc/internals/tempname.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libc/misc/internals/tempname.c b/libc/misc/internals/tempname.c
index d3a8ccbd8..f9b714a68 100644
--- a/libc/misc/internals/tempname.c
+++ b/libc/misc/internals/tempname.c
@@ -218,7 +218,8 @@ int attribute_hidden __gen_tempname (char *tmpl, int kind, int
flags,
/* Give up now. */
return -1;
} else
- fd = 0;
+ /* File already exists, so return with non-zero value */
+ return -1;
}
case __GT_FILE:
fd = open (tmpl, O_RDWR | O_CREAT | O_EXCL | flags, mode);
--
2.43.0
_______________________________________________
devel mailing list -- devel(a)uclibc-ng.org
To unsubscribe send an email to devel-leave(a)uclibc-ng.org
312
days inactive
331
days old
6 comments
4 participants
participants (4)
-
Marius Melzer -
Sven Linker -
Waldemar Brodkorb -
Waldemar Brodkorb