Re: [PATCH v2 1/2] mktemp: avoid removed mktemp()
On Mon, Apr 14, 2014 at 02:29:21PM +0200, Denys Vlasenko wrote:
> On Mon, Apr 14, 2014 at 12:20 PM, Bernhard Reutner-Fischer
> wrote:
> >> This is a change in behavior - now we would
> >> actually create, and then immediately delete the directory
> >> if run as "mktemp -du".
> >>
> >> I have mixed feelings about that.
> >> Do you think it's ok?
> >
> > I think it's ok, yes. The race-window is about the same as
> > before, the creat()/unlink() do make it a bit slower but that
> > is IMO ok.
>
> There was no race at all before for "mktemp -n" case:
>
> if (opts & OPT_u) {
> chp = mktemp(chp);
> if (chp[0] == '\0')
> goto error;
> } else ...
> puts(chp);
> return EXIT_SUCCESS;
>
> No race here.
The 'race' is in the caller, but it's not always a race. If the caller
properly uses O_EXCL or similar and retries on fail, there's no race.
Note that there are legitimate uses of mktemp -u, such as along with
mkfifo or for choosing a name for a socket or shared memory object or
similar.
> > An alternative would be to export a uClibc-specific
> > __gen_tempname() function
>
> How about exporting a function named mktemp()?
> ;)
> POSIX removing it from its text doesn't mean that
> a conforming libc must *not* provide it.
Agreed. I don't see any value in removing these functions. If support
for libcs that lack them is desired, a proper implementation should be
provided in busybox rather than poor hacks that involve creating and
deleting the temp file, and which could fail due to odd permission
setups, etc.
Rich
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] mktemp: avoid removed mktemp()
On Mon, Apr 14, 2014 at 12:20 PM, Bernhard Reutner-Fischer
wrote:
>> This is a change in behavior - now we would
>> actually create, and then immediately delete the directory
>> if run as "mktemp -du".
>>
>> I have mixed feelings about that.
>> Do you think it's ok?
>
> I think it's ok, yes. The race-window is about the same as
> before, the creat()/unlink() do make it a bit slower but that
> is IMO ok.
There was no race at all before for "mktemp -n" case:
if (opts & OPT_u) {
chp = mktemp(chp);
if (chp[0] == '\0')
goto error;
} else ...
puts(chp);
return EXIT_SUCCESS;
No race here.
Your patch would add one.
> An alternative would be to export a uClibc-specific
> __gen_tempname() function to avoid all those duplicate implementations
> (in busybox, coreutils etc, etc) but i don't really think it's worth the
> effort
> given that only mktemp(1) has something like it's -u and -u
> obviously should not be used in the first place (due to races).
We need to support compatibility anyway. So...
> An alternative would be to export a uClibc-specific
> __gen_tempname() function
How about exporting a function named mktemp()?
;)
POSIX removing it from its text doesn't mean that
a conforming libc must *not* provide it.
--
vda
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] mktemp: avoid removed mktemp()
On 13 April 2014 16:41, Denys Vlasenko wrote:
> On Tue, Apr 8, 2014 at 3:11 PM, Bernhard Reutner-Fischer
> wrote:
>> + if (opts & OPT_d) {
>> if (mkdtemp(chp) == NULL)
>> goto error;
>> + if (opts & OPT_u)
>> + rmdir(chp);
>
> This is a change in behavior - now we would
> actually create, and then immediately delete the directory
> if run as "mktemp -du".
>
> I have mixed feelings about that.
> Do you think it's ok?
I think it's ok, yes. The race-window is about the same as
before, the creat()/unlink() do make it a bit slower but that
is IMO ok.
An alternative would be to export a uClibc-specific
__gen_tempname() function to avoid all those duplicate implementations
(in busybox, coreutils etc, etc) but i don't really think it's worth the effort
given that only mktemp(1) has something like it's -u and -u
obviously should not be used in the first place (due to races).
I fear that the (IMO) "correct" fix, i.e. removing -u, potentially breaks quite
some scripts, so unlink/rmdir seems to be the least intrusive thing to
do right now.
Makes sense?
thanks,
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2 1/2] mktemp: avoid removed mktemp()
On Tue, Apr 8, 2014 at 3:11 PM, Bernhard Reutner-Fischer
wrote:
> + if (opts & OPT_d) {
> if (mkdtemp(chp) == NULL)
> goto error;
> + if (opts & OPT_u)
> + rmdir(chp);
This is a change in behavior - now we would
actually create, and then immediately delete the directory
if run as "mktemp -du".
I have mixed feelings about that.
Do you think it's ok?
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2 1/2] mktemp: avoid removed mktemp()
mktemp() was removed from POSIX.1-2008
function old new delta
mktemp_main 214 223 +9
--
(add/remove: 0/0 grow/shrink: 1/0 up/down: 9/0) Total: 9 bytes
Signed-off-by: Bernhard Reutner-Fischer
---
debianutils/mktemp.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/debianutils/mktemp.c b/debianutils/mktemp.c
index 983d7a2..39d4345 100644
--- a/debianutils/mktemp.c
+++ b/debianutils/mktemp.c
@@ -92,16 +92,16 @@ int mktemp_main(int argc UNUSED_PARAM, char **argv)
if (opts & (OPT_t|OPT_p))
chp = concat_path_file(path, chp);
- if (opts & OPT_u) {
- chp = mktemp(chp);
- if (chp[0] == '\0')
- goto error;
- } else if (opts & OPT_d) {
+ if (opts & OPT_d) {
if (mkdtemp(chp) == NULL)
goto error;
+ if (opts & OPT_u)
+ rmdir(chp);
} else {
if (mkstemp(chp) < 0)
goto error;
+ if (opts & OPT_u)
+ unlink(chp);
}
puts(chp);
return EXIT_SUCCESS;
--
1.9.1
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
