Dirk Fieldhouse <fieldhouse@gmx.net> wrote:
>
> In POSIX.1-2017 ("simultaneously IEEE Std 1003.1™-2017 and The Open
> Group Technical Standard Base Specifications, Issue 7")
> <http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09

>,
> we read under '2.9.1 Simple Commands'
>
> "Variable assignments shall be performed as follows:
> ...
> -    If the command name is a standard utility implemented as a function
> (see XBD Utility), the effect of variable assignments shall be as if the
> utility was not implemented as a function.
> ...
> -    If the command name is a function that is not a standard utility
> implemented as a function, variable assignments shall affect the current
> execution environment during the execution of the function. It is
> unspecified:
>
>     *   Whether or not the variable assignments persist after the
> completion of the function
>
>     *   Whether or not the variables gain the export attribute during
> the execution of the function
>
>     *   Whether or not export attributes gained as a result of the
> variable assignments persist after the completion of the function (if
> variable assignments persist after the completion of the function)"

POSIX used to require the current dash behaviour.  However, you're
right that this is no longer the case.

This patch will remove the persistence of the variable assignment.

I have considered the exporting the variables during the function
execution but have decided against it because:

1) It makes the code bigger.
2) dash has never done this in the past.
3) You cannot use this portably anyway.

Reported-by: Dirk Fieldhouse <fieldhouse@gmx.net>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

The parsing of parameter expansion inside inner double quotes
breaks because we never look for ENDVAR while innerdq is true.

	echo "${x#"${x+''}"''}

This patch fixes it by pushing the syntax stack if innerdq is
true and we enter a new parameter expansion.

This patch also fixes a corner case where a bad substitution error
occurs within arithmetic expansion.

Reported-by: Denys Vlasenko <vda.linux@googlemail.com>
Fixes: ab1cecb4 (" parser: Add syntax stack for recursive...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

dash -c 'echo ${}' should print "Bad subtitution" but instead
fails with "Syntax error: Missing '}'".  This is caused by us
reading an extra character beyond the right brace.  This patch
fixes it so that this construct only fails during expansion rather
than during parsing.

Fixes: 3df3edd1 ("[PARSER] Report substition errors at...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Op 27-03-18 om 20:23 schreef Larry Hynes:
> Funny, I did wonder if it might be a contraction, but I did find
> it odd that it's not mentioned or explained. I'll leave it be, if
> you all are happy enough to keep it 'as is', or can resubmit if you
> think it's warranted.

I think the simple fact that it came up here is evidence that this is
too jargony for a manual. Patch attached.

- M.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

On Mon, Mar 26, 2018 at 07:25:20PM +0200, Martijn Dekker wrote:
> Op 26-03-18 om 17:38 schreef Harald van Dijk:
> > And not by dash 0.5.4. Like I wrote, dash 0.5.5 had some bugs that were
> > fixed in 0.5.6, which mostly restored the behaviour to match <0.5.5.
>
> Ah, sorry. dash 0.5.4 and earlier don't compile on my system, so they
> are not included in my conveniently accessible arsenal of test shells.
>
> > As for my patches, that was by accident and doesn't work reliably. When
> > the shell sees no metacharacters, pathname expansion is bypassed, and
> > backslash isn't considered a metacharacter. Which got me to my original
> > example of /de\v: there are no metacharacters in there, so the shell
> > doesn't look to see if it matches anything. Which seems highly
> > desirable: the shell shouldn't need to hit the file system for words not
> > containing metacharacters. The only way then to get consistent behaviour
> > is if the backslash is taken as quoted, so I'm not tempted to argue for
> > the behaviour you're hoping for, sorry. :)

Here is a better example:

	a="/*/\nullx" b="/*/\null"; printf "%s\n" $a $b

dash currently prints

	/*/\nullx
	/*/\null

bash prints

	/*/\nullx
	/dev/null

You may argue the bash behaviour is inconsistent but it actually
makes sense.  What happens is that quote removal only applies to
the original token as seen by the shell.  It is never applied to
the result of parameter expansion.

Now you may ask why on earth does the second line say "/dev/null"
instead of "/dev/\null".  Well that's because it is not the quote
removal step that removed the backslash, but the pathname expansion.

The fact that the /de\v does not become /dev even though it exists
is just the result of the optimisation to avoid unnecessarily
calling stat(2).  I have checked POSIX and I don't see anything
that forbids this behaviour.

So going back to dash yes I think we should adopt the bash behaviour
for pathname expansion and keep the existing case semantics.

This patch does exactly that.  Note that this patch does not work
unless you have already applied

	https://patchwork.kernel.org/patch/10306507/



because otherwise the optimisation mentioned above does not get
detected correctly and we will end up doing quote removal twice.

This patch also updates expmeta to handle naked backslashes at
the end of the pattern which is now possible.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

I've attached a patch which adds the subdir-objects option to AM_INIT_AUTOMAKE.

For a while now when I've compiled dash I received a warning from
automake that there are source files in a subdirectory but that the
subdir-objects automake option was not supplied. I've just been adding
it myself, but I finally got around to submitting a patch. The code
still compiles for now (i'm using automake 1.15.1), but warning text
is rarely nice to see and, if the warning text is to be believed, then
the warning will eventually become an error.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

When evalcommand invokes a command that modifies parsefile and
then bails out without popping the file, we need to ensure the
input file is restored so that the shell can continue to execute.

Reported-by: Martijn Dekker <martijn@inlv.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Currently dash does not reap dead children after built-in commands
or functions.  This means that if you construct a loop consisting
of solely built-in commands and functions, then zombies can hang
around indefinitely.

This patch fixes this by reaping when necessary after each built-in
command and function.

Reported-by: Denys Vlasenko <vda.linux@googlemail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

The noclobber code has a typo in it that causes it to fail.  This
patch fixes it.

Reported-by: Denys Vlasenko <vda.linux@googlemail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

It's been a while since we disabled glob(3) support by default.
It appears to be working now, however, we have to change our
code to detect the no-match case correctly.

In particular, we need to test for GLOB_NOMAGIC | GLOB_NOCHECK
instead of GLOB_MAGCHAR.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

The native version of expandmeta allocates a buffer that may be
overrun for two reasons.  First of all the size is 1 byte too small
but this is normally hidden because the minimum size is rounded
up to 2048 bytes.  Secondly, if the directory level is deep enough,
any buffer can be overrun.

This patch fixes both problems by calling realloc when necessary.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Currently echocmd uses print_escape_str to do everything apart
from printing the spaces/newlines separating its arguments.  This
patch moves the actual printing into print_escape_str as well
using the format parameter.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

The commit d6c0e1e2 ("[BUILTIN]
Handle embedded NULs correctly in printf") caused a performance
regression in the echo built-in because every echo call now goes
through the printf %b slow path where the string is always printed
twice to ensure the space padding is correct in the presence of
NUL characters.  In fact this regression applies to printf %b as
well.

This is easily fixed by making printf %b take the fast path when
no precision/field width modifiers are present.

This patch also changes the second strchurnul call to strspn which
generates slightly better code.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Harald van Dijk <harald@gigawatt.nl> wrote:
> On 22/03/2018 22:38, Martijn Dekker wrote:
>> Op 22-03-18 om 20:28 schreef Harald van Dijk:
>>> On 22/03/2018 03:40, Martijn Dekker wrote:
>>>> This patch fixes the bug that, given no positional parameters, unquoted
>>>> $@ and $* incorrectly generate one empty field (they should generate no
>>>> fields). Apparently that was a side effect of the above.
>>>
>>> This seems weird though. If you want to remove the recording of empty
>>> regions because they are pointless, then how does removing them fix a
>>> bug? Doesn't this show that empty regions do have an effect? Perhaps
>>> they're not supposed to have any effect, perhaps it's a specific
>>> combination of empty regions and something else that triggers some bug,
>>> and perhaps that combination can no longer occur with your patch.
>>
>> The latter is my guess, but I haven't had time to investigate it.
>
> Looking into it again:
>
> When IFS is set to an empty string, sepc is set to '\0' in varvalue().
> This then causes *quotedp to be set to true, meaning evalvar()'s quoted
> variable is turned on. quoted is then passed to recordregion() as the
> nulonly parameter.
>
> ifsp->nulonly has a bigger effect than merely selecting whether to use
> $IFS or whether to only split on null bytes: in ifsbreakup(), nulonly
> also causes string termination to be suppressed. That's correct: that
> special treatment is required to preserve empty fields in "$@"
> expansion. But it should *only* be used when $@ is quoted: ifsbreakup()
> takes nulonly from the last IFS region, even if it's empty, so having an
> additional zero-length region with nulonly enabled causes confusion.
>
> Passing quoted by value to varvalue() and not attempting to modify it
> should therefore, and in my quick testing does, also work to fix the
> original $@ bug.

You're right.  The proper fix to this is to ensure that nulonly
is not set in varvalue for $*.  It should only be set for $@ when
it's inside double quotes.

In fact there is another bug while we're playing with $@/$*.
When IFS is set to a non-whitespace character such as :, $*
outside quotes won't remove empty fields as it should.

This patch fixes both problems.

Reported-by: Martijn Dekker <martijn@inlv.org>
Suggested-by: Harald van Dijk <harald@gigawatt.nl>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

On Fri, Mar 16, 2018 at 11:27:22AM +0800, Herbert Xu wrote:
> On Thu, Mar 15, 2018 at 10:49:15PM +0100, Harald van Dijk wrote:
> >
> > Okay, it can be trivially modified to something that does work in other
> > shells (even if it were actually executed), but gets rejected at parse time
> > by dash:
> >
> >   if false; then
> >     : ${$+
> >   }
> >   fi
>
> That's just a bug in dash's parser with ${} in general, because
> it bombs out without the if clause too:
>
> 	: ${$+
> 	}

This patch fixes the parsing of newlines with parameter substitution.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

On Sun, Mar 04, 2018 at 12:44:59PM +0100, Harald van Dijk wrote:
>
> command:      set -- a ""; space=" "; printf "<%s>" "$@"$space
> bash:         <a><>
> dash 0.5.8:   <a>< >
> dash 0.5.9.1: <a>< >
> dash patched: <a><>

This is actually composed of two bugs.  First of all our tracking
of quotemark is wrong so anything after "$@" becomes quoted.  Once
we fix that then the problem is that the first space character
after "$@" is not recognised as an IFS.

This patch fixes both.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This reverts commit 7bb41325.

The commit breaks printf with more than argument.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Currently using backquotes in a here-document EOF mark is broken
because dash tries to do command substitution on it.  This patch
fixes it by checking whether we're looking for an EOF mark during
tokenisation.

Reported-by: Harald van Dijk <harald@gigawatt.nl>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Here's a .gitignore file for the convenience of casual git users.

- M.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

The script

	x=*
	cat <<- EOF
		${x#'*'}
	EOF

prints * instead of nothing as it should.  The problem is that
when we're in sqsyntax context in a here-document, we won't add
CTLESC as we should.  This patch fixes it:

Reported-by: Harald van Dijk <harald@gigawatt.nl>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Without a stack of syntaxes we cannot correctly these two cases
together:

        "${a#'$$'}"
        "${a#"${b-'$$'}"}"

A recursive parser also helps in some other corner cases such
as nested arithmetic expansion with paratheses.

This patch adds a syntax stack allocated from the stack using
alloca.  As a side-effect this allows us to remove the naked
backslashes for patterns within double-quotes, which means that
EXP_QPAT also has to go.

This patch also fixes removes any backslashes that precede right
braces when they are present within a parameter expansion context,
and backslashes that precede double quotes within inner double
quotes inside a parameter expansion in a here-document context.

The idea of a recursive parser is based on a patch by Harald van
Dijk.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

dash has a pgetc_eatbnl function in parser.c which skips any
backslash-newline combinations. It's not used everywhere it could be.
There is also some duplicated backslash-newline handling elsewhere in
parser.c. Replace most of the calls to pgetc() with calls to
pgetc_eatbnl() and remove the duplicated backslash-newline handling.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Op 07-03-18 om 15:46 schreef Martijn Dekker:
> Op 06-03-18 om 09:19 schreef Herbert Xu:
>> On Thu, Jun 22, 2017 at 10:30:02AM +0200, Petr Skočík wrote:
>>> would you be willing to pull something like this?
> [...]
>>> I could use greater resolution in `test -nt` / `test -ot`, and st_mtim
>>> field is standardized under POSIX.1-2008 (or so stat(2) says).
>>
>> Sure.  But your patch is corrupted.
>
> Fixed patch attached.
>
> But I wouldn't apply it as is. My system does not have st_mtim. So I
> think it needs a configure test and a fallback to the old method.

Here's an attempt to make that happen. See attached.

- M.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Op 07-03-18 om 06:26 schreef Herbert Xu:
> Martijn Dekker <martijn@inlv.org> wrote:
>>
>>> Since base is always a constant 0 or a constant 10, never a
>>> user-provided value, the only error that strtoimax will ever report on
>>> glibc systems is ERANGE. Checking only ERANGE therefore preserves the
>>> glibc behaviour, and allows the exact same set of errors to be detected
>>> on non-glibc systems.
>>
>> That makes sense, thanks.
>
> Could you resend your patch with this change please?

OK, see below.

- M.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Op 29-03-17 om 20:02 schreef Martijn Dekker:
> Bug: if either the 'nolog' or the 'debug' option is set, trying to
> expand "$-" silently aborts parsing of an entire argument.
>
> $ dash -o nolog -c 'set -fuC; echo "|$- are the options|"; \
> 	set +o nolog; echo "|$- are the options|"'
> |
> |uCf are the options|
> $ dash -o debug -c 'set -fuC; echo "|$- are the options|"; \
> 	set +o debug; echo "|$- are the options|"'
> |
> |uCf are the options|

This turned out to be easy to fix. The routine producing the "$-"
expansion failed to skip options for which there is no option letter,
but only a long-form name. In dash, 'nolog' and 'debug' are currently
the only two such options. Patch below.

- Martijn

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

musl libc defines the optreset BSD extension only in getopt.h. This
fixes the following build failure:

histedit.c: In function 'histcmd':
histedit.c:220:2: error: 'optreset' undeclared (first use in this function)
  optreset = 1; optind = 1; /* initialize getopt */
  ^~~~~~~~

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

[ Ugh; forgot to attach patch - apologies, I need more coffee ]

Dear all,

Attached is a trivial patch that removes the assumption that fnmatch.h
is available - the configure script already checks for fnmatch(3) and
supplies its own implementation if necessary, but fnmatch.h is always
included.

Let me know what you think.

Regards,
Rink

Do not assume we can include fnmatch.h

Signed-off-by: Rink Springer <rink@rink.nu>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

On 27/06/17 16:29, Zando Fardones wrote:
> Hello,
>
> I think I've found a bug when using the here-document redirection in
> an interactive shell. What basically happens is that you can't see the
> command output if you set the "vi" or "emacs" options.

That's not quite what happens: the here-document contents got lost, so
there is no command output to see. Nice find.

The problem is that getprompt() is implicitly called by el_gets(). This
messes with the memory used by the parser to store the here-document's
contents. In the non-emacs/vi case, the prompt is explicitly written by
setprompt(), which wraps the getprompt() call in a
pushstackmark()/popstackmark() pair to restore the state so that parsing
can continue. But when getprompt() is called by el_gets(), it knows
nothing about this.

The whole call to el_gets() can be surrounded by another
pushstackmark()/popstackmark() pair to solve the problem, as attached.

Cheers,
Harald van Dijk

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Jilles Tjoelker <jilles@stack.nl> wrote:
> On Sat, Jun 17, 2017 at 03:53:26PM +0100, Larry Hynes wrote:
>> src/dash.1, under Command Line Editing, states:

>>       It's similar to vi: typing <ESC> will throw you into command
>>       VI command mode.

>> - There appears to be no need for both occurrences of 'command'
>> - I can't see a reason for VI to be capitalised
>> - 'will throw you into' seems a little... enthusiastic

>> Following diff changes it to

>>       It's similar to vi: typing <ESC> enters vi command mode.

>> diff --git a/src/dash.1 b/src/dash.1
>> index 8b8026d..f35d89d 100644
>> --- a/src/dash.1
>> +++ b/src/dash.1
>> @@ -2232,7 +2232,7 @@ enabled, sh can be switched between insert mode and command mode.
>>  The editor is not described in full here, but will be in a later document.
>>  It's similar to vi: typing
>>  .Aq ESC
>> -will throw you into command VI command mode.
>> +enters vi command mode.
>>  Hitting
>>  .Aq return
>>  while in command mode will pass the line to the shell.

> I agree. If you're changing things here anyway, I suggest getting rid of
> the contraction as well (changing It's to It is). The fairly formal
> style of man pages avoids contractions, just like it avoids "you".

> The reference to the "later document" can probably be removed as well,
> since said document does not exist yet after many years.

Hi

Revised diff, below, expands the contraction, deletes reference to
'later document' and changes 'place' to 'places' in the following:

	The command ‘set -o vi’ enables vi-mode editing and place sh
	into vi insert mode.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Hi,

On 26/05/17 09:04, Youfu Zhang wrote:
> $ PATH=/extra/path:/usr/sbin:/usr/bin:/sbin:/bin \
>> sh -xc 'command -V ls; command -V ls; command -Vp ls; command -vp ls'
> + command -V ls
> ls is /bin/ls
> + command -V ls
> ls is a tracked alias for /bin/ls
> + command -Vp ls
> ls is a tracked alias for (null)
> + command -vp ls
> Segmentation fault (core dumped)
>
> describe_command should respect `path' argument. Looking up in the hash table
> may gives incorrect index in entry.u.index and finally causes incorrect output
> or SIGSEGV.

True, but only when a path is passed in. If the default path is used,
looking up in the hash table is correct, and printing tracked aliases is
intentional.

If it's desirable to drop that feature, then it should be dropped
completely, code shouldn't be left in that can no longer be used. But
it's possible to keep it working: how about this instead?

Signed-off-by: Harald van Dijk <harald@gigawatt.nl>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This variable does not contain "sigs" (plural).
It contains either 0 or (one) signal number of a pending signal.

For someone unfamiliar with this code, "pendingsigs" name is confusing -
it hints at being an array or bit mask of pending singnals.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: dash@vger.kernel.org
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

When there is an unmatched left square bracket in patterns, pmatch
will behave strangely and exhibit undefined behaviour.  This patch
(based on Harld van Dijk's original) fixes this by treating it as
a literal left square bracket.

Reported-by: Olof Johansson <olof@ethup.se>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

The commit 7a784244 ("[BUILTIN]
Simplify echo command") broke echo -n by making it always terminate
after printing the first argument.

This patch fixes this by only terminating when we have reached
the end of the arguments.

Fixes: 7a784244 ("[BUILTIN] Simplify echo command")
Reported-by: Luigi Tarenga <luigi.tarenga@gmail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

The read built-in does not handle trailing IFS white spaces in
the right way, when there are more fields than variables.  Part
of the problem is that this case is handled outside of ifsbreakup.

Harald van Dijk wrote a patch to fix this by moving the magic
into ifsbreakup itself.

This patch further reorganises the ifsbreakup loop by having only
one loop over the whole string.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Tested-by: Harald van Dijk <harald@gigawatt.nl>

The exit status is currently clobbered too early for case statements
and loops.  This patch fixes it by making the eval functions return
the current exit status and setting them in one place -- evaltree.

Harald van Dijk pointed out a number of bugs in the original patch.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

When looking for a job using a string descriptor, e.g.

	fg %man

the relevant loop in src/jobs.c only ever exits to the err label. With
this patch, when the end condition is reached, we check whether a job
was found, and if so, set things up to exit correctly via gotit.
Multiple matches are already caught using the test in the match block.

Signed-off-by: Stephen Kitt <steve@sk2.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Jonathan Perkin submitted a patch to fix the behaviour of trap
when the first argument is an integer.  Currently it is treated
as a command while POSIX requires it to be treated as a signal.

This patch is based on his idea but instead of adding an extra
argument to decode_signal I have added a new decode_signum helper.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

On 17/11/2015 03:18, Gioele Barabucci wrote:
> Hello,
>
> a bug has been filed in the Debian BTS about dash not resetting the exit
> status after sourcing an empty file with the dot command. [1]
>
> The following test echoes "OK" with bash and "fail" with dash
>
>      #!/bin/sh
>
>      echo > ./empty
>      false
>
>      . ./empty && echo "OK" || echo "fail"
 >
> A similar bug in dash has been discussed and addressed in 2011 [2], but
> it looks like the solution has been only partial.
>
> The version of dash I tested is the current git master branch, commit
> 2e584225.
>
> [1] https://bugs.debian.org/777262
> [2] http://article.gmane.org/gmane.comp.shells.dash/531



The bug described there was about empty files. While the fix has been 
applied and does make dash handle empty files properly, your test 
doesn't use an empty file, it uses a file containing a single blank 
line. Unfortunately, the single blank line gets parsed by dash as a null 
command, null commands don't (and shouldn't) reset the exit status, and 
the fix you link to doesn't handle this because it sees a command has 
been executed and saves the exit status after executing that command as 
the exit status to be used by ".".

I think the easiest way to fix this is to prevent null commands from 
affecting status in cmdloop, as attached.

An alternative could be to change the outer if condition to exclude
n == NULL, but I didn't do that because the change of job_warning and 
clearing of numeof make sense to me even for null commands. Besides, 
when debug tracing is enabled, null commands have a visible effect that 
should remain.

Note that this fixes the problem with . but the same problem can be 
present in other locations. For example,

     false
     eval "
     " && echo OK || echo Fail

used to print Fail, and needed the same modification in the evalstring 
function to make that print OK (included in the attached patch). There 
may be other similar bugs lurking.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>