mkdir builtin and $'\0'

mkdir builtin and $'\0'

[Stephane Chazelas]

$ zsh --version
zsh 5.0.8 (x86_64-pc-linux-gnu)
$ strace -e mkdir zsh -c "zmodload zsh/files; mkdir $'a\0b'"
mkdir("a\203 b", 0755)                  = -1 EEXIST (File exists)
zsh:mkdir:1: cannot make directory `a^@b': file exists
+++ exited with 1 +++

See how that $'\0' became "\203 " except in the error message.

Not a big problem in that nothing good will ever come out of
mkdir $'\0' anyway since one can't pass a NUL character to the
mkdir() system call.

A bit worse:

$ strace -e chdir zsh -c "cd $'a\0b'; print -r -- \$PWD; pwd"
chdir("/export/home/stephane/a")        = -1 ENOENT (No such file or directory)
chdir("a\203 b")                        = 0
chdir("..")                             = 0
chdir("..")                             = 0
chdir("..")                             = 0
chdir("/home/stephane/a\203 b")         = 0
/export/home/stephane/a
/home/stephane/a� b
+++ exited with 0 +++

I suppose the chdir("..")s come after the realisation that $PWD
probably doesn't represent the current directory anymore (since
chdir("$PWD/dir") doesn't work while chdir("dir") does).

"mv" at least doesn't have the problem. A note though:

$ strace -e rename zsh -c "zmodload zsh/files; mv $'a\0b' x"
rename("a", "x/a")                      = -1 ENOENT (No such file or directory)
zsh:mv:1: a^@b: no such file or directory
+++ exited with 1 +++

The error message mentions a^@b not existing while it did
attempt to rename "a" instead.

Maybe those builtins should fail with an error if one attempts
to call them with an argument containing NUL.

-- 
Stephane

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Tue, 18 Aug 2015 10:19:04 +0100
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> $ zsh --version
> zsh 5.0.8 (x86_64-pc-linux-gnu)
> $ strace -e mkdir zsh -c "zmodload zsh/files; mkdir $'a\0b'"
> mkdir("a\203 b", 0755)                  = -1 EEXIST (File exists)
> zsh:mkdir:1: cannot make directory `a^@b': file exists
> +++ exited with 1 +++
> 
> See how that $'\0' became "\203 " except in the error message.

Looks like a long-standing mistake --- the unmetafied form of the
variable wasn't propagated everywhere.  A quick scan elsewhere suggests
this is just a one-off (I haven't confirmed in detail).

diff --git a/Src/Modules/files.c b/Src/Modules/files.c
index f86b9c1..dbcff63 100644
--- a/Src/Modules/files.c
+++ b/Src/Modules/files.c
@@ -131,7 +131,7 @@ domkdir(char *nam, char *path, mode_t mode, int p)
 	    return 0;
     }
     oumask = umask(0);
-    err = mkdir(path, mode) ? errno : 0;
+    err = mkdir(rpath, mode) ? errno : 0;
     umask(oumask);
     if(!err)
 	return 0;

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Tue, 18 Aug 2015 10:19:04 +0100
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> Maybe those builtins should fail with an error if one attempts
> to call them with an argument containing NUL.

We'd probably want to do this generally: create an unmeta_filename()
function that fails hard with an embedded NULL and make sure we handle
the errpr in calling code.

pws

Re: mkdir builtin and $'\0'

[Stephane Chazelas]

2015-08-18 10:49:11 +0100, Peter Stephenson:
[...]
> Looks like a long-standing mistake --- the unmetafied form of the
> variable wasn't propagated everywhere.  A quick scan elsewhere suggests
> this is just a one-off (I haven't confirmed in detail).
> diff --git a/Src/Modules/files.c b/Src/Modules/files.c
[...]

Thanks, but see also the rest of my email about cd/chdir and the
note about inconsistent error message.

More generally, it may be a good idea to review all the cases
where $'\0' would not be acceptable.

And also those where $'\0' would be acceptable. Found another
one:

$ strftime $'%Y\0%m\0%d' 0 | sed -n l
1970\203 01\203 01$

-- 
Stephane

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Tue, 18 Aug 2015 12:11:34 +0100
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> $ strftime $'%Y\0%m\0%d' 0 | sed -n l
> 1970\203 01\203 01$

This should fix that one.

pws

diff --git a/Src/Modules/datetime.c b/Src/Modules/datetime.c
index d941667..f5a47e3 100644
--- a/Src/Modules/datetime.c
+++ b/Src/Modules/datetime.c
@@ -140,7 +140,8 @@ output_strftime(char *nam, char **argv, Options ops, UNUSED(int func))
     if (scalar) {
 	setsparam(scalar, metafy(buffer, -1, META_DUP));
     } else {
-	printf("%s\n", buffer);
+	zputs(buffer, stdout);
+	putchar('\n');
     }
     zfree(buffer, bufsize);
 

Re: mkdir builtin and $'\0'

[Stephane Chazelas]

2015-08-18 12:27:55 +0100, Peter Stephenson:
> On Tue, 18 Aug 2015 12:11:34 +0100
> Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> > $ strftime $'%Y\0%m\0%d' 0 | sed -n l
> > 1970\203 01\203 01$
> 
> This should fix that one.
[...]

Thanks, though it seems to have broken some internationalisation
support. I was trying to see what would happen for month names
that include that \203 byte (like October in ar_AE.UTF-8), but
it seems to also break with some other byte values:

Without the fix:

~$ LC_ALL=ar_AE.UTF-8 date -d @0 +%b | hd
00000000  d9 8a d9 86 d8 a7 0a                              |.......|
00000007
~$ LC_ALL=ar_AE.UTF-8 strftime %b 0  | hd
00000000  d9 8a d9 86 d8 a7 0a                              |.......|
00000007

With the git head:

~$ LC_ALL=ar_AE.UTF-8 strftime %b 0  | hd
00000000  d9 d9 d8 a7 0a                                    |.....|
00000005

I suppose those 8a 86 are special as well.

For \203 0x83:

~$ LC_ALL=ar_AE.UTF-8 date -d @1443697200 +%b | hd
00000000  d8 a3 d9 83 d8 aa 0a                              |.......|
00000007
~$ LC_ALL=ar_AE.UTF-8 strftime %b 1443697200 | hd
00000000  d8 a3 d9 f8 aa 0a                                 |......|
00000006

-- 
Stephane

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Tue, 18 Aug 2015 13:55:19 +0100
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> 2015-08-18 12:27:55 +0100, Peter Stephenson:
> > On Tue, 18 Aug 2015 12:11:34 +0100
> > Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> > > $ strftime $'%Y\0%m\0%d' 0 | sed -n l
> > > 1970\203 01\203 01$
> > 
> > This should fix that one.
> [...]
> 
> Thanks, though it seems to have broken some internationalisation
> support.

That was Ismail's problem, too... in fact, if I'd looked above, I'd have
seen that buffer isn't actually metafied at that point, duh.  So I'll
need to look further at what's causing you're original probalem ---
looks like something metafied has crept in.  I'll back off the original
change.

pws

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Tue, 18 Aug 2015 14:24:58 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Tue, 18 Aug 2015 13:55:19 +0100
> Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> > 2015-08-18 12:27:55 +0100, Peter Stephenson:
> > > On Tue, 18 Aug 2015 12:11:34 +0100
> > > Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> > > > $ strftime $'%Y\0%m\0%d' 0 | sed -n l
> > > > 1970\203 01\203 01$
> > > 
> > > This should fix that one.
> > [...]
> > 
> > Thanks, though it seems to have broken some internationalisation
> > support.
> 
> That was Ismail's problem, too... in fact, if I'd looked above, I'd have
> seen that buffer isn't actually metafied at that point, duh.  So I'll
> need to look further at what's causing you're original probalem ---
> looks like something metafied has crept in.  I'll back off the original
> change.

OK, so the answer at the moment is that ztrftime() assumes everything is
unmetafied.  That's inevitable with the output coming out from
strftime(), so what we pass in needs to be consistent.  However,
actually in every case fmt is passed in still metafied.  So we should
unmetafy it and handle appropriately internally.

For example:

% print -P $'%D{%Y\0%m\0%d}' 0 | sed -n l
2015\203 08\203 18 0$

The caller then needs to metafy where appropriate, so we need to use the
length returned from ztrftime().

This seems to work for strftime and primt -P %D and the tests pass.

One unconnected drive-by unmeta if sched output --- the ztrftime there
was OK because the input is a fixed string and the output is going
straight to printf --- although that means it doesn't handled embedded
NULLs which probably isn't a big issue in this case.

pws

diff --git a/Src/Builtins/sched.c b/Src/Builtins/sched.c
index bcf7661..5d5dac6 100644
--- a/Src/Builtins/sched.c
+++ b/Src/Builtins/sched.c
@@ -220,7 +220,8 @@ bin_sched(char *nam, char **argv, UNUSED(Options ops), UNUSED(int func))
 		endstr = "-- ";
 	    else
 		endstr = "";
-	    printf("%3d %s %s%s%s\n", sn, tbuf, flagstr, endstr, sch->cmd);
+	    printf("%3d %s %s%s%s\n", sn, tbuf, flagstr, endstr,
+		   unmeta(sch->cmd));
 	}
 	return 0;
     } else if (!argptr[1]) {
diff --git a/Src/Modules/datetime.c b/Src/Modules/datetime.c
index d941667..86c61cf 100644
--- a/Src/Modules/datetime.c
+++ b/Src/Modules/datetime.c
@@ -98,7 +98,7 @@ reverse_strftime(char *nam, char **argv, char *scalar, int quiet)
 static int
 output_strftime(char *nam, char **argv, Options ops, UNUSED(int func))
 {
-    int bufsize, x;
+    int bufsize, x, len;
     char *endptr = NULL, *scalar = NULL, *buffer;
     time_t secs;
     struct tm *t;
@@ -131,16 +131,19 @@ output_strftime(char *nam, char **argv, Options ops, UNUSED(int func))
     bufsize = strlen(argv[0]) * 8;
     buffer = zalloc(bufsize);
 
+    len = 0;
     for (x=0; x < 4; x++) {
-        if (ztrftime(buffer, bufsize, argv[0], t, 0L) >= 0)
+        if ((len = ztrftime(buffer, bufsize, argv[0], t, 0L)) >= 0)
 	    break;
 	buffer = zrealloc(buffer, bufsize *= 2);
     }
+    DPUTS(len < 0, "bad output from ztrftime");
 
     if (scalar) {
-	setsparam(scalar, metafy(buffer, -1, META_DUP));
+	setsparam(scalar, metafy(buffer, len, META_DUP));
     } else {
-	printf("%s\n", buffer);
+	fwrite(buffer, 1, len, stdout);
+	putchar('\n');
     }
     zfree(buffer, bufsize);
 
diff --git a/Src/Modules/stat.c b/Src/Modules/stat.c
index 6fc5389..3961771 100644
--- a/Src/Modules/stat.c
+++ b/Src/Modules/stat.c
@@ -197,8 +197,11 @@ stattimeprint(time_t tim, char *outbuf, int flags)
     }
     if (flags & STF_STRING) {
 	char *oend = outbuf + strlen(outbuf);
-	ztrftime(oend, 40, timefmt, (flags & STF_GMT) ? gmtime(&tim) :
-		 localtime(&tim), 0L);
+	/* Where the heck does "40" come from? */
+	int len = ztrftime(oend, 40, timefmt, (flags & STF_GMT) ? gmtime(&tim) :
+			   localtime(&tim), 0L);
+	if (len > 0)
+	    metafy(oend, len, META_NOALLOC);
 	if (flags & STF_RAW)
 	    strcat(oend, ")");
     }
diff --git a/Src/builtin.c b/Src/builtin.c
index 4a97a31..572a0dd 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -1783,9 +1783,12 @@ fclist(FILE *f, Options ops, zlong first, zlong last,
 	       command, if required */
 	    if (tdfmt != NULL) {
 		struct tm *ltm;
+		int len;
 		ltm = localtime(&ent->stim);
-		if (ztrftime(timebuf, 256, tdfmt, ltm, 0L))
-		    fprintf(f, "%s  ", timebuf);
+		if ((len = ztrftime(timebuf, 256, tdfmt, ltm, 0L)) >= 0) {
+		    fwrite(timebuf, 1, len, f);
+		    fprintf(f, "  ");
+		}
 	    }
 	    /* display the time taken by the command, if required */
 	    if (OPT_ISSET(ops,'D')) {
diff --git a/Src/prompt.c b/Src/prompt.c
index 9e8589d..be067ee 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -271,7 +271,7 @@ static int
 putpromptchar(int doprint, int endchar, unsigned int *txtchangep)
 {
     char *ss, *hostnam;
-    int t0, arg, test, sep, j, numjobs;
+    int t0, arg, test, sep, j, numjobs, len;
     struct tm *tm;
     struct timezone dummy_tz;
     struct timeval tv;
@@ -673,12 +673,14 @@ putpromptchar(int doprint, int endchar, unsigned int *txtchangep)
 		     */
 		    for(j = 0, t0 = strlen(tmfmt)*8; j < 3; j++, t0*=2) {
 			addbufspc(t0);
-			if (ztrftime(bv->bp, t0, tmfmt, tm, tv.tv_usec) >= 0)
+			if ((len = ztrftime(bv->bp, t0, tmfmt, tm, tv.tv_usec))
+			    >= 0)
 			    break;
 		    }
 		    /* There is enough room for this because addbufspc(t0)
 		     * allocates room for t0 * 2 bytes. */
-		    metafy(bv->bp, -1, META_NOALLOC);
+		    if (len >= 0)
+			metafy(bv->bp, len, META_NOALLOC);
 		    bv->bp += strlen(bv->bp);
 		    zsfree(tmbuf);
 		    break;
diff --git a/Src/utils.c b/Src/utils.c
index 236661a..20e01a2 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2878,6 +2878,10 @@ ztrftimebuf(int *bufsizeptr, int decr)
  * not enough memory --- and return -1.  Not guaranteed to be portable,
  * since the strftime() interface doesn't make any guarantees about
  * the state of the buffer if it returns zero.
+ *
+ * fmt is metafied, but we need to unmetafy it on the fly to
+ * pass into strftime / combine with the output from strftime.
+ * The return value in buf is not metafied.
  */
 
 /**/
@@ -2898,8 +2902,14 @@ ztrftime(char *buf, int bufsize, char *fmt, struct tm *tm, long usec)
     char *origbuf = buf;
 
 
-    while (*fmt)
-	if (*fmt == '%') {
+    while (*fmt) {
+	if (*fmt == Meta) {
+	    int chr = fmt[1] ^ 32;
+	    if (ztrftimebuf(&bufsize, 1))
+		return -1;
+	    *buf++ = chr;
+	    fmt += 2;
+	} else if (*fmt == '%') {
 	    int strip;
 	    int digs = 3;
 
@@ -3083,8 +3093,21 @@ strftimehandling:
 		 */
 		{
 		    int size = fmt - fmtstart;
-		    char *tmp = zhalloc(size + 1);
+		    char *tmp, *last;
+		    tmp = zhalloc(size + 1);
 		    strncpy(tmp, fmtstart, size);
+		    last = fmt-1;
+		    if (*last == Meta) {
+			/*
+			 * This is for consistency in counting:
+			 * a metafiable character isn't actually
+			 * a valid strftime descriptor.
+			 *
+			 * Previous characters were explicitly checked,
+			 * so can't be metafied.
+			 */
+			*last = *++fmt ^ 32;
+		    }
 		    tmp[size] = '\0';
 		    *buf = '\1';
 		    if (!strftime(buf, bufsize + 2, tmp, tm))
@@ -3107,6 +3130,7 @@ strftimehandling:
 		return -1;
 	    *buf++ = *fmt++;
 	}
+    }
     *buf = '\0';
     return buf - origbuf;
 }
diff --git a/Src/watch.c b/Src/watch.c
index e1bdaa4..c804913 100644
--- a/Src/watch.c
+++ b/Src/watch.c
@@ -237,6 +237,7 @@ watchlog2(int inout, WATCH_STRUCT_UTMP *u, char *fmt, int prnt, int fini)
     time_t timet;
     struct tm *tm;
     char *fm2;
+    int len;
 # ifdef WATCH_UTMP_UT_HOST
     char *p;
     int i;
@@ -330,7 +331,9 @@ watchlog2(int inout, WATCH_STRUCT_UTMP *u, char *fmt, int prnt, int fini)
 		    }
 		    timet = getlogtime(u, inout);
 		    tm = localtime(&timet);
-		    ztrftime(buf, 40, fm2, tm, 0L);
+		    len = ztrftime(buf, 40, fm2, tm, 0L);
+		    if (len > 0)
+			metafy(buf, len, META_NOALLOC);
 		    printf("%s", (*buf == ' ') ? buf + 1 : buf);
 		    break;
 		case '%':

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Tue, 18 Aug 2015 15:21:45 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> This seems to work for strftime and primt -P %D and the tests pass.

I think there's a pre-existing confusion here, which is that we should
really treat ztrftime return value of both 0 and -1 as failure.  The 0
comes from strftime(), which returns the number of characters it's
succesfully converted (the manual helpfully says 0 "doesn't necessarily
indicate an error", just that it coverted nothing); the -1 was a local
enhancement.

I'll fix this locally before submitting.

pws

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Tue, 18 Aug 2015 16:07:33 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Tue, 18 Aug 2015 15:21:45 +0100
> Peter Stephenson <p.stephenson@samsung.com> wrote:
> > This seems to work for strftime and primt -P %D and the tests pass.
> 
> I think there's a pre-existing confusion here, which is that we should
> really treat ztrftime return value of both 0 and -1 as failure.  The 0
> comes from strftime(), which returns the number of characters it's
> succesfully converted (the manual helpfully says 0 "doesn't necessarily
> indicate an error", just that it coverted nothing); the -1 was a local
> enhancement.

Sigh.  No, I've got it now.  I think.

The -1 was supposed to be an enhancement so we really *can* detect
whether strftime() failed or not, by detecting whether strftime() output
something and then later gave up, or actually didn't output anything.
Dunno how robust it is, but I'll keep to it after all.

pws

Re: mkdir builtin and $'\0'

[Mikael Magnusson]

On Tue, Aug 18, 2015 at 5:07 PM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Tue, 18 Aug 2015 15:21:45 +0100
> Peter Stephenson <p.stephenson@samsung.com> wrote:
>> This seems to work for strftime and primt -P %D and the tests pass.
>
> I think there's a pre-existing confusion here, which is that we should
> really treat ztrftime return value of both 0 and -1 as failure.  The 0
> comes from strftime(), which returns the number of characters it's
> succesfully converted (the manual helpfully says 0 "doesn't necessarily
> indicate an error", just that it coverted nothing); the -1 was a local
> enhancement.
>
> I'll fix this locally before submitting.

I'm not sure what part of the code this is in reference to exactly,
but we only call strftime in one place so I suppose that place. You
saw this comment above the function?

/*
Like the system function, this returns the number of characters
copied, not including the terminating NUL.  This may be zero if the
string didn't fit.

As an extension, try to detect an error in strftime --- typically not
enough memory --- and return -1.  Not guaranteed to be portable, since
the strftime() interface doesn't make any guarantees about the state
of the buffer if it returns zero.
*/

It's not clear to me what "I'll fix this" implies, either :). If you
call strftime with an empty format string for example, I'm pretty sure
it'll return 0 but there was no error.

-- 
Mikael Magnusson

Re: mkdir builtin and $'\0'

[Stephane Chazelas]

2015-08-18 10:19:04 +0100, Stephane Chazelas:
[...]
> A bit worse:
> 
> $ strace -e chdir zsh -c "cd $'a\0b'; print -r -- \$PWD; pwd"
> chdir("/export/home/stephane/a")        = -1 ENOENT (No such file or directory)
> chdir("a\203 b")                        = 0
> chdir("..")                             = 0
> chdir("..")                             = 0
> chdir("..")                             = 0
> chdir("/home/stephane/a\203 b")         = 0
> /export/home/stephane/a
> /home/stephane/a� b
> +++ exited with 0 +++
> 
> I suppose the chdir("..")s come after the realisation that $PWD
> probably doesn't represent the current directory anymore (since
> chdir("$PWD/dir") doesn't work while chdir("dir") does).
[...]

How about my "cd" case?

The discrepancy between $PWD and pwd is strange there.

-- 
Stephane

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Tue, 18 Aug 2015 17:20:40 +0100
Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> 2015-08-18 10:19:04 +0100, Stephane Chazelas:
> [...]
> > A bit worse:
> > 
> > $ strace -e chdir zsh -c "cd $'a\0b'; print -r -- \$PWD; pwd"
> > chdir("/export/home/stephane/a")        = -1 ENOENT (No such file or directory)
> > chdir("a\203 b")                        = 0

Looks like that's this one (igroning for now the silent use of embedded
nulls).

diff --git a/Src/compat.c b/Src/compat.c
index b3a8b06..a0ce182 100644
--- a/Src/compat.c
+++ b/Src/compat.c
@@ -465,7 +465,7 @@ zchdir(char *dir)
     int currdir = -2;
 
     for (;;) {
-	if (!*dir || chdir(dir) == 0) {
+	if (!*dir || chdir(unmeta(dir)) == 0) {
 #ifdef HAVE_FCHDIR
            if (currdir >= 0)
                close(currdir);

pws

Re: mkdir builtin and $'\0'

[Peter Stephenson]

Test for this.

It's difficult to test with prompts because we don't have control over
the date, so I've stuck with testing strftime.  It's the same interface,
except that any re-metafication is in the caller.

[Is there any future (not right now, obviously) in a shell variable that
overrides the date/time that's output, for debugging?  I can think of
cases where this would be really useful for debugging output from
scripts that include timestamps and you want to diff different
versions.  You could freeze the time to the same value, then run
both versions, and get a controlled difference.]

pws

diff --git a/Test/V09datetime.ztst b/Test/V09datetime.ztst
index 5c66055..c935199 100644
--- a/Test/V09datetime.ztst
+++ b/Test/V09datetime.ztst
@@ -65,3 +65,7 @@
 >   JANUARY
 >090
 >1
+
+  print ${(V)"$(strftime $'%Y\0%m\0%d' 100000000)"}
+0:Embedded nulls
+>1973^@03^@03

Re: mkdir builtin and $'\0'

[Stephane Chazelas]

2015-08-19 10:52:50 +0100, Peter Stephenson:
[...]
> [Is there any future (not right now, obviously) in a shell variable that
> overrides the date/time that's output, for debugging?  I can think of
> cases where this would be really useful for debugging output from
> scripts that include timestamps and you want to diff different
> versions.  You could freeze the time to the same value, then run
> both versions, and get a controlled difference.]
[...]

Note that there's a "faketime" tool (using LD_PRELOAD) that can
be used to fake a particular time (and/or speed of time) which
is quite handy for testing date/calendar/timezone related things.

-- 
Stephane

Re: mkdir builtin and $'\0'

[Mikael Magnusson]

On Tue, Aug 18, 2015 at 6:37 PM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Tue, 18 Aug 2015 17:20:40 +0100
> Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
>> 2015-08-18 10:19:04 +0100, Stephane Chazelas:
>> [...]
>> > A bit worse:
>> >
>> > $ strace -e chdir zsh -c "cd $'a\0b'; print -r -- \$PWD; pwd"
>> > chdir("/export/home/stephane/a")        = -1 ENOENT (No such file or directory)
>> > chdir("a\203 b")                        = 0
>
> Looks like that's this one (igroning for now the silent use of embedded
> nulls).
>
> diff --git a/Src/compat.c b/Src/compat.c
> index b3a8b06..a0ce182 100644
> --- a/Src/compat.c
> +++ b/Src/compat.c
> @@ -465,7 +465,7 @@ zchdir(char *dir)
>      int currdir = -2;
>
>      for (;;) {
> -       if (!*dir || chdir(dir) == 0) {
> +       if (!*dir || chdir(unmeta(dir)) == 0) {
>  #ifdef HAVE_FCHDIR
>             if (currdir >= 0)
>                 close(currdir);

[replying to the correct thread]

% mkdir /tmp/ホ
% cd /tmp/ホ
cd: no such file or directory: /tmp/ホ
% cd /tmp
% cd ホ
% pwd
/tmp/ホ
% echo ホ|xxd
00000000: e383 9b0a

> It doesn't happen with all japanese characters. I'm hoping one of
> those bytes in the output mean something to you. I'm also assuming
> this is related to the recent fudging of unmetafy in chdir().

I just checked and
% octtohex 203
0x83

this is the \203 from the chdir strace above, and 83 from the middle
of my character. Do we have inconsistent metafication in this string
somehow?

-- 
Mikael Magnusson

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Fri, 21 Aug 2015 08:20:55 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> % mkdir /tmp/ホ
> % cd /tmp/ホ
> cd: no such file or directory: /tmp/ホ
> % cd /tmp
> % cd ホ
> % pwd
> /tmp/ホ
> % echo ホ|xxd
> 00000000: e383 9b0a
> 
> Do we have inconsistent metafication in this string somehow?

Yes --- it appears by tracing system calls everything is unmetafied but
null terminated up to lchdir().  The culprit for the case Stephane saw
was the second of a pair of lchdir() calls, where there's a string that
needs unmetafying, while you're hitting the first, which is raw.  I
think this fixes both cases.

I think we weren't seeing this before because Stephane's case is
actually quite unusual --- we failed to changed directory normally
because it doesn't exist, and only then hit the second lchdir().

There are a couple of remaining lchdir() calls in glob.c.  I haven't
determined whether the arguments are already unmetafied or not.

(Er, this is if people think we really need to fix up little-used
builtins of this sort... mmm....)

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 572a0dd..3d34aa7 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -1163,7 +1163,7 @@ cd_try_chdir(char *pfix, char *dest, int hard)
      * or a parent directory is renamed in the interim.
      */
     if (lchdir(buf, NULL, hard) &&
-	(pfix || *dest == '/' || lchdir(dest, NULL, hard))) {
+	(pfix || *dest == '/' || lchdir(unmeta(dest), NULL, hard))) {
 	free(buf);
 	return NULL;
     }
diff --git a/Src/compat.c b/Src/compat.c
index a0ce182..db46852 100644
--- a/Src/compat.c
+++ b/Src/compat.c
@@ -454,8 +454,13 @@ zgetcwd(void)
     return ret;
 }
 
-/* chdir with arbitrary long pathname.  Returns 0 on success, -1 on normal *
- * failure and -2 when chdir failed and the current directory is lost.  */
+/*
+ * chdir with arbitrary long pathname.  Returns 0 on success, -1 on normal *
+ * failure and -2 when chdir failed and the current directory is lost.
+ *
+ * This is to be treated as if at system level, so dir is unmetafied but
+ * terminated by a NULL.
+ */
 
 /**/
 mod_export int
@@ -465,7 +470,7 @@ zchdir(char *dir)
     int currdir = -2;
 
     for (;;) {
-	if (!*dir || chdir(unmeta(dir)) == 0) {
+	if (!*dir || chdir(dir) == 0) {
 #ifdef HAVE_FCHDIR
            if (currdir >= 0)
                close(currdir);
diff --git a/Src/utils.c b/Src/utils.c
index 20e01a2..4c4dc55 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -6440,10 +6440,15 @@ init_dirsav(Dirsav d)
     d->dirfd = d->level = -1;
 }
 
-/* Change directory, without following symlinks.  Returns 0 on success, -1 *
- * on failure.  Sets errno to ENOTDIR if any symlinks are encountered.  If *
- * fchdir() fails, or the current directory is unreadable, we might end up *
- * in an unwanted directory in case of failure.                            */
+/*
+ * Change directory, without following symlinks.  Returns 0 on success, -1
+ * on failure.  Sets errno to ENOTDIR if any symlinks are encountered.  If
+ * fchdir() fails, or the current directory is unreadable, we might end up
+ * in an unwanted directory in case of failure.
+ *
+ * path is an unmetafied but null-terminated string, as needed by system
+ * calls.
+ */
 
 /**/
 mod_export int
diff --git a/Test/D07multibyte.ztst b/Test/D07multibyte.ztst
index 644d280..0e3e98d 100644
--- a/Test/D07multibyte.ztst
+++ b/Test/D07multibyte.ztst
@@ -496,3 +496,15 @@
 >OK
 >OK
 >OK
+
+  () {
+     emulate -L zsh
+     setopt errreturn
+     local cdpath=(.)
+     mkdir ホ
+     cd ホ
+     cd ..
+     cd ./ホ
+     cd ..
+  }
+0:cd with special characters

Re: mkdir builtin and $'\0'

[Peter Stephenson]

On Fri, 21 Aug 2015 09:57:21 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> There are a couple of remaining lchdir() calls in glob.c.  I haven't
> determined whether the arguments are already unmetafied or not.

Evidence seems quite positive that pathbuf in glob.c (and pattern.c) is
supposed to be metafied.

(However, a couple of postdoctoral researchers at an institute in Outer
Mongolia have found new evidence that under some circumstances certain
components of the path may be slightly sensitive to unmetafication in
the infra-red...  I wish this were funny.)

Mikael might to try a bit of globbing with some of his usefully named
directories.

pws

diff --git a/Src/glob.c b/Src/glob.c
index 3af4690..dea1bf5 100644
--- a/Src/glob.c
+++ b/Src/glob.c
@@ -102,8 +102,14 @@ int badcshglob;
 /**/
 int pathpos;		/* position in pathbuf (needed by pattern code) */
 
+/*
+ * pathname buffer (needed by pattern code).
+ * It is currently believed the string in here is stored metafied and is
+ * unmetafied temporarily as needed by system calls.
+ */
+
 /**/
-char *pathbuf;		/* pathname buffer (needed by pattern code) */
+char *pathbuf;
 
 typedef struct stat *Statptr;	 /* This makes the Ultrix compiler happy.  Go figure. */
 
@@ -495,7 +501,7 @@ scanner(Complist q, int shortcircuit)
 
 	    if (l >= PATH_MAX)
 		return;
-	    err = lchdir(pathbuf + pathbufcwd, &ds, 0);
+	    err = lchdir(unmeta(pathbuf + pathbufcwd), &ds, 0);
 	    if (err == -1)
 		return;
 	    if (err) {
@@ -517,7 +523,7 @@ scanner(Complist q, int shortcircuit)
 		    else if (!strcmp(str, "..")) {
 			struct stat sc, sr;
 
-			add = (stat("/", &sr) || stat(pathbuf, &sc) ||
+			add = (stat("/", &sr) || stat(unmeta(pathbuf), &sc) ||
 			       sr.st_ino != sc.st_ino ||
 			       sr.st_dev != sc.st_dev);
 		    }
@@ -564,7 +570,7 @@ scanner(Complist q, int shortcircuit)
 
 		    DPUTS(pathpos == pathbufcwd,
 			  "BUG: filename longer than PATH_MAX");
-		    err = lchdir(pathbuf + pathbufcwd, &ds, 0);
+		    err = lchdir(unmeta(pathbuf + pathbufcwd), &ds, 0);
 		    if (err == -1)
 			break;
 		    if (err) {

mkdir builtin and $'\0'

[Oliver Kiddle]

On 21 Aug, Mikael Magnusson wrote:
> % mkdir /tmp/ホ
> % cd /tmp/ホ

While catching up on the mails, I didn't immediately spot the message
with the fix for this so instead tried to reproduce it. In the process,
I noticed an additional missing unmeta() in the fallback code used when
getcwd() returns NULL:

% rmdir $PWD
% pwd
/tmp/ャ»

Oliver

diff --git a/Src/compat.c b/Src/compat.c
index db46852..9041c0b 100644
--- a/Src/compat.c
+++ b/Src/compat.c
@@ -448,7 +448,7 @@ zgetcwd(void)
     }
 #endif /* HAVE_GETCWD */
     if (!ret)
-	ret = pwd;
+	ret = unmeta(pwd);
     if (!ret)
 	ret = dupstring(".");
     return ret;