PATCH: Allow setting $0 when POSIX_ARGZERO is not set

PATCH: Allow setting $0 when POSIX_ARGZERO is not set

[Mikael Magnusson]

I don't think this should hurt anything.

---
 Src/params.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/Src/params.c b/Src/params.c
index 98541a6..3b75735 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -196,7 +196,7 @@ static const struct gsu_integer ttyidle_gsu =
 { ttyidlegetfn, nullintsetfn, stdunsetfn };
 
 static const struct gsu_scalar argzero_gsu =
-{ argzerogetfn, nullstrsetfn, nullunsetfn };
+{ argzerogetfn, argzerosetfn, nullunsetfn };
 static const struct gsu_scalar username_gsu =
 { usernamegetfn, usernamesetfn, stdunsetfn };
 static const struct gsu_scalar dash_gsu =
@@ -4044,6 +4044,21 @@ lcsetfn(Param pm, char *x)
 }
 #endif /* USE_LOCALE */
 
+/* Function to set value for special parameter `0' */
+
+/**/
+static void
+argzerosetfn(UNUSED(Param pm), char *x)
+{
+    if (x) {
+	if (!isset(POSIXARGZERO)) {
+	    zsfree(argzero);
+	    argzero = ztrdup(x);
+	}
+	zsfree(x);
+    }
+}
+
 /* Function to get value for special parameter `0' */
 
 /**/
-- 
2.4.0

Re: PATCH: Allow setting $0 when POSIX_ARGZERO is not set

[Bart Schaefer]

On Jun 16,  1:44am, Mikael Magnusson wrote:
}
} I don't think this should hurt anything.

When sourcing a script the global argzero (which has been initialized
from runscript) is not malloc'd memory.

==25941== Invalid free() / delete / delete[]
==25941==    at 0x4004EFA: free (vg_replace_malloc.c:235)
==25941==    by 0x80950D7: zsfree (mem.c:1828)
==25941==    by 0x80A4DE9: argzerosetfn (params.c:4055)
==25941==    by 0x80A0CDF: assignstrvalue (params.c:2349)
==25941==    by 0x80A22D2: assignsparam (params.c:2806)
==25941==    by 0x806642F: addvars (exec.c:2317)
==25941==    by 0x80635F0: execsimple (exec.c:1117)
==25941==    by 0x80639AD: execlist (exec.c:1247)
==25941==    by 0x80634D1: execode (exec.c:1074)
==25941==    by 0x807F4EC: loop (init.c:207)
==25941==    by 0x808290B: zsh_main (init.c:1674)
==25941==    by 0x804C2E9: main (main.c:93)
==25941==  Address 0xBEEAFA62 is on thread 1's stack

I'm not sure if this comes up anywhere else.  I wasn't trivially able
to make it fail.

I think it would cause leaks to have parseargs() call ztrdup() for its
runscript argument, so it looks like setupshin() should do so when
assigning to argzero.

There are probably other race conditions if a signal were to arrive
while the shell is still initializing state.  Maybe we should get a
queue_signals() in there somewhere early.


diff --git a/Src/init.c b/Src/init.c
index 102276a..0fe4d75 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1117,8 +1117,9 @@ setupshin(char *runscript)
 	    exit(127);
 	}
 	scriptfilename = sfname;
-	zsfree(argzero); /* ztrdup'd in parseargs */
-	argzero = runscript;
+	sfname = argzero; /* copy to avoid race condition */
+	argzero = ztrdup(runscript);
+	zsfree(sfname); /* argzero ztrdup'd in parseargs */
     }
     /*
      * We only initialise line numbering once there is a script to

Re: PATCH: Allow setting $0 when POSIX_ARGZERO is not set

[Mikael Magnusson]

On Tue, Jun 16, 2015 at 5:50 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jun 16,  1:44am, Mikael Magnusson wrote:
> }
> } I don't think this should hurt anything.
>
> When sourcing a script the global argzero (which has been initialized
> from runscript) is not malloc'd memory.
>
> ==25941== Invalid free() / delete / delete[]
> ==25941==    at 0x4004EFA: free (vg_replace_malloc.c:235)
> ==25941==    by 0x80950D7: zsfree (mem.c:1828)
> ==25941==    by 0x80A4DE9: argzerosetfn (params.c:4055)
> ==25941==    by 0x80A0CDF: assignstrvalue (params.c:2349)
> ==25941==    by 0x80A22D2: assignsparam (params.c:2806)
> ==25941==    by 0x806642F: addvars (exec.c:2317)
> ==25941==    by 0x80635F0: execsimple (exec.c:1117)
> ==25941==    by 0x80639AD: execlist (exec.c:1247)
> ==25941==    by 0x80634D1: execode (exec.c:1074)
> ==25941==    by 0x807F4EC: loop (init.c:207)
> ==25941==    by 0x808290B: zsh_main (init.c:1674)
> ==25941==    by 0x804C2E9: main (main.c:93)
> ==25941==  Address 0xBEEAFA62 is on thread 1's stack
>
> I'm not sure if this comes up anywhere else.  I wasn't trivially able
> to make it fail.
>
> I think it would cause leaks to have parseargs() call ztrdup() for its
> runscript argument, so it looks like setupshin() should do so when
> assigning to argzero.
>
> There are probably other race conditions if a signal were to arrive
> while the shell is still initializing state.  Maybe we should get a
> queue_signals() in there somewhere early.

Oh, in that case maybe it's a lot more trouble than it's worth...

> diff --git a/Src/init.c b/Src/init.c
> index 102276a..0fe4d75 100644
> --- a/Src/init.c
> +++ b/Src/init.c
> @@ -1117,8 +1117,9 @@ setupshin(char *runscript)
>             exit(127);
>         }
>         scriptfilename = sfname;
> -       zsfree(argzero); /* ztrdup'd in parseargs */
> -       argzero = runscript;
> +       sfname = argzero; /* copy to avoid race condition */
> +       argzero = ztrdup(runscript);
> +       zsfree(sfname); /* argzero ztrdup'd in parseargs */
>      }
>      /*
>       * We only initialise line numbering once there is a script to



-- 
Mikael Magnusson

Re: PATCH: Allow setting $0 when POSIX_ARGZERO is not set

[Bart Schaefer]

On Jun 16,  8:25am, Mikael Magnusson wrote:
}
} > There are probably other race conditions if a signal were to arrive
} > while the shell is still initializing state.  Maybe we should get a
} > queue_signals() in there somewhere early.
} 
} Oh, in that case maybe it's a lot more trouble than it's worth...

Sorry, the "other race conditions" part has nothing to do with setting
argzero, I was merely commenting that if I noticed a race in this part
of the initialization code there are probably races elsewhere as well.

Re: PATCH: Allow setting $0 when POSIX_ARGZERO is not set

[Mikael Magnusson]

On Tue, Jun 16, 2015 at 5:58 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jun 16,  8:25am, Mikael Magnusson wrote:
> }
> } > There are probably other race conditions if a signal were to arrive
> } > while the shell is still initializing state.  Maybe we should get a
> } > queue_signals() in there somewhere early.
> }
> } Oh, in that case maybe it's a lot more trouble than it's worth...
>
> Sorry, the "other race conditions" part has nothing to do with setting
> argzero, I was merely commenting that if I noticed a race in this part
> of the initialization code there are probably races elsewhere as well.

Okay, so it should be fine to push both of the patches in the thread?

-- 
Mikael Magnusson

Re: PATCH: Allow setting $0 when POSIX_ARGZERO is not set

[Bart Schaefer]

On Jun 17,  1:46am, Mikael Magnusson wrote:
} Subject: Re: PATCH: Allow setting $0 when POSIX_ARGZERO is not set
}
} On Tue, Jun 16, 2015 at 5:58 PM, Bart Schaefer
} <schaefer@brasslantern.com> wrote:
} > On Jun 16,  8:25am, Mikael Magnusson wrote:
} > }
} > } > There are probably other race conditions if a signal were to arrive
} > } > while the shell is still initializing state.  Maybe we should get a
} > } > queue_signals() in there somewhere early.
} > }
} > } Oh, in that case maybe it's a lot more trouble than it's worth...
} >
} > Sorry, the "other race conditions" part has nothing to do with setting
} > argzero, I was merely commenting that if I noticed a race in this part
} > of the initialization code there are probably races elsewhere as well.
} 
} Okay, so it should be fine to push both of the patches in the thread?

I had already committed mine, then had a bit of a problem resolving the
ChangeLog diffs when you pushed 54c2c442eeee46b28d5596e7e83812df01d1f13f
(for which by the way you left out the article number in the commit log
message).

Anyway 35482 is pushed now.