This is already implemented <https://bugzilla.redhat.com/60870> in some distributions of bash and tcsh <https://bugzilla.redhat.com/711066>. Steps to reproduce: echo 'int main () { return 0; }' > u.c gcc -o u u.c -Wl,-dynamic-linker,/foo/bar/baz zsh -c ./u Bug: https://bugzilla.redhat.com/711067 --- Src/exec.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- configure.ac | 2 +- 2 files changed, 131 insertions(+), 4 deletions(-) diff --git a/Src/exec.c b/Src/exec.c index 2a8185c..18408d7 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -30,6 +30,10 @@ #include "zsh.mdh" #include "exec.pro" +#ifdef HAVE_ELF_H +# include <elf.h> +#endif + /* Flags for last argument of addvars */ enum { @@ -427,6 +431,112 @@ execcursh(Estate state, int do_exec) return lastval; } +/* The following code is taken from <https://bugzilla.redhat.com/60870>. */ +#ifdef HAVE_ELF_H +static int +checkelfinterp(const char *pth, int fd, const char *sample, int sample_len) +{ + off_t offset = -1; + int eno = ENOENT; + + /* Read the offset of the interpreter string. */ + if (sample[EI_CLASS] == ELFCLASS32 && sample_len >= sizeof(Elf32_Ehdr)) { + Elf32_Ehdr ehdr; + Elf32_Phdr *phdr; + Elf32_Half nphdr; + + /* + * We have to copy the data since the sample buffer might not be + * aligned correctly to be accessed as an Elf32_Ehdr struct. + */ + memcpy(&ehdr, sample, sizeof(Elf32_Ehdr)); + + nphdr = ehdr.e_phnum; + phdr = (Elf32_Phdr *) zalloc((size_t)nphdr * (size_t)ehdr.e_phentsize); + if (phdr != NULL) { + if (lseek(fd, ehdr.e_phoff, SEEK_SET) != -1) + sample_len = read(fd, phdr, nphdr * ehdr.e_phentsize); + else + sample_len = -1; + + if (sample_len == nphdr * ehdr.e_phentsize) + while (nphdr-- > 0) + if (phdr[nphdr].p_type == PT_INTERP) { + offset = phdr[nphdr].p_offset; + break; + } + free(phdr); + } + } else if (sample[EI_CLASS] == ELFCLASS64 + && sample_len >= sizeof(Elf64_Ehdr)) { + Elf64_Ehdr ehdr; + Elf64_Phdr *phdr; + Elf32_Half nphdr; + + /* + * We have to copy the data since the sample buffer might not be + * aligned correctly to be accessed as an Elf64_Ehdr struct. + */ + memcpy(&ehdr, sample, sizeof(Elf64_Ehdr)); + + nphdr = ehdr.e_phnum; + phdr = (Elf64_Phdr *) zalloc((size_t)nphdr * (size_t)ehdr.e_phentsize); + if (phdr != NULL) { + if (lseek(fd, ehdr.e_phoff, SEEK_SET) != -1) + sample_len = read(fd, phdr, + nphdr * ehdr.e_phentsize); + else + sample_len = -1; + + if (sample_len == nphdr * ehdr.e_phentsize) + while (nphdr-- > 0) + if (phdr[nphdr].p_type == PT_INTERP) { + offset = phdr[nphdr].p_offset; + break; + } + free(phdr); + } + } + + if (offset != -1) { + ssize_t maxlen = 0; + ssize_t actlen = 0; + ssize_t nread = 0; + off_t pos = 0; + char *interp = NULL; + + for (;;) { + if (actlen >= maxlen) { + char *newinterp = zrealloc(interp, maxlen += 200); + if (newinterp == NULL) + /* out of memroy */ + break; + interp = newinterp; + } + + if (lseek(fd, offset + pos, SEEK_SET) == -1) + break; + + if ((nread = read(fd, interp + pos, maxlen - pos)) == -1) + break; + + if (memchr(interp + pos, '\0', nread) != NULL) { + zerr("%s: %s: bad ELF interpreter", pth, interp); + eno = ENOEXEC; + break; + } + + actlen += nread; + pos += nread; + } + + free(interp); + } + + return eno; +} +#endif + /* execve after handling $_ and #! */ #define POUNDBANGLIMIT 64 @@ -468,12 +578,20 @@ zexecve(char *pth, char **argv, char **newenvp) int fd, ct, t0; if ((fd = open(pth, O_RDONLY|O_NOCTTY)) >= 0) { + size_t hdrsize = POUNDBANGLIMIT; +#ifdef HAVE_ELF_H + /* Inspect 32 and 64 ELF */ + if (sizeof(Elf64_Ehdr) > hdrsize) + hdrsize = sizeof(Elf64_Ehdr); + if (sizeof(Elf32_Ehdr) > hdrsize) + hdrsize = sizeof(Elf32_Ehdr); +#endif argv0 = *argv; *argv = pth; - ct = read(fd, execvebuf, POUNDBANGLIMIT); - close(fd); + ct = read(fd, execvebuf, hdrsize); if (ct > 0) { if (execvebuf[0] == '#') { + close(fd); if (execvebuf[1] == '!') { for (t0 = 0; t0 != ct; t0++) if (execvebuf[t0] == '\n') @@ -513,6 +631,7 @@ zexecve(char *pth, char **argv, char **newenvp) execve("/bin/sh", argv - 1, newenvp); } } else if (eno == ENOEXEC) { + close(fd); for (t0 = 0; t0 != ct; t0++) if (!execvebuf[t0]) break; @@ -521,7 +640,15 @@ zexecve(char *pth, char **argv, char **newenvp) winch_unblock(); execve("/bin/sh", argv - 1, newenvp); } - } +#ifdef HAVE_ELF_H + } else if (eno == ENOENT + && ct > EI_NIDENT + && memcmp(execvebuf, ELFMAG, SELFMAG) == 0) { + eno = checkelfinterp(pth, fd, execvebuf, ct); + close(fd); +#endif + } else + close(fd); } else eno = errno; *argv = argv0; diff --git a/configure.ac b/configure.ac index e4de193..645385d 100644 --- a/configure.ac +++ b/configure.ac @@ -676,7 +676,7 @@ AC_CHECK_HEADERS(sys/time.h sys/times.h sys/select.h termcap.h termio.h \ utmp.h utmpx.h sys/types.h pwd.h grp.h poll.h sys/mman.h \ netinet/in_systm.h pcre.h langinfo.h wchar.h stddef.h \ sys/stropts.h iconv.h ncurses.h ncursesw/ncurses.h \ - ncurses/ncurses.h) + ncurses/ncurses.h elf.h) if test x$dynamic = xyes; then AC_CHECK_HEADERS(dlfcn.h) AC_CHECK_HEADERS(dl.h) -- 2.1.0
... in order to prevent SIGSEGV on the following input when running in the interactive mode: <<X;' ^C ^C The console output looks like this: kdudka% <<X;' quote> kdudka% heredoc> zsh: segmentation fault (core dumped) zsh -f Bug: https://bugzilla.redhat.com/972624 --- Src/parse.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Src/parse.c b/Src/parse.c index 91a81e1..374d18b 100644 --- a/Src/parse.c +++ b/Src/parse.c @@ -605,6 +605,7 @@ par_event(int endtok) if (!par_event(endtok)) { ecused = oec; ecbuf[p] |= wc_bdata(Z_END); + clear_hdocs(); } } return 1; -- 2.1.0
Kamil Dudka wrote on Fri, Apr 17, 2015 at 15:25:54 +0200:
> +/* The following code is taken from <https://bugzilla.redhat.com/60870>. */
> +#ifdef HAVE_ELF_H
> +static int
> +checkelfinterp(const char *pth, int fd, const char *sample, int sample_len)
Is this code available under a license compatible with zsh's license?
The bug report doesn't state a license explicitly, so I presume any code
on the bug report is GPLed unless stated otherwise.
Sorry to be license nanny.
Cheers,
Daniel
On Friday 17 April 2015 13:49:57 Daniel Shahaf wrote:
> Kamil Dudka wrote on Fri, Apr 17, 2015 at 15:25:54 +0200:
> > +/* The following code is taken from <https://bugzilla.redhat.com/60870>.
> > */ +#ifdef HAVE_ELF_H
> > +static int
> > +checkelfinterp(const char *pth, int fd, const char *sample, int
> > sample_len)
> Is this code available under a license compatible with zsh's license?
>
> The bug report doesn't state a license explicitly, so I presume any code
> on the bug report is GPLed unless stated otherwise.
>
> Sorry to be license nanny.
>
> Cheers,
>
> Daniel
Good point. I have asked the original author about this privately...
Kamil
This behaviour got mentioned on one of the blogs that I tend to read here: http://utcc.utoronto.ca/~cks/space/blog/unix/BashSuperintelligentExec I would agree with the conclusion of that blog writer that this is going a little bit overboard. Also, I'm fairly sure that this would require linking with libelf on some systems which is an added dependency. Oliver
On Fri, 2015-04-17 at 15:25 +0200, Kamil Dudka wrote:
> This is already implemented <https://bugzilla.redhat.com/60870> in some
> distributions of bash and tcsh <https://bugzilla.redhat.com/711066>.
>
> Steps to reproduce:
>
> echo 'int main () { return 0; }' > u.c
> gcc -o u u.c -Wl,-dynamic-linker,/foo/bar/baz
> zsh -c ./u
>
> Bug: https://bugzilla.redhat.com/711067
This is such a corner case.
It shouldn't be the shell's job to reinterpret the errno codes returned
by the underlying OS.
This patch does not belong in the shell code IMHO, but at a lower-level.
I've noticed that this patch has not been accepted upstream in bash yet.
Phil.
On Fri, Apr 17, 2015 at 8:41 AM, Philippe Troin <phil@fifi.org> wrote:
>> Bug: https://bugzilla.redhat.com/711067
>
> This is such a corner case.
> It shouldn't be the shell's job to reinterpret the errno codes returned
> by the underlying OS.
> This patch does not belong in the shell code IMHO, but at a lower-level.
My thoughts exactly. This is NOT the shell's responsibility and I
recommend rejecting this patch.
On Friday 17 April 2015 17:21:40 Oliver Kiddle wrote:
> This behaviour got mentioned on one of the blogs that I tend to read
> here:
>
> http://utcc.utoronto.ca/~cks/space/blog/unix/BashSuperintelligentExec
>
> I would agree with the conclusion of that blog writer that this is going
> a little bit overboard.
>
> Also, I'm fairly sure that this would require linking with libelf on
> some systems which is an added dependency.
>
> Oliver
Why would the patch require linking with libelf?
The patch does not rely on any symbols provided by the libelf library. It
just optionally uses <elf.h> if the header file is available during build.
Kamil
On Fri, 17 Apr 2015 15:25:55 +0200 Kamil Dudka <kdudka@redhat.com> wrote: > ... in order to prevent SIGSEGV on the following input when running > in the interactive mode: > > <<X;' > ^C > > ^C I think that still leaves cruft around of other kinds. I think the fix is something like the following. diff --git a/Src/parse.c b/Src/parse.c index 91a81e1..985eb8e 100644 --- a/Src/parse.c +++ b/Src/parse.c @@ -605,6 +605,7 @@ par_event(int endtok) if (!par_event(endtok)) { ecused = oec; ecbuf[p] |= wc_bdata(Z_END); + return errflag ? 0 : 1; } } return 1; pws
Kamil Dudka wrote:
> Why would the patch require linking with libelf?
My mistake sorry, I see the patch is using read() direct to the structures from
elf.h rather than calling routines. I guess it gets away without
handling byte order because that would result in an errno other than
ENOENT from exec.
Still I'm in agreement with Philippe and Bart on this one. The shell is
not the right place for this.
Oliver
On Friday 17 April 2015 20:17:53 Peter Stephenson wrote:
> On Fri, 17 Apr 2015 15:25:55 +0200
>
> Kamil Dudka <kdudka@redhat.com> wrote:
> > ... in order to prevent SIGSEGV on the following input when running
> > in the interactive mode:
> >
> > <<X;'
> > ^C
> >
> > ^C
>
> I think that still leaves cruft around of other kinds. I think the fix
> is something like the following.
>
> diff --git a/Src/parse.c b/Src/parse.c
> index 91a81e1..985eb8e 100644
> --- a/Src/parse.c
> +++ b/Src/parse.c
> @@ -605,6 +605,7 @@ par_event(int endtok)
> if (!par_event(endtok)) {
> ecused = oec;
> ecbuf[p] |= wc_bdata(Z_END);
> + return errflag ? 0 : 1;
> }
> }
> return 1;
>
> pws
Thanks for the patch! This works for me.
Kamil
[-- Attachment #1: Type: text/plain, Size: 921 bytes --] On Friday, April 17, 2015 9:17:53 PM CET Peter Stephenson wrote: > On Fri, 17 Apr 2015 15:25:55 +0200 > > Kamil Dudka <kdudka@redhat.com> wrote: > > ... in order to prevent SIGSEGV on the following input when running > > in the interactive mode: > > > > <<X;' > > ^C > > > > ^C > > I think that still leaves cruft around of other kinds. I think the fix > is something like the following. > > diff --git a/Src/parse.c b/Src/parse.c > index 91a81e1..985eb8e 100644 > --- a/Src/parse.c > +++ b/Src/parse.c > @@ -605,6 +605,7 @@ par_event(int endtok) > if (!par_event(endtok)) { > ecused = oec; > ecbuf[p] |= wc_bdata(Z_END); > + return errflag ? 0 : 1; > } > } > return 1; > > pws Since the above patch was applied, zsh exits successfully (despite it reports parse error) on the attached example: % zsh parse-error.sh; echo $? parse-error.sh:4: parse error 0 Is this expected? Kamil [-- Attachment #2: parse-error.sh --] [-- Type: application/x-shellscript, Size: 28 bytes --]
On Thu, 2018-11-29 at 17:24 +0100, Kamil Dudka wrote: > Since the above patch was applied, zsh exits successfully (despite it reports > parse error) on the attached example: > > % zsh parse-error.sh; echo $? > parse-error.sh:4: parse error > 0 > >... > > cat <<EOF > $(print <<XXX > EOF I don't think the problem is with the changed code, which is signalling the error as it should. I think it's up above. There's some slightly icky linkage between lex errors and the top level requiring tok to be LEXERR. The simple fix using the existing signalling looks like the following. I definitely don't think the tok = LEXERR has a moral right to percolate through in the way it must previously have been doing to avoid this, and the lexer does certainly have the right to update the token when signalling a parse error, so (famous last words) it's hard to see what could be wrong with this... pws diff --git a/Src/lex.c b/Src/lex.c index fa29da3..f43bcc7 100644 --- a/Src/lex.c +++ b/Src/lex.c @@ -1613,6 +1613,7 @@ parsestr(char **s) zerr("parse error near `%c'", err); else zerr("parse error"); + tok = LEXERR; } } return err;
On Thu, Nov 29, 2018 at 9:42 AM Peter Stephenson
<p.stephenson@samsung.com> wrote:
>
> There's some slightly icky linkage between lex errors and the top level
> requiring tok to be LEXERR. The simple fix using the existing
> signalling looks like the following. I definitely don't think the tok =
> LEXERR has a moral right to percolate through in the way it must
> previously have been doing to avoid this, and the lexer does certainly
> have the right to update the token when signalling a parse error, so
> (famous last words) it's hard to see what could be wrong with this...
Might this need to be conditional upon ... something ... so that e.g.
${(z)...} on a malformed here-document doesn't throw an error? I
don't have the code handy to poke around.
On Thu, 29 Nov 2018 10:39:15 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Thu, Nov 29, 2018 at 9:42 AM Peter Stephenson
> <p.stephenson@samsung.com> wrote:
> >
> > There's some slightly icky linkage between lex errors and the top level
> > requiring tok to be LEXERR. The simple fix using the existing
> > signalling looks like the following. I definitely don't think the tok =
> > LEXERR has a moral right to percolate through in the way it must
> > previously have been doing to avoid this, and the lexer does certainly
> > have the right to update the token when signalling a parse error, so
> > (famous last words) it's hard to see what could be wrong with this...
>
> Might this need to be conditional upon ... something ... so that e.g.
> ${(z)...} on a malformed here-document doesn't throw an error? I
> don't have the code handy to poke around.
All this change is doing is setting the token to LEXERR in a case which
already loudly complains about the error and sets the error flag (this
is in a function parsestr() which calls parsetrnoerr() just above).
In most cases you'd expect the token in such a case already to be
LEXERR: Kamil's test includes an incomplete $( inside a here document so
there was some interesting nested parsing involved.
So anything reacting badly to the change in the token needs fixing.
pws
On Thursday, November 29, 2018 6:34:04 PM CET Peter Stephenson wrote:
> On Thu, 2018-11-29 at 17:24 +0100, Kamil Dudka wrote:
> > Since the above patch was applied, zsh exits successfully (despite it
> > reports >
> > parse error) on the attached example:
> >
> >
> > % zsh parse-error.sh; echo $?
> > parse-error.sh:4: parse error
> > 0
> >
> >...
> >
> > cat <<EOF
> > $(print <<XXX
> > EOF
>
> I don't think the problem is with the changed code, which is signalling the
> error as it should. I think it's up above.
>
> There's some slightly icky linkage between lex errors and the top level
> requiring tok to be LEXERR. The simple fix using the existing
> signalling looks like the following. I definitely don't think the tok =
> LEXERR has a moral right to percolate through in the way it must
> previously have been doing to avoid this, and the lexer does certainly
> have the right to update the token when signalling a parse error, so
> (famous last words) it's hard to see what could be wrong with this...
>
> pws
>
> diff --git a/Src/lex.c b/Src/lex.c
> index fa29da3..f43bcc7 100644
> --- a/Src/lex.c
> +++ b/Src/lex.c
> @@ -1613,6 +1613,7 @@ parsestr(char **s)
> zerr("parse error near `%c'", err);
> else
> zerr("parse error");
> + tok = LEXERR;
> }
> }
> return err;
Thanks for the quick fix! It seems to work perfectly.
Kamil