Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
On 12/7/18 10:15 AM, David Seikel wrote: >> Sorry for the delay replying, gmail unsubscribed everybody from the >> list again and I had to do the web gui dance. > > I had left my read only gmail account on this list as a comparison to > my account on my own email server. As expected, the gmail one did the > bouncy spam thing, mine didn't. So I'll go and properly remove my > gmail one now, should not be any of my gmail accounts left in your > mailing lists anymore. That's one less to dance for next time. There's a little over 100 gmail accounts subscribed to the list. And the dreamhost web interface can't _not_ sort them alphabetically, so I have to click on every letter one at a time to bring up _that_ page of accounts and unclick the ones with "B" as their disabled reason, then submit to reload the page, then click on the next letter. (And there used to be some starting with digits, but those apparently dropped off...) My internet here in milwaukee is via phone tethering (never got a cable modem for the apartment and I don't connect personal devices to the $DAYJOB network), and since dreamhost's https support is only for the main website and _not_ the lists.landley.net server, I prefer to do the unavoidably plaintext password admin stuff via USB cable rather than wireless access point (so at least _one_ less hop is insecurely encrypted), and I didn't have a working USB cable with me. (The little short one is a "charge but not carry data" cable, which I prefer whem plugging into who knows what USB power to keep my phone charged, but didn't have the other one in my backpack so had to do it when I got home.) Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
> Sorry for the delay replying, gmail unsubscribed everybody from the > list again and I had to do the web gui dance. I had left my read only gmail account on this list as a comparison to my account on my own email server. As expected, the gmail one did the bouncy spam thing, mine didn't. So I'll go and properly remove my gmail one now, should not be any of my gmail accounts left in your mailing lists anymore. That's one less to dance for next time. -- A big old stinking pile of genius that no one wants coz there are too many silver coated monkeys in the world. ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
On 12/6/18 1:11 PM, enh wrote: > On Thu, Dec 6, 2018 at 10:48 AM Rob Landley wrote: >> >> On 12/6/18 10:55 AM, enh wrote: >>> it occurred to me overnight that the passwd code allows '/'. plus, as >>> i've said, it seems to be slightly wrong. so i've just sent a >>> replacement for my second patch (to be applied on top of the first >>> patch) that just does the simplest xgetrandom + convert to valid >>> character loop. >>> >>> can we at least get the first patch in, since mktemp is pretty broken >>> right now (_except_ for -u, which was all the old tests exercised)? >> >> Can you send me just a tests/mktemp.test patch to demonstrate the failures? >> I'll >> try to fix it this evening. (Too many changes at once, I need to know what >> success looks like...) > > you could just apply the two patches... that way you'll have the tests > _and_ they'll pass. (the _real_ question is whether we're still > missing important tests, since we were only testing -u until now. plus > testing "the Xs get replaced with random data" is inherently hard to > test.) > > i've attached "just the tests" to this mail. Sorry for the delay replying, gmail unsubscribed everybody from the list again and I had to do the web gui dance. I just pushed a version that passes all those tests, but there's probably more corner cases to add to mktemp.test. My notes are to check what -p "" and TMPDIR="" do (set but act like not set), look at unifying the passwd and salt and mktemp random char functions, and my "I tested this while developing" cut and paste backscroll is: $ TMPDIR=. mktemp -u blah.XXX blah.Iq2 $ TMPDIR=/tmp mktemp -u murgle blah.XXX mktemp: too many templates $ TMPDIR=/tmp mktemp -p murgle/ blah.XXX mktemp: failed to create file via template ‘murgle/blah.XXX’: No such file or directory $ TMPDIR=/tmp mktemp -up murgle/ blah.XXX murgle/blah.jjs $ TMPDIR=/tmp mktemp -tup murgle/ blah.XXX /tmp/blah.3NR $ mktemp -tup murgle/ blah.XXX murgle/blah.rel $ mktemp -tup "" blah.XXX blah.V6Q $ TMPDIR=/woot mktemp -tup "" blah.XXX /woot/blah.lpm $ TMPDIR=/woot mktemp -up "" blah.XXX /woot/blah.Fh5 But it is $DAYJOB time again... Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
On Thu, Dec 6, 2018 at 10:48 AM Rob Landley wrote: > > On 12/6/18 10:55 AM, enh wrote: > > it occurred to me overnight that the passwd code allows '/'. plus, as > > i've said, it seems to be slightly wrong. so i've just sent a > > replacement for my second patch (to be applied on top of the first > > patch) that just does the simplest xgetrandom + convert to valid > > character loop. > > > > can we at least get the first patch in, since mktemp is pretty broken > > right now (_except_ for -u, which was all the old tests exercised)? > > Can you send me just a tests/mktemp.test patch to demonstrate the failures? > I'll > try to fix it this evening. (Too many changes at once, I need to know what > success looks like...) you could just apply the two patches... that way you'll have the tests _and_ they'll pass. (the _real_ question is whether we're still missing important tests, since we were only testing -u until now. plus testing "the Xs get replaced with random data" is inherently hard to test.) i've attached "just the tests" to this mail. > Thanks, > > Rob From 4eb17f7d48f5a2b408d3b050fa27700925902288 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Thu, 6 Dec 2018 11:06:55 -0800 Subject: [PATCH] mktemp: just the missing tests. --- tests/mktemp.test | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/mktemp.test b/tests/mktemp.test index ee7702dc..0c235469 100755 --- a/tests/mktemp.test +++ b/tests/mktemp.test @@ -6,11 +6,20 @@ # mktemp by default should use tmp.XX as the template, # and $TMPDIR as the directory. -testing "default" "TMPDIR=/t mktemp -u | grep -q '^/t/tmp\...$' && echo yes" "yes\n" "" "" +testing "default" "TMPDIR=. mktemp | grep -q '^./tmp\...$' && echo yes" "yes\n" "" "" + +# Test that -d creates a directory and the default is a file. +testing "dir" "test -d `TMPDIR=. mktemp -d` && echo yes" "yes\n" "" "" +testing "file" "test -f `TMPDIR=. mktemp` && echo yes" "yes\n" "" "" # mktemp with a template should *not* use $TMPDIR. testing "TEMPLATE" "TMPDIR=/t mktemp -u hello. | grep -q '^hello\.$' && echo yes" "yes\n" "" "" +# mktemp with a template that includes a '/' should ignore $TMPDIR +testing "/ TEMPLATE" "TMPDIR=/t mktemp -u /x/hello. | grep -q '^/x/hello\.$' && echo yes" "yes\n" "" "" +# ...and setting DIR is an error. +testing "/ TEMPLATE -p DIR" "TMPDIR=/t mktemp -p DIR -u /x/hello. || echo error" "error\n" "" "" + # mktemp with -t and a template should use $TMPDIR. testing "-t TEMPLATE" "TMPDIR=/t mktemp -u -t hello. | grep -q '^/t/hello\.$' && echo yes" "yes\n" "" "" @@ -28,3 +37,9 @@ testing "-p DIR -t TEMPLATE but no TMPDIR" "TMPDIR= mktemp -u -p DIR -t hello.XX # mktemp -u doesn't need to be able to write to the directory. testing "-u" "mktemp -u -p /proc | grep -q '^/proc/tmp\...$' && echo yes" "yes\n" "" "" + +# mktemp needs at least XX in the template. +testing "bad template" "mktemp -u helloX || echo error" "error\n" "" "" + +# mktemp -q shouldn't print the path. +testing "-q" "mktemp -p /proc -q || echo only-failure" "only-failure\n" "" "" -- 2.20.0.rc1.387.gf8505762e3-goog ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
On 12/6/18 10:55 AM, enh wrote: > it occurred to me overnight that the passwd code allows '/'. plus, as > i've said, it seems to be slightly wrong. so i've just sent a > replacement for my second patch (to be applied on top of the first > patch) that just does the simplest xgetrandom + convert to valid > character loop. > > can we at least get the first patch in, since mktemp is pretty broken > right now (_except_ for -u, which was all the old tests exercised)? Can you send me just a tests/mktemp.test patch to demonstrate the failures? I'll try to fix it this evening. (Too many changes at once, I need to know what success looks like...) Thanks, Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
it occurred to me overnight that the passwd code allows '/'. plus, as i've said, it seems to be slightly wrong. so i've just sent a replacement for my second patch (to be applied on top of the first patch) that just does the simplest xgetrandom + convert to valid character loop. can we at least get the first patch in, since mktemp is pretty broken right now (_except_ for -u, which was all the old tests exercised)? On Wed, Dec 5, 2018 at 5:34 PM enh wrote: > > (sent. you can just run `./toybox mktemp -u > tmp.XXX` to see the effect.) > On Wed, Dec 5, 2018 at 5:27 PM enh wrote: > > > > which seems to have found a bug in the password code? > > > > i'll send you that patch as a follow up to this one, and you can look at > > why if mktemp reuses that code, long enough templates end in a string of > > '.'s... > > > > On Wed, Dec 5, 2018 at 4:08 PM enh wrote: > >> > >> note that you've already got "random ascii" code in password.c --- i have > >> a half-finished patch that switches mktemp over to reusing that instead... > >> > >> On Wed, Dec 5, 2018 at 3:40 PM Rob Landley wrote: > >>> > >>> On 12/5/18 5:03 PM, enh via Toybox wrote: > >>> > Multiple bugs: > >>> > > >>> > * We weren't outputting anything in the case where we actually create a > >>> > file or directory (but all the tests were for the -u case). > >>> > > >>> > * There are more gnarls to the behavior if TEMPLATE contains a '/'. The > >>> > new tests cover these. > >>> > >>> Sigh. (See attached...) > >>> > >>> Lemme try to reconcile them... > >>> > >>> Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
(sent. you can just run `./toybox mktemp -u tmp.XXX` to see the effect.) On Wed, Dec 5, 2018 at 5:27 PM enh wrote: > > which seems to have found a bug in the password code? > > i'll send you that patch as a follow up to this one, and you can look at why > if mktemp reuses that code, long enough templates end in a string of '.'s... > > On Wed, Dec 5, 2018 at 4:08 PM enh wrote: >> >> note that you've already got "random ascii" code in password.c --- i have a >> half-finished patch that switches mktemp over to reusing that instead... >> >> On Wed, Dec 5, 2018 at 3:40 PM Rob Landley wrote: >>> >>> On 12/5/18 5:03 PM, enh via Toybox wrote: >>> > Multiple bugs: >>> > >>> > * We weren't outputting anything in the case where we actually create a >>> > file or directory (but all the tests were for the -u case). >>> > >>> > * There are more gnarls to the behavior if TEMPLATE contains a '/'. The >>> > new tests cover these. >>> >>> Sigh. (See attached...) >>> >>> Lemme try to reconcile them... >>> >>> Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
which seems to have found a bug in the password code? i'll send you that patch as a follow up to this one, and you can look at why if mktemp reuses that code, long enough templates end in a string of '.'s... On Wed, Dec 5, 2018 at 4:08 PM enh wrote: > note that you've already got "random ascii" code in password.c --- i have > a half-finished patch that switches mktemp over to reusing that instead... > > On Wed, Dec 5, 2018 at 3:40 PM Rob Landley wrote: > >> On 12/5/18 5:03 PM, enh via Toybox wrote: >> > Multiple bugs: >> > >> > * We weren't outputting anything in the case where we actually create a >> > file or directory (but all the tests were for the -u case). >> > >> > * There are more gnarls to the behavior if TEMPLATE contains a '/'. The >> > new tests cover these. >> >> Sigh. (See attached...) >> >> Lemme try to reconcile them... >> >> Rob >> > ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
note that you've already got "random ascii" code in password.c --- i have a half-finished patch that switches mktemp over to reusing that instead... On Wed, Dec 5, 2018 at 3:40 PM Rob Landley wrote: > On 12/5/18 5:03 PM, enh via Toybox wrote: > > Multiple bugs: > > > > * We weren't outputting anything in the case where we actually create a > > file or directory (but all the tests were for the -u case). > > > > * There are more gnarls to the behavior if TEMPLATE contains a '/'. The > > new tests cover these. > > Sigh. (See attached...) > > Lemme try to reconcile them... > > Rob > ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] mktemp: fix the tests and the logic.
On 12/5/18 5:03 PM, enh via Toybox wrote: > Multiple bugs: > > * We weren't outputting anything in the case where we actually create a > file or directory (but all the tests were for the -u case). > > * There are more gnarls to the behavior if TEMPLATE contains a '/'. The > new tests cover these. Sigh. (See attached...) Lemme try to reconcile them... Rob diff --git a/lib/portability.c b/lib/portability.c index a80ca56..5c754d7 100644 --- a/lib/portability.c +++ b/lib/portability.c @@ -30,18 +30,20 @@ pid_t xfork(void) } #endif -void xgetrandom(void *buf, unsigned buflen, unsigned flags) +int xgetrandom(void *buf, unsigned buflen, unsigned flags) { int fd; #if CFG_TOYBOX_GETRANDOM - if (buflen == getrandom(buf, buflen, flags)) return; - if (!CFG_TOYBOX_ON_ANDROID || errno!=ENOSYS) perror_exit("getrandom"); + if (buflen == getrandom(buf, buflen, flags&~WARN_ONLY)) return 1; + if (errno!=ENOSYS && !(flags&WARN_ONLY)) perror_exit("getrandom"); #endif - - fd = xopen(flags ? "/dev/random" : "/dev/urandom", O_RDONLY); + fd = xopen(flags ? "/dev/random" : "/dev/urandom",O_RDONLY|(flags&WARN_ONLY)); + if (fd == -1) return 0; xreadall(fd, buf, buflen); close(fd); + + return 1; } #if defined(__APPLE__) diff --git a/lib/portability.h b/lib/portability.h index 21d0b8a..60d4049 100644 --- a/lib/portability.h +++ b/lib/portability.h @@ -266,7 +266,7 @@ extern CODE prioritynames[], facilitynames[]; #if CFG_TOYBOX_GETRANDOM #include #endif -void xgetrandom(void *buf, unsigned len, unsigned flags); +int xgetrandom(void *buf, unsigned len, unsigned flags); // Android's bionic libc doesn't have confstr. #ifdef __BIONIC__ diff --git a/toys/lsb/mktemp.c b/toys/lsb/mktemp.c index 112f84c..6d159d6 100644 --- a/toys/lsb/mktemp.c +++ b/toys/lsb/mktemp.c @@ -34,12 +34,9 @@ GLOBALS( void mktemp_main(void) { char *template = *toys.optargs; - int use_dir = (toys.optflags & (FLAG_p|FLAG_t)); + int use_dir = (toys.optflags & (FLAG_p|FLAG_t)), len; - if (!template) { -template = "tmp.XX"; -use_dir = 1; - } + if (!template) template = "tmp.XX"; // Normally, the precedence is DIR (if set), $TMPDIR (if set), /tmp. // With -t it's $TMPDIR, DIR, /tmp. @@ -48,17 +45,38 @@ void mktemp_main(void) if (toys.optflags & FLAG_t) { if (tmpdir && *tmpdir) TT.p = tmpdir; -} else { - if (!TT.p || !*TT.p) TT.p = tmpdir; -} +} else if (!TT.p || !*TT.p) TT.p = tmpdir; if (!TT.p || !*TT.p) TT.p = "/tmp"; } // TODO: coreutils cleans paths, so -p /t/// would result in /t/xxx... template = use_dir ? xmprintf("%s/%s", TT.p, template) : xstrdup(template); + len = strlen(template); + if (len<3 || strcmp(template+len-3, "XXX")) perror_exit("need XXX"); + // In theory you just xputs(mktemp(template)) for -u, in practice there's + // link-time deprecation warnings if you do that. So we fake up our own: if (toys.optflags & FLAG_u) { -xputs(mktemp(template)); +long long rr; +char *s = template+len; + +// Fall back to random-ish if xgetrandom fails. +if (!xgetrandom(&rr, sizeof(rr), WARN_ONLY)) { + struct timespec ts; + + clock_gettime(CLOCK_REALTIME, &ts); + rr = ts.tv_nsec*65537+(long)template+getpid()+(long)&template; +} +// Replace X with 64 chars from posix portable character set (all but "_"). +while (--s>template) { + if (*s != 'X') break; + *s = '-'+(rr&63); + if (*s>'.') ++*s; + if (*s>'9') (*s) += 7; + if (*s>'Z') (*s) += 6; + rr>>=6; +} +xputs(template); } else if (toys.optflags & FLAG_d ? !mkdtemp(template) : mkstemp(template) == -1) { if (toys.optflags & FLAG_q) toys.exitval = 1; else perror_exit("Failed to create %s %s/%s", ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net