[PATCH] v5 mktemp: make -u option work more like coreutils
Tito
farmatito at tiscali.it
Fri Sep 21 21:56:33 UTC 2012
On Friday 21 September 2012 17:10:54 Ron Yorston wrote:
> BusyBox mktemp uses tempnam to generate filenames with the -u option.
> tempnam works differently from the mechanism used by coreutils: it pays
> too much attention to TMPDIR and it only uses the first five characters
> of the template.
>
> Consider the following commands:
>
> mktemp -u temp.XXXXXX
> mktemp -u -t temp.XXXXXX
> mktemp -u temp123.XXXXXX
> mktemp -u t/temp.XXXXXX
> mktemp -u tmpdir/temp.XXXXXX
> mktemp -u /var/tmp/temp.XXXXXX
>
> coreutils mktemp returns (with TMPDIR unset in the left column and set
> to /tmp in the right):
>
> temp.N5sFF8 temp.szUHKA
> /tmp/temp.7uPmpr /tmp/temp.EnN3Cb
> temp123.pCKP0t temp123.mLLGKy
> t/temp.y1Tr2X t/temp.j91VbW
> tmpdir/temp.9bHHbd tmpdir/temp.WR2iXj
> /var/tmp/temp.H6ypnZ /var/tmp/temp.uOWZM5
>
> Original BusyBox mktemp returns:
>
> temp.AwvZ1U mp/temp.MjmUfi
> /tmp/temp.eUYIwR /tmp/temp.yJX2ox
> temp15ugCZW mp/temp18qPhgB
> t/temABm01N mp/t/tem6acA5F
> tmpdivTbGrF mp/tmpdi6aop9L
> /var/bN6XBU mp//var/6jeTv6
>
> With this patch BusyBox mktemp returns:
>
> temp.r0jnvN temp.Fi7Zgh
> /tmp/temp.2Hi55L /tmp/temp.oQLmgi
> temp123.rCzPdM temp123.tUqsBg
> t/temp.21IcyP t/temp.cgeAmg
> tmpdir/temp.FV5FxO tmpdir/temp.VJW19j
> /var/tmp/temp.E45xYO /var/tmp/temp.sYHyok
>
> Signed-off-by: Ron Yorston <rmy at tigress.co.uk>
>
> ---
> debianutils/mktemp.c | 22 ++++++----------------
> 1 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/debianutils/mktemp.c b/debianutils/mktemp.c
> index dbe4309..1f3bc9b 100644
> --- a/debianutils/mktemp.c
> +++ b/debianutils/mktemp.c
> @@ -84,25 +84,15 @@ int mktemp_main(int argc UNUSED_PARAM, char **argv)
> opts |= OPT_t;
> }
>
> - if (opts & OPT_u) {
> - /* Remove (up to) 6 X's */
> - unsigned len = strlen(chp);
> - int cnt = len > 6 ? 6 : len;
> - while (--cnt >= 0 && chp[--len] == 'X')
> - chp[len] = '\0';
> -
> - chp = tempnam(opts & (OPT_t|OPT_p) ? path : "./", chp);
> - if (!chp)
> - return EXIT_FAILURE;
> - if (!(opts & (OPT_t|OPT_p)))
> - chp += 2;
> - goto ret;
> - }
> -
> if (opts & (OPT_t|OPT_p))
> chp = concat_path_file(path, chp);
>
> - if (opts & OPT_d) {
> + if (opts & OPT_u) {
> + chp = mktemp(chp);
> + if ( chp[0] == '\0' )
> + return EXIT_FAILURE;
> + }
> + else if (opts & OPT_d) {
> if (mkdtemp(chp) == NULL)
> return EXIT_FAILURE;
> } else {
>
Hi,
I agree with you that real mktemp definitely doesn't use tmpnam,
so your patch looks better than my previous one.
Still I would add some minor cleanups and improvements:
1) remove the ret: label as it is now unused
2) improve opt_complementary to make -p imply -t (to be coherent)
3) fix whitespace damaging as your patch could be applied only with patch -l
4) explicitly add to the help text that -u is unsafe
5) don't let mktemp fail silently, print some error messages
6) don't ignore -q
7) don't allow directory separator in template
Due to the several changes in the code review and more testing
from list menbers is needed. Hints and critics are welcome.
Ciao,
Tito
--- debianutils/mktemp.c.original 2012-09-21 22:15:29.000000000 +0200
+++ debianutils/mktemp.c 2012-09-21 23:50:01.000000000 +0200
@@ -38,10 +38,10 @@
//usage: "TEMPLATE must end with XXXXXX (e.g. [/dir/]nameXXXXXX).\n"
//usage: "Without TEMPLATE, -t tmp.XXXXXX is assumed.\n"
//usage: "\n -d Make directory, not file"
-////usage: "\n -q Fail silently on errors" - we ignore this opt
+//usage: "\n -q Fail silently on errors"
//usage: "\n -t Prepend base directory name to TEMPLATE"
//usage: "\n -p DIR Use DIR as a base directory (implies -t)"
-//usage: "\n -u Do not create anything; print a name"
+//usage: "\n -u Do not create anything; print a name (unsafe)"
//usage: "\n"
//usage: "\nBase directory is: -p DIR, else $TMPDIR, else /tmp"
//usage:
@@ -71,8 +71,8 @@
if (!path || path[0] == '\0')
path = "/tmp";
- /* -q is ignored */
- opt_complementary = "?1"; /* 1 argument max */
+ /* -p implies -t */
+ opt_complementary = "?1:pt"; /* 1 argument max */
opts = getopt32(argv, "dqtp:u", &path);
chp = argv[optind];
@@ -83,33 +83,30 @@
chp = xstrdup("tmp.XXXXXX");
opts |= OPT_t;
}
-
- if (opts & OPT_u) {
- /* Remove (up to) 6 X's */
- unsigned len = strlen(chp);
- int cnt = len > 6 ? 6 : len;
- while (--cnt >= 0 && chp[--len] == 'X')
- chp[len] = '\0';
-
- chp = tempnam(opts & (OPT_t|OPT_p) ? path : "./", chp);
- if (!chp)
- return EXIT_FAILURE;
- if (!(opts & (OPT_t|OPT_p)))
- chp += 2;
- goto ret;
+ /* Don't allow directory separator in template */
+ if ((opts & OPT_t) && bb_basename(chp) != chp) {
+ errno = EINVAL;
+ goto error;
}
-
if (opts & (OPT_t|OPT_p))
chp = concat_path_file(path, chp);
- if (opts & OPT_d) {
+ if (opts & OPT_u) {
+ chp = mktemp(chp);
+ if ( chp[0] == '\0' )
+ goto error;
+ } else if (opts & OPT_d) {
if (mkdtemp(chp) == NULL)
- return EXIT_FAILURE;
+ goto error;
} else {
if (mkstemp(chp) < 0)
- return EXIT_FAILURE;
+ goto error;
}
- ret:
puts(chp);
return EXIT_SUCCESS;
+ error:
+ if (opts & OPT_q)
+ return EXIT_FAILURE;
+ /* don't use chp as it gets mangled in case of error */
+ bb_perror_nomsg_and_die();
}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mktemp_05.patch
Type: text/x-patch
Size: 2643 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20120921/db5b3048/attachment.bin>
More information about the busybox
mailing list