Re: [PATCH] parser: Fix regression on ${#10} expansion
On Wed, Feb 10, 2021 at 07:30:17PM +0400, Vladimir N. Oleynik wrote: > > But I do not show this patch in > https://git.kernel.org/pub/scm/utils/dash/dash.git/ > Its ok? It's still in the review process: https://patchwork.kernel.org/project/dash/list/ Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] parser: Fix regression on ${#10} expansion
Thanks for the patch!
---8<---
Vladimir N. Oleynik wrote:
>
> dash have bug for ${#10} and etc: ignores 0... in name (without errors :)
>
> $ foo() { echo "length 10-th arg '${10}' is ${#10}"; }
> $ foo a1 a2 a3 a4 a5 a6 a7 a8 a9 a10 a11
> length 10-th arg 'a10' is 2
>
> But need:
> length 10-th arg 'a10' is 3
>
> Micro patch attached
>
> --- parser.c~ 2021-02-04 00:51:34.0 +0400
> +++ parser.c2021-02-05 12:42:43.616635640 +0400
> @@ -1274,7 +1274,7 @@
>do {
>STPUTC(c, out);
>c = pgetc_eatbnl();
> - } while (!subtype && is_digit(c));
> + } while ((!subtype || subtype == VSLENGTH) &&
> is_digit(c));
>} else if (c != '}') {
>int cc = c;
I would rather test against VSNORMAL.
Fixes: 7710a926b321 ("parser: Only accept single-digit parameter...")
Reported-by: Vladimir N. Oleynik
Signed-off-by: Herbert Xu
diff --git a/src/parser.c b/src/parser.c
index 3c80d17..834d2e3 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1252,7 +1252,7 @@ varname:
do {
STPUTC(c, out);
c = pgetc_eatbnl();
- } while (!subtype && is_digit(c));
+ } while (subtype != VSNORMAL && is_digit(c));
} else if (c != '}') {
int cc = c;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2] Cache the expanded prompt for editline
On Thu, Jan 14, 2021 at 10:43:21AM -0500, June Bug wrote: > > It seems unlikely given the lack of consistency of when libedit > calls the prompt callback. I only noticed it was happening when I > patched in support for EL_RPROMPT (right-aligned prompts), which > when active causes libedit to call the prompt callback on *every* > keypress. It seemed not to happen with only a regular prompt and > it took some experimenting to notice typing backspaces causes it. > The behaviour is completely undocumented in libedit. > > Anecdotally, typing at the prompt becomes painful when it is expanded > over and over and does anything non-trivial, even just a command > substitution that runs no external commands. OK, in that case I think it's best to fix this in libedit instead. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Fail if building --with-libedit and can't find libedit
C. McEnroe wrote: > Previously, configure --with-libedit would only fail in the case where > libedit is available but its header file histedit.h is not. > --- > configure.ac | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2] Cache the expanded prompt for editline
C. McEnroe wrote: > Previously, the prompt would be expanded every time editline called the > getprompt callback. I think the code may have been written assuming that > editline only calls getprompt once per prompt, but it may actually call > it many times, for instance every time you type backspace. This results > not only in slower editing from expanding complex prompts repeatedly, it > also consumes more and more stack memory each time getprompt is called. > This can be seen by setting PS1 to some command substitution, typing > many characters at the prompt, then holding backspace and observing > memory usage. Thankfully all this stack memory is freed between prompts > by the stackmark calls around el_gets. > > This change causes prompt expansion to always happen in the setprompt > call, as it would when editline is disabled, and a cached copy of the > prompt is saved for getprompt to return every time editline calls it. > Since getprompt is no longer doing expansion, the stackmark calls > surrounding el_gets can be removed. > --- > > v2: Missed adding breaks when replacing returns with assignments. What if someone actually wanted the prompt to change? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] jobs: Always reset SIGINT/SIGQUIT handlers
On Fri, Jan 08, 2021 at 08:55:41PM +, Harald van Dijk wrote:
> On 18/05/2018 19:39, Herbert Xu wrote:
> > This patch adds basic vfork support for the case of a simple command.
> > ... @@ -879,17 +892,30 @@ forkchild(struct job *jp, union node *n, int
> > mode)
> > }
> > }
> > if (!oldlvl && iflag) {
> > - setsignal(SIGINT);
> > - setsignal(SIGQUIT);
> > + if (mode != FORK_BG) {
> > + setsignal(SIGINT);
> > + setsignal(SIGQUIT);
> > + }
> > setsignal(SIGTERM);
> > }
> > +
> > + if (lvforked)
> > + return;
> > +
> > for (jp = curjob; jp; jp = jp->prev_job)
> > freejob(jp);
> > }
>
> This leaves SIGQUIT ignored in background jobs in interactive shells.
>
> ENV= dash -ic 'dash -c "kill -QUIT \$\$; echo huh" & wait'
>
> As of dash 0.5.11, this prints "huh". Before, the subprocess process killed
> itself before it could print anything. Other shells do not leave SIGQUIT
> ignored.
>
> (In a few other shells, this also prints "huh", but in those other shells,
> that is because the inner shell chooses to ignore SIGQUIT, not because the
> outer shell leaves it ignored.)
Thanks for catching this. I have no idea how that got in there
and it makes no sense whatsoever. This patch removes the if
conditional.
Fixes: e94a964e7dd0 ("eval: Add vfork support")
Reported-by: Harald van Dijk
Signed-off-by: Herbert Xu
diff --git a/src/jobs.c b/src/jobs.c
index 516786f..08c4f13 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -892,10 +892,8 @@ static void forkchild(struct job *jp, union node *n, int
mode)
}
}
if (!oldlvl && iflag) {
- if (mode != FORK_BG) {
- setsignal(SIGINT);
- setsignal(SIGQUIT);
- }
+ setsignal(SIGINT);
+ setsignal(SIGQUIT);
setsignal(SIGTERM);
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: dash 0.5.11.2: somehow falsely waits
Steffen Nurpmeso wrote: > > |Can you please retest with 6359d7aa739b9f02f622805f4dbddeaf0ae61981? > > It works fine with the patch from the other message (repository at > kernel.org does not know about the commit). Thanks for confirming. It's strange that you can't find the commit though because it appears to be there: https://git.kernel.org/pub/scm/utils/dash/dash.git/commit/?id=6359d7aa739b9f02f622805f4dbddeaf0ae61981 Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: dash 0.5.11.2: somehow falsely waits
Steffen Nurpmeso wrote: > Hello. > > There is another issue with at least dash (the others not yet > tested). I cloned the git repo and it is still present at > 6ba88b3ed28fb4b52f20b730194c4ad3d8aad037. Can you please retest with 6359d7aa739b9f02f622805f4dbddeaf0ae61981? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: dash 0.5.11.2, busybox sh 1.32.0, FreeBSD 12.2 sh: spring TTOU but should not i think
Jilles Tjoelker wrote: > > This seems a good approach, but certain writes to the terminal may need > the same treatment, for example the error message when fork fails for > the second and further elements of a pipeline. This also makes me wonder > why SIGTTOU is ignored at all by default. I think the approach of blocking all signals should be able to resolve this too if anyone cares enough about this case. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] jobs: Block signals during tcsetpgrp
Harald van Dijk wrote:
> On 19/12/2020 22:21, Steffen Nurpmeso wrote:
>> Steffen Nurpmeso wrote in
>> <20201219172838.1b-wb%stef...@sdaoden.eu>:
>> |Long story short, after falsely accusing BSD make of not working
>>
>> After dinner i shortened it a bit more, and attach it again, ok?
>> It is terrible, but now less redundant than before.
>> Sorry for being so terse, that problem crosses my head for about
>> a week, and i was totally mislead and if you bang your head
>> against the wall so many hours bugs or misbehaviours in a handful
>> of other programs is not the expected outcome.
>
> I think a minimal test case is simply
>
> all:
> $(SHELL) -c 'trap "echo TTOU" TTOU; set -m; echo all good'
>
> unless I accidentally oversimplified.
>
> The SIGTTOU is caused by setjobctl's xtcsetpgrp(fd, pgrp) call to make
> its newly started process group the foreground process group when job
> control is enabled, where xtcsetpgrp is a wrapper for tcsetpgrp. (That's
> in dash, the other variants may have some small differences.) tcsetpgrp
> has this little bit in its specification:
>
>Attempts to use tcsetpgrp() from a process which is a member of
>a background process group on a fildes associated with its con‐
>trolling terminal shall cause the process group to be sent a
>SIGTTOU signal. If the calling thread is blocking SIGTTOU sig‐
>nals or the process is ignoring SIGTTOU signals, the process
>shall be allowed to perform the operation, and no signal is
>sent.
>
> Ordinarily, when job control is enabled, SIGTTOU is ignored. However,
> when a trap action is specified for SIGTTOU, the signal is not ignored,
> and there is no blocking in place either, so the tcsetpgrp() call is not
> allowed.
>
> The lowest impact change to make here, the one that otherwise preserves
> the existing shell behaviour, is to block signals before calling
> tcsetpgrp and unblocking them afterwards. This ensures SIGTTOU does not
> get raised here, but also ensures that if SIGTTOU is sent to the shell
> for another reason, there is no window where it gets silently ignored.
>
> Another way to fix this is by not trying to make the shell start a new
> process group, or at least not make it the foreground process group.
> Most other shells appear to not try to do this.
This patch implements the blocking of SIGTTOU (and everything else)
while we call tcsetpgrp.
Reported-by: Steffen Nurpmeso
Signed-off-by: Herbert Xu
diff --git a/src/jobs.c b/src/jobs.c
index 516786f..809f37c 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -1512,7 +1512,13 @@ showpipe(struct job *jp, struct output *out)
STATIC void
xtcsetpgrp(int fd, pid_t pgrp)
{
- if (tcsetpgrp(fd, pgrp))
+ int err;
+
+ sigblockall(NULL);
+ err = tcsetpgrp(fd, pgrp);
+ sigclearmask();
+
+ if (err)
sh_error("Cannot set tty process group (%s)", strerror(errno));
}
#endif
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: set -I is not required by standard, and does not match bash
On Mon, Jan 04, 2021 at 01:22:30PM +0100, Denys Vlasenko wrote: > Hello, > > In dash, set -I is a short-option alias to set -o ignoreeof. > > However, bash does not have such alias, it has an undocumented > set -I which switches off "invisible variables" > (I don't know what that is). > > Standards do not mention any -I option. > > I propose, in the interests of keeping things less disparate, > to remove set -I support from dash: make "ignoreeof" to be > only an -o long option. This option goes all the way back to BSD. Unless they have all removed this option I'm not going to change it. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] input: Clear unget on RESET
On Sat, Dec 19, 2020 at 02:23:44PM +0100, Denys Vlasenko wrote:
> Current git:
>
> $ ;l
> dash: 1: Syntax error: ";" unexpected
> $ s
> COPYINGChangeLog.OMakefile.am aclocal.m4 autom4te.cache
> config.h config.log configure dash
> dollar_altvalue1.tests missing stamp-h1
> ChangeLog MakefileMakefile.in autogen.sh compile
> config.h.in config.status configure.ac depcomp install-sh
> src trace
This patch fixes it by clearing ungetc on RESET.
Fixes: 17db43b58415 ("input: Allow two consecutive calls to pungetc")
Reported-by: Denys Vlasenko
Signed-off-by: Herbert Xu
diff --git a/src/input.c b/src/input.c
index 4987732..d7c101b 100644
--- a/src/input.c
+++ b/src/input.c
@@ -87,6 +87,7 @@ INIT {
RESET {
/* clear input buffer */
basepf.lleft = basepf.nleft = 0;
+ basepf.unget = 0;
popallfiles();
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Fwd: Bug#974900: dash removes trailing slash from script arguments
On Wed, Dec 09, 2020 at 01:27:17PM +0100, Aurelien Jarno wrote: > > Can you please describe more precisely what is the problem with glob(3)? Create a regular file called "foo", then call glob(3) with the pattern "foo\/". This returns a single match with the string "foo". This should return no match. If you change the pattern to "foo/", then it also matches but returns with the string "foo/" as expected. The only flag we pass to glob(3) is GLOB_NOMAGIC. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Bug#976865: Fwd: Bug#974900: dash removes trailing slash from script arguments
On Thu, Dec 10, 2020 at 08:58:37AM +0100, Aurelien Jarno wrote: > > That's the dash symptoms. glob(3) takes a pattern and just returns the > paths matching the pattern, as they are named on the filesystem. That > said, the option GLOB_MARK can return a trailing slash for all matched > path that are a directory. Yes but it's really a bug in glob(3). It should really return a no-match for the case in question, rather than matching and then returning a filename without the slash. IOW the pattern "foo\/" should not match a regular file foo. Note that the problem doesn't occur for "foo/". Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Bug#976865: Fwd: Bug#974900: dash removes trailing slash from script arguments
Aurelien Jarno wrote: > > Can you please describe more precisely what is the problem with glob(3)? It's stripping trailing slashes from the pattern, even when the name in question is a regular file. https://patchwork.kernel.org/project/dash/patch/20201116025222.ga28...@gondor.apana.org.au/ Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Thu, Dec 03, 2020 at 01:27:51PM +0100, Michael Biebl wrote: > > Hm, so I applied this change and switched backed to dash, but now I get > > autopkgtest systemd --test-name=timedated -s -- lxc -s autopkgtest-sid > ... > disable NTP > enable NTP > Terminated > autopkgtest [13:21:42]: test timedated: ---] > autopkgtest [13:21:42]: test timedated: - - - - - - - - - - results - - - - > - - - - - - > timedatedFAIL non-zero exit status 143 > > > Running with sh -x > ... > + kill 825 > + wait 825 > Terminated > > > Herbert, any idea what going wrong there? > (systemd 247.1-2 and dash 0.5.11+git20200708+dd9ef66-2) Sorry, my bad. wait(1) with no arguments ignores the error status of the child and always return zero. wait(1) specifically on a child obviously returns the error status of that child. Since the child was killed, we need to ignore that, so this works for me: wait $MONPID 2> /dev/null || : Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] jobs: Only block in waitcmd on first run
This patch ensures that waitcmd never blocks unless there are
outstanding jobs. This could otherwise trigger a hang if children
were created prior to the shell coming into existence, or if
there are backgrounded children of other kinds (e.g., a here-
document).
Fixes: 6c691b3e5099 ("jobs: Only clear gotsigchld when waiting...")
Reported-by: Michael Biebl
Signed-off-by: Herbert Xu
diff --git a/src/jobs.c b/src/jobs.c
index 3417633..516786f 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -81,6 +81,7 @@
#define DOWAIT_NONBLOCK 0
#define DOWAIT_BLOCK 1
#define DOWAIT_WAITCMD 2
+#define DOWAIT_WAITCMD_ALL 4
/* array of jobs */
static struct job *jobtab;
@@ -615,7 +616,7 @@ waitcmd(int argc, char **argv)
jp->waited = 1;
jp = jp->prev_job;
}
- if (!dowait(DOWAIT_WAITCMD, 0))
+ if (!dowait(DOWAIT_WAITCMD_ALL, 0))
goto sigout;
}
}
@@ -1138,6 +1139,7 @@ static int dowait(int block, struct job *jp)
pid = waitone(block, jp);
rpid &= !!pid;
+ block &= ~DOWAIT_WAITCMD_ALL;
if (!pid || (jp && jp->state != JOBRUNNING))
block = DOWAIT_NONBLOCK;
} while (pid >= 0);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Wed, Dec 02, 2020 at 12:21:52AM +0100, Michael Biebl wrote: > > I'll update the timedated test accordingly. > Incidentally we already have > https://salsa.debian.org/systemd-team/systemd/-/merge_requests/110 Thanks. I will make dash not wait for existing children as this is indeed a change in behaviour which is undesirable. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Tue, Dec 01, 2020 at 10:59:20AM +, Harald van Dijk wrote: > > I do not appreciate your false accusation that I did not read your follow-up > email. You suggested that as an alternative, without retracting your claim > that this is a problem in bash. Well I'm not accusing you of anything, I was just trying to make sure you are aware of my complete message. In any case, I never said that this is a bug in bash, merely that bash is involved in creating a situation where dash started out with children in the background. If anything this could just be an artefact of the bash script rather than bash itself. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Tue, Dec 01, 2020 at 10:55:06AM +, Harald van Dijk wrote: > > You wrote: "So the problem is really in the parent of this shell, which > appears to be bash:" You should read my follow-up email too that suggested changing the systemd script. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Tue, Dec 01, 2020 at 10:50:19AM +, Harald van Dijk wrote: > > This used to exit immediately, leaving the sleep process running in the > background without waiting for it. On the dash that's currently provided by > Ubuntu, based on 0.5.10.2, it still behaves that way. With current dash from > Git, it does not. This is clearly a change in behaviour in dash not in > response to any bug (real or not) in bash. I'm not suggesting it's a bug in bash. If anything it's a bug in the script. You should never do a naked wait unless you are sure that there are no other children around that you don't know of. In the original bug, the proper solution is to wait on the PID that the script just sent a kill signal to. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Tue, Dec 01, 2020 at 10:14:04AM +, Harald van Dijk wrote: > > POSIX says: > > "If the wait utility is invoked with no operands, it shall wait until > all process IDs known to the invoking shell have terminated and exit with a > zero exit status." > > I would say that child processes that were created before dash was started > do not have process IDs known to dash. Well I disagree and the fact that the original ksh does the same thing is an important fact. > > bash -c set -e; export USER=`id -nu`; . /etc/profile >/dev/null 2>&1 || > > true; . ~/.profile >/dev/null 2>&1 || true; > > buildtree="/tmp/autopkgtest-lxc.is4n6xxr/downtmp/build.f2G/real-tree"; > > mkdir -p -m 1777 -- > > "/tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-artifacts"; export > > AUTOPKGTEST_ARTIFACTS="/tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-artifacts"; > > export ADT_ARTIFACTS="$AUTOPKGTEST_ARTIFACTS"; mkdir -p -m 755 > > "/tmp/autopkgtest-lxc.is4n6xxr/downtmp/autopkgtest_tmp"; export > > AUTOPKGTEST_TMP="/tmp/autopkgtest-lxc.is4n6xxr/downtmp/autopkgtest_tmp"; > > export ADTTMP="$AUTOPKGTEST_TMP"; export DEBIAN_FRONTEND=noninteractive; > > export LANG=C.UTF-8; export DEB_BUILD_OPTIONS=parallel=2; unset LANGUAGE > > LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES LC_PAPER > > LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT LC_IDENTIFICATION > > LC_ALL;rm -f /tmp/autopkgtest_script_pid; set -C; echo $$ > > > /tmp/autopkgtest_script_pid; set +C; trap "rm -f > > /tmp/autopkgtest_script_pid" EXIT INT QUIT PIPE; cd "$buildtree"; export > > AUTOPKGTEST_NORMAL_USER=; export ADT_NORMAL_USER=; chmod +x > > /tmp/autopkgtest-lxc.is4n6xxr/downtmp/build.f2G/real-tree/debian/tests/timedated; > > touch /tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-stdout > > /tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-stderr; > > /tmp/autopkgtest-lxc.is4n6xxr/downtmp/build.f2G/real-tree/debian/tests/timedated > > 2> >(tee -a /tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-stderr >&2) > > > >(tee -a /tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-stdout); > > > > For some reason this is causing the final two tee's to be created > > as children of debian/tests/timedated rather than the bash shell. > > This is because of the same optimisation that dash also has, where it tries > to avoid creating a subshell for the last command in a list when it can just > exec() without a fork() instead. A minimal example without an explicit exec > is > > bash -c 'dash -c ": & wait" <(sleep 1d)' I'm not sure about that because bash itself is still hanging around, if it were really the -c optimisation then bash should not appear in the ps output at all. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Tue, Dec 01, 2020 at 05:06:18PM +1100, Herbert Xu wrote: > > For some reason this is causing the final two tee's to be created > as children of debian/tests/timedated rather than the bash shell. An alternative to changing the parent is of course to do wait $MONPID instead of wait I think this makes more sense anyway as otherwise someone could easily introduce a hang if they unwittingly add a backgrounded job to this script. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Tue, Dec 01, 2020 at 04:42:03PM +1100, Herbert Xu wrote: > > Nevermind, I see that the script has been modified to use bash. > > I can reproduce the problem now so it's all good. OK the problem is this: sh -c 'sleep 1d& exec $MYSHELL -c "sleep 1& wait"' You can replace MYSHELL with whatever shell you want to use. Essentially dash will now wait for all children, even ones that were created prior to its existence, however, bash only waits for children that it created directly. FWIW ksh exhibits the same behaviour as dash and I think there is nothing wrong with this. So the problem is really in the parent of this shell, which appears to be bash: bash -c set -e; export USER=`id -nu`; . /etc/profile >/dev/null 2>&1 || true; . ~/.profile >/dev/null 2>&1 || true; buildtree="/tmp/autopkgtest-lxc.is4n6xxr/downtmp/build.f2G/real-tree"; mkdir -p -m 1777 -- "/tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-artifacts"; export AUTOPKGTEST_ARTIFACTS="/tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-artifacts"; export ADT_ARTIFACTS="$AUTOPKGTEST_ARTIFACTS"; mkdir -p -m 755 "/tmp/autopkgtest-lxc.is4n6xxr/downtmp/autopkgtest_tmp"; export AUTOPKGTEST_TMP="/tmp/autopkgtest-lxc.is4n6xxr/downtmp/autopkgtest_tmp"; export ADTTMP="$AUTOPKGTEST_TMP"; export DEBIAN_FRONTEND=noninteractive; export LANG=C.UTF-8; export DEB_BUILD_OPTIONS=parallel=2; unset LANGUAGE LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE LC_MONETARY LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT LC_IDENTIFICATION LC_ALL;rm -f /tmp/autopkgtest_script_pid; set -C; echo $$ > /tmp/autopkgtest_script_pid; set +C; trap "rm -f /tmp/autopkgtest_script_pid" EXIT INT QUIT PIPE; cd "$buildtree"; export AUTOPKGTEST_NORMAL_USER=; export ADT_NORMAL_USER=; chmod +x /tmp/autopkgtest-lxc.is4n6xxr/downtmp/build.f2G/real-tree/debian/tests/timedated; touch /tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-stdout /tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-stderr; /tmp/autopkgtest-lxc.is4n6xxr/downtmp/build.f2G/real-tree/debian/tests/timedated 2> >(tee -a /tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-stderr >&2) > >(tee -a /tmp/autopkgtest-lxc.is4n6xxr/downtmp/timedated-stdout); For some reason this is causing the final two tee's to be created as children of debian/tests/timedated rather than the bash shell. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Tue, Dec 01, 2020 at 04:38:37PM +1100, Herbert Xu wrote: > > FWIW I'm unable to reproduce it with autopkgtest. This is what > I get: > > root@test0:~# autopkgtest --test-name=timedated systemd-246.6/ -B -- lxc -s > autopkgtest-sid > autopkgtest [16:32:45]: starting date: 2020-12-01 > autopkgtest [16:32:45]: version 5.15 > autopkgtest [16:32:45]: host test0; command line: /usr/bin/autopkgtest > --test-name=timedated systemd-246.6/ -B -- lxc -s autopkgtest-sid > autopkgtest [16:33:13]: testbed dpkg architecture: amd64 > autopkgtest [16:33:13]: testbed running kernel: Linux 5.9.0-rc1+ #18 SMP > PREEMPT Sat Aug 29 23:45:30 AEST 2020 > autopkgtest [16:33:13]: unbuilt-tree systemd-246.6/ > autopkgtest [16:33:18]: testing package systemd version 246.6-5 Nevermind, I see that the script has been modified to use bash. I can reproduce the problem now so it's all good. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Changes to job handling cause hangs in wait
On Mon, Nov 16, 2020 at 10:29:16AM +, Andrej Shadura wrote: > > I’d like to forward a bug report I received. Michael, who reported it, > was able to bisect it to 6c691b3, which was the first of the series of > commits changing job handling. > > I must note that I had troubles debugging it: I was only able to > reproduce the issue in an autopkgtest virtual machine, but not on my > normal system nor in a fresh Debian unstable Docker container. FWIW I'm unable to reproduce it with autopkgtest. This is what I get: root@test0:~# autopkgtest --test-name=timedated systemd-246.6/ -B -- lxc -s autopkgtest-sid autopkgtest [16:32:45]: starting date: 2020-12-01 autopkgtest [16:32:45]: version 5.15 autopkgtest [16:32:45]: host test0; command line: /usr/bin/autopkgtest --test-name=timedated systemd-246.6/ -B -- lxc -s autopkgtest-sid autopkgtest [16:33:13]: testbed dpkg architecture: amd64 autopkgtest [16:33:13]: testbed running kernel: Linux 5.9.0-rc1+ #18 SMP PREEMPT Sat Aug 29 23:45:30 AEST 2020 autopkgtest [16:33:13]: unbuilt-tree systemd-246.6/ autopkgtest [16:33:18]: testing package systemd version 246.6-5 autopkgtest [16:33:18]: build not needed autopkgtest [16:33:18]: test timedated: preparing testbed Reading package lists... Done Building dependency tree Reading state information... Done Correcting dependencies...Starting pkgProblemResolver with broken count: 0 Starting 2 pkgProblemResolver with broken count: 0 Done Done Starting pkgProblemResolver with broken count: 0 Starting 2 pkgProblemResolver with broken count: 0 Done The following additional packages will be installed: acl libnss-systemd The following NEW packages will be installed: acl libnss-systemd 0 upgraded, 2 newly installed, 0 to remove and 0 not upgraded. 1 not fully installed or removed. Need to get 255 kB of archives. After this operation, 577 kB of additional disk space will be used. Get:1 http://deb.debian.org/debian sid/main amd64 libnss-systemd amd64 246.6-5 [194 kB] Get:2 http://deb.debian.org/debian sid/main amd64 acl amd64 2.2.53-8 [60.8 kB] Fetched 255 kB in 1s (489 kB/s) debconf: delaying package configuration, since apt-utils is not installed Selecting previously unselected package libnss-systemd:amd64. (Reading database ... 12641 files and directories currently installed.) Preparing to unpack .../libnss-systemd_246.6-5_amd64.deb ... Unpacking libnss-systemd:amd64 (246.6-5) ... Selecting previously unselected package acl. Preparing to unpack .../acl_2.2.53-8_amd64.deb ... Unpacking acl (2.2.53-8) ... Setting up libnss-systemd:amd64 (246.6-5) ... First installation detected... Checking NSS setup... Setting up acl (2.2.53-8) ... Setting up autopkgtest-satdep (0) ... Processing triggers for libc-bin (2.31-4) ... (Reading database ... 12667 files and directories currently installed.) Removing autopkgtest-satdep (0) ... autopkgtest [16:33:29]: test timedated: [--- original tz: Etc/UTC timedatectl works change timezone reset timezone to original no adjtime file UTC set in adjtime file non-zero values in adjtime file fourth line adjtime file no final newline in adjtime file only one line in adjtime file only one line in adjtime file, no final newline only two lines in adjtime file only two lines in adjtime file, no final newline unknown value in 3rd line of adjtime file disable NTP enable NTP re-disable NTP autopkgtest [16:33:32]: test timedated: ---] autopkgtest [16:33:32]: test timedated: - - - - - - - - - - results - - - - - - - - - - timedatedPASS autopkgtest [16:33:32]: summary timedatedPASS root@test0:~# Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Fwd: Bug#974900: dash removes trailing slash from script arguments
Andrej Shadura wrote: > > this is another bug report I have received. This is a bug in glob(3). Please dup and reassign to libc6. There is a patch in the queue to disable glob by default again. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] shell: Disable glob again as it strips traing slashes
On Mon, Nov 16, 2020 at 01:47:48PM +1100, Herbert Xu wrote: > René Scharfe wrote: > > > > on Debian testing dash eats trailing slashes of parameters that happen > > to be regular files when expanding "$@". Example: > > > > $ rm -f foo bar > > $ touch foo > > $ dash -c 'echo "$0" "$@"' baz foo/ bar/ ./ > > baz foo bar/ ./ > > In fact you just have to do > > dash -c 'echo bar\/' > > This is a bug in glob(3). It's stripping the slash. > > I guess we'll just have to disable glob again. This patch disables glob(3) by default. Reported-by: René Scharfe Signed-off-by: Herbert Xu diff --git a/configure.ac b/configure.ac index ab3c02e..e9ae792 100644 --- a/configure.ac +++ b/configure.ac @@ -39,8 +39,7 @@ fi AC_ARG_ENABLE(fnmatch, AS_HELP_STRING(--disable-fnmatch, \ [Do not use fnmatch(3) from libc])) -AC_ARG_ENABLE(glob, AS_HELP_STRING(--disable-glob, \ - [Do not use glob(3) from libc])) +AC_ARG_ENABLE(glob, AS_HELP_STRING(--enable-glob, [Use glob(3) from libc])) dnl Checks for libraries. @@ -128,7 +127,7 @@ if test "$enable_fnmatch" != no; then AC_CHECK_FUNCS(fnmatch, use_fnmatch=yes) fi -if test "$use_fnmatch" = yes && test "$enable_glob" != no; then +if test "$use_fnmatch" = yes && test "$enable_glob" = yes; then AC_CHECK_FUNCS(glob) fi -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] shell: Group readdir64/dirent64 with open64
Martijn Dekker wrote: > This commit introduced the build failure: > 3e3e7af1a49273a5e49d50565b3b079a2ab19142 > > The first error is: > expand.c:1365:9: error: incomplete definition of type 'struct dirent64' > if (dp->d_name[0] == '.' && ! matchdot) > ~~^ Thanks for the report, does this patch help? ---8<--- The test for open64 is separate from stat64 for macOS. However, the newly introduced tests for readdir64/dirent64 should be grouped with open64 instead of stat64 as otherwise they cause similar build failures. Reported-by: Martijn Dekker Signed-off-by: Herbert Xu diff --git a/configure.ac b/configure.ac index 955e2bb..ab3c02e 100644 --- a/configure.ac +++ b/configure.ac @@ -145,10 +145,6 @@ AC_CHECK_FUNC(stat64,, [ AC_DEFINE(fstat64, fstat, [64-bit operations are the same as 32-bit]) AC_DEFINE(lstat64, lstat, [64-bit operations are the same as 32-bit]) AC_DEFINE(stat64, stat, [64-bit operations are the same as 32-bit]) - AC_DEFINE(readdir64, readdir, - [64-bit operations are the same as 32-bit]) - AC_DEFINE(dirent64, dirent, - [64-bit operations are the same as 32-bit]) ]) AC_CHECK_FUNC(glob64,, [ @@ -161,6 +157,10 @@ AC_CHECK_FUNC(glob64,, [ dnl OS X apparently has stat64 but not open64. AC_CHECK_FUNC(open64,, [ AC_DEFINE(open64, open, [64-bit operations are the same as 32-bit]) + AC_DEFINE(readdir64, readdir, + [64-bit operations are the same as 32-bit]) + AC_DEFINE(dirent64, dirent, + [64-bit operations are the same as 32-bit]) ]) dnl Check if struct stat has st_mtim. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] dash: man pages: fix formatting
Bjarni Ingi Gislason wrote: > Fix formatting according to the output of "mandoc -Tlint". > > Overview: > > Start each sentence on a new line. > > Protect a punctuation mark in a macro call with '\&'. > > Trim trailing space. > > Add a missing comma in a row of words. > > Use an en-dash instead of '--' if there is space around it. > An em-dash is used without space around it. > > Comment out ".Pp" macros that do nothing. > > Split long sentences after a punctuation mark. > > Remove a "-width ..." for a ".Bl -item" macro, as it has no influence > > Details: > > mandoc: ./src/bltin/echo.1:69:38: WARNING: new sentence, new line > mandoc: ./src/bltin/echo.1:75:35: WARNING: new sentence, new line > mandoc: ./src/bltin/printf.1:205:12: WARNING: skipping empty macro: No > mandoc: ./src/bltin/printf.1:284:28: STYLE: whitespace at end of input line > mandoc: ./src/bltin/printf.1:288:20: STYLE: whitespace at end of input line > mandoc: ./src/bltin/printf.1:293:28: STYLE: whitespace at end of input line > mandoc: ./src/bltin/printf.1:353:31: WARNING: new sentence, new line > mandoc: ./src/bltin/printf.1:74:2: STYLE: useless macro: Tn > mandoc: ./src/bltin/printf.1:111:2: STYLE: useless macro: Tn > mandoc: ./src/bltin/printf.1:116:2: STYLE: useless macro: Tn > mandoc: ./src/bltin/printf.1:279:2: STYLE: useless macro: Tn > mandoc: ./src/bltin/printf.1:334:2: WARNING: unusual Xr punctuation: none > before vis(3) > mandoc: ./src/bltin/printf.1:334:2: WARNING: unusual Xr order: vis(3) after > printf(9) > mandoc: ./src/bltin/printf.1:348:2: STYLE: useless macro: Tn > mandoc: ./src/bltin/printf.1:333:6: STYLE: referenced manual not found: Xr > printf 9 > mandoc: ./src/bltin/printf.1:334:6: STYLE: referenced manual not found: Xr > vis 3 > mandoc: ./src/bltin/test.1:46:16: WARNING: skipping empty macro: Cm > mandoc: ./src/bltin/test.1:105:5: STYLE: useless macro: Tn > mandoc: ./src/dash.1:1180:58: WARNING: new sentence, new line > mandoc: ./src/dash.1:1186:13: STYLE: whitespace at end of input line > mandoc: ./src/dash.1:1194:38: WARNING: new sentence, new line > mandoc: ./src/dash.1:1200:35: WARNING: new sentence, new line > mandoc: ./src/dash.1:1474:71: WARNING: new sentence, new line > mandoc: ./src/dash.1:1783:62: WARNING: new sentence, new line > mandoc: ./src/dash.1:2061:22: WARNING: new sentence, new line > mandoc: ./src/dash.1:2311:54: WARNING: new sentence, new line > mandoc: ./src/dash.1:2315:63: WARNING: new sentence, new line > mandoc: ./src/dash.1:37:2: WARNING: prologue macros out of order: Dt after Os > mandoc: ./src/dash.1:87:2: STYLE: useless macro: Tn > mandoc: ./src/dash.1:94:2: STYLE: useless macro: Tn > mandoc: ./src/dash.1:343:2: STYLE: useless macro: Tn > mandoc: ./src/dash.1:442:17: STYLE: verbatim "--", maybe consider using \(em > mandoc: ./src/dash.1:466:2: STYLE: useless macro: Tn > mandoc: ./src/dash.1:581:34: STYLE: verbatim "--", maybe consider using \(em > mandoc: ./src/dash.1:583:25: STYLE: verbatim "--", maybe consider using \(em > mandoc: ./src/dash.1:585:43: STYLE: verbatim "--", maybe consider using \(em > mandoc: ./src/dash.1:595:11: STYLE: verbatim "--", maybe consider using \(em > mandoc: ./src/dash.1:618:29: STYLE: verbatim "--", maybe consider using \(em > mandoc: ./src/dash.1:697:2: WARNING: skipping paragraph macro: Pp before Bd > mandoc: ./src/dash.1:1344:2: STYLE: useless macro: Tn > mandoc: ./src/dash.1:1420:2: WARNING: skipping paragraph macro: Pp before Bd > mandoc: ./src/dash.1:1434:2: WARNING: skipping paragraph macro: Pp before Bd > mandoc: ./src/dash.1:1556:2: STYLE: useless macro: Tn > mandoc: ./src/dash.1:1587:2: STYLE: useless macro: Tn > mandoc: ./src/dash.1:1746:2: STYLE: useless macro: Tn > mandoc: ./src/dash.1:1875:5: STYLE: useless macro: Tn > mandoc: ./src/dash.1:1525:2: WARNING: skipping paragraph macro: Pp before It > mandoc: ./src/dash.1:2182:2: WARNING: skipping paragraph macro: Pp before It > mandoc: ./src/dash.1:2247:2: WARNING: sections out of conventional order: Sh > ENVIRONMENT > mandoc: ./src/dash.1:2323:11: WARNING: skipping -width argument: Bl -item > mandoc: ./src/dash.1:2347:31: STYLE: consider using OS macro: Nx > mandoc: ./src/dash.1:92:6: STYLE: referenced manual not found: Xr ksh 1 (2 > times) > mandoc: ./src/dash.1:253:6: STYLE: referenced manual not found: Xr emacs 1 > mandoc: ./src/dash.1:2253:9: STYLE: referenced manual not found: Xr passwd 4 > mandoc: ./src/dash.1:2330:6: STYLE: referenced manual not found: Xr csh 1 > > Signed-off-by: Bjarni Ingi Gislason > --- > src/bltin/echo.1 | 6 +++-- > src/bltin/printf.1 | 13 ++- > src/bltin/test.1 | 2 +- > src/dash.1 | 57 +++--- > 4 files changed, 45 insertions(+), 33 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Status of set -o pipefail
Ibrahim Ghazal wrote: > Back in December 2019, there was a thread on this mailing list about > set -o pipefail [1]. Two patches implementing it were posted [2][3]. > However, there was no activity afterwards and it was never merged into > dash. > > Could a committer look into the patches and apply either of them > please? From the last thread, there was no objection to adding the > feature since it will be in the next POSIX standard. > > [1] https://www.spinics.net/lists/dash/msg01879.html > [2] https://www.spinics.net/lists/dash/msg01886.html > [3] https://www.spinics.net/lists/dash/msg01888.html AFAIK nobody has actually posted a patch to this list. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Don't include config.h when building helpers using the native compiler
Fabrice Fontaine wrote: > config.h contains settings for the cross compiler (most importantly > 32/64bit versions of functions), so don't include it when calling the > native compiler to build the helpers. > > Otherwise we get build errors like: > > /usr/bin/gcc -include ../config.h -DBSD=1 -DSHELL -DIFS_BROKEN -g -O2 -Wall > -o mkinit mkinit.c > In file included from /usr/include/sys/stat.h:107, > from /usr/include/fcntl.h:38, > from mkinit.c:50: > /usr/include/bits/stat.h:117: error: redefinition of ‘struct stat’ > In file included from /usr/include/fcntl.h:38, > from mkinit.c:50: > /usr/include/sys/stat.h:504: error: redefinition of ‘stat’ > /usr/include/sys/stat.h:455: note: previous definition of ‘stat’ was here > > Signed-off-by: Peter Korsgaard > [Retrieved from: > https://git.buildroot.net/buildroot/tree/package/dash/0001-no-config.h-for-helpers.patch] > Signed-off-by: Fabrice Fontaine > --- > src/Makefile.am | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/src/Makefile.am b/src/Makefile.am > index 1732465..3b1b54e 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -7,7 +7,6 @@ COMMON_CPPFLAGS = \ > AM_CFLAGS = $(COMMON_CFLAGS) > AM_CPPFLAGS = -include $(top_builddir)/config.h $(COMMON_CPPFLAGS) > AM_CFLAGS_FOR_BUILD = -g -O2 $(COMMON_CFLAGS) > -AM_CPPFLAGS_FOR_BUILD = $(COMMON_CPPFLAGS) Hmm, why is config.h ending up in COMMON_CPPFLAGS at all? I don't see in my build: $ make V=1 -C build ... gcc -I. -I../../src -I.. -DBSD=1 -DSHELL -g -O2 -Wall-o mkinit ../../src/mkinit.c ./mkinit ../../src/alias.c ../../src/arith_yacc.c ../../src/arith_yylex.c ../../src/cd.c ../../src/error.c ../../src/eval.c ../../src/exec.c ../../src/expand.c ../../src/histedit.c ../../src/input.c ../../src/jobs.c ../../src/mail.c ../../src/main.c ../../src/memalloc.c ../../src/miscbltin.c ../../src/mystring.c ../../src/options.c ../../src/parser.c ../../src/redir.c ../../src/show.c ../../src/trap.c ../../src/output.c ../../src/bltin/printf.c ../../src/system.c ../../src/bltin/test.c ../../src/bltin/times.c ../../src/var.c I'm not cross-compiling though. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] eval: Check nflag in evaltree instead of cmdloop
This patch moves the nflag check from cmdloop into evaltree. This
is so that nflag will be in force even if we enter the shell via a
path other than cmdloop, e.g., through sh -c.
Reported-by: Joey Hess
Signed-off-by: Herbert Xu
diff --git a/src/eval.c b/src/eval.c
index d10be38..9476fbb 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -213,6 +213,9 @@ evaltree(union node *n, int flags)
setstackmark();
+ if (nflag)
+ goto out;
+
if (n == NULL) {
TRACE(("evaltree(NULL) called\n"));
goto out;
diff --git a/src/main.c b/src/main.c
index 7a28534..5c49fdc 100644
--- a/src/main.c
+++ b/src/main.c
@@ -233,7 +233,7 @@ cmdloop(int top)
out2str("\nUse \"exit\" to leave shell.\n");
}
numeof++;
- } else if (nflag == 0) {
+ } else {
int i;
job_warning = (job_warning == 2) ? 1 : 0;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Bug when parsing redirection syntax error
Matt Whitlock wrote:
> $ cat << dash: 4: Syntax error: redirection unexpected
> $ true
> dash: 4: Etrue: not found
>
> This is with Dash 0.5.11.
>
> It looks like the first character after the unexpected redirection operator
> mistakenly gets retained in a buffer somewhere and interferes with the next
> command to be read.
This is because dash needs to clear the unget buffer on reset.
Reported-by: Matt Whitlock
Fixes: 17db43b58415 ("input: Allow two consecutive calls to pungetc")
Signed-off-by: Herbert Xu
diff --git a/src/input.c b/src/input.c
index 4987732..d7c101b 100644
--- a/src/input.c
+++ b/src/input.c
@@ -87,6 +87,7 @@ INIT {
RESET {
/* clear input buffer */
basepf.lleft = basepf.nleft = 0;
+ basepf.unget = 0;
popallfiles();
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] jobs: Fix waitcmd busy loop
We need to clear gotsigchld in waitproc because it is used as
a loop conditional for the waitcmd case. Without it waitcmd
may busy loop after a SIGCHLD.
This patch also changes gotsigchld into a volatile sig_atomic_t
to prevent compilers from optimising its accesses away.
Fixes: 6c691b3e5099 ("jobs: Only clear gotsigchld when waiting...")
Signed-off-by: Herbert Xu
diff --git a/src/jobs.c b/src/jobs.c
index 94bf47e..3417633 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -1135,7 +1135,6 @@ static int dowait(int block, struct job *jp)
rpid = 1;
do {
- gotsigchld = 0;
pid = waitone(block, jp);
rpid &= !!pid;
@@ -1175,6 +1174,7 @@ waitproc(int block, int *status)
#endif
do {
+ gotsigchld = 0;
do
err = wait3(status, flags, NULL);
while (err < 0 && errno == EINTR);
diff --git a/src/trap.c b/src/trap.c
index 82e7ece..cd84814 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -76,7 +76,7 @@ static char gotsig[NSIG - 1];
/* last pending signal */
volatile sig_atomic_t pending_sig;
/* received SIGCHLD */
-int gotsigchld;
+volatile sig_atomic_t gotsigchld;
extern char *signal_names[];
diff --git a/src/trap.h b/src/trap.h
index 4c455a8..beaf660 100644
--- a/src/trap.h
+++ b/src/trap.h
@@ -39,7 +39,7 @@
extern int trapcnt;
extern char sigmode[];
extern volatile sig_atomic_t pending_sig;
-extern int gotsigchld;
+extern volatile sig_atomic_t gotsigchld;
int trapcmd(int, char **);
void setsignal(int);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] error: Remove USE_NORETURN ifdef
The USE_NORETURN was added because gcc was buggy almost 20 years ago. This is no longer needed and this patch removes it. Signed-off-by: Herbert Xu diff --git a/src/error.h b/src/error.h index 94e30a2..661a8a0 100644 --- a/src/error.h +++ b/src/error.h @@ -116,11 +116,7 @@ void __inton(void); #define int_pending() intpending void exraise(int) __attribute__((__noreturn__)); -#ifdef USE_NORETURN void onint(void) __attribute__((__noreturn__)); -#else -void onint(void); -#endif extern int errlinno; void sh_error(const char *, ...) __attribute__((__noreturn__)); void exerror(int, const char *, ...) __attribute__((__noreturn__)); -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] expand: Make glob(3) interruptible by SIGINT
If glob(3) is used then it can't be interrupted by SIGINT. This
is bad when an expansion causes a large number of entries to be
generated. This patch improves things by adding an int_pending
check to gl_opendir call. Note that this is still not perfect,
e.g., the sort would still be uninterruptible.
Signed-off-by: Herbert Xu
diff --git a/configure.ac b/configure.ac
index ce5feec..955e2bb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -151,6 +151,13 @@ AC_CHECK_FUNC(stat64,, [
[64-bit operations are the same as 32-bit])
])
+AC_CHECK_FUNC(glob64,, [
+ AC_DEFINE(glob64_t, glob_t, [64-bit operations are the same as 32-bit])
+ AC_DEFINE(glob64, glob, [64-bit operations are the same as 32-bit])
+ AC_DEFINE(globfree64, globfree,
+ [64-bit operations are the same as 32-bit])
+])
+
dnl OS X apparently has stat64 but not open64.
AC_CHECK_FUNC(open64,, [
AC_DEFINE(open64, open, [64-bit operations are the same as 32-bit])
diff --git a/src/expand.c b/src/expand.c
index 1730670..aea5cc4 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -120,7 +120,7 @@ static size_t memtodest(const char *p, size_t len, int
flags);
STATIC ssize_t varvalue(char *, int, int, int);
STATIC void expandmeta(struct strlist *);
#ifdef HAVE_GLOB
-STATIC void addglob(const glob_t *);
+static void addglob(const glob64_t *);
#else
STATIC void expmeta(char *, unsigned, unsigned);
STATIC struct strlist *expsort(struct strlist *);
@@ -1154,6 +1154,20 @@ out:
*/
#ifdef HAVE_GLOB
+#ifdef __GLIBC__
+void *opendir_interruptible(const char *pathname)
+{
+ if (int_pending()) {
+ suppressint = 0;
+ onint();
+ }
+
+ return opendir(pathname);
+}
+#else
+#define GLOB_ALTDIRFUNC 0
+#endif
+
STATIC void
expandmeta(struct strlist *str)
{
@@ -1161,14 +1175,23 @@ expandmeta(struct strlist *str)
while (str) {
const char *p;
- glob_t pglob;
+ glob64_t pglob;
int i;
if (fflag)
goto nometa;
+
+#ifdef __GLIBC__
+ pglob.gl_closedir = (void *)closedir;
+ pglob.gl_readdir = (void *)readdir64;
+ pglob.gl_opendir = opendir_interruptible;
+ pglob.gl_lstat = lstat64;
+ pglob.gl_stat = stat64;
+#endif
+
INTOFF;
p = preglob(str->text, RMESCAPE_ALLOC | RMESCAPE_HEAP);
- i = glob(p, GLOB_NOMAGIC, 0, );
+ i = glob64(p, GLOB_ALTDIRFUNC | GLOB_NOMAGIC, 0, );
if (p != str->text)
ckfree(p);
switch (i) {
@@ -1177,12 +1200,12 @@ expandmeta(struct strlist *str)
(GLOB_NOMAGIC | GLOB_NOCHECK))
goto nometa2;
addglob();
- globfree();
+ globfree64();
INTON;
break;
case GLOB_NOMATCH:
nometa2:
- globfree();
+ globfree64();
INTON;
nometa:
*exparg.lastp = str;
@@ -1201,9 +1224,7 @@ nometa:
* Add the result of glob(3) to the list.
*/
-STATIC void
-addglob(pglob)
- const glob_t *pglob;
+static void addglob(const glob64_t *pglob)
{
char **p = pglob->gl_pathv;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] shell: Enable fnmatch/glob by default
As fnmatch(3) and glob(3) from glibc are now working consistently, this patch enables them by default. Signed-off-by: Herbert Xu diff --git a/configure.ac b/configure.ac index dbd97d8..d73b2bf 100644 --- a/configure.ac +++ b/configure.ac @@ -37,9 +37,10 @@ if test "$enable_static" = "yes"; then export LDFLAGS="-static -Wl,--fatal-warnings" fi -AC_ARG_ENABLE(fnmatch, AS_HELP_STRING(--enable-fnmatch, \ - [Use fnmatch(3) from libc])) -AC_ARG_ENABLE(glob, AS_HELP_STRING(--enable-glob, [Use glob(3) from libc])) +AC_ARG_ENABLE(fnmatch, AS_HELP_STRING(--disable-fnmatch, \ + [Do not use fnmatch(3) from libc])) +AC_ARG_ENABLE(glob, AS_HELP_STRING(--disable-glob, \ + [Do not use glob(3) from libc])) dnl Checks for libraries. @@ -122,12 +123,12 @@ if test "$enable_test_workaround" = "yes"; then [Define if your faccessat tells root all files are executable]) fi -if test "$enable_fnmatch" = yes; then +if test "$enable_fnmatch" != no; then use_fnmatch= AC_CHECK_FUNCS(fnmatch, use_fnmatch=yes) fi -if test "$use_fnmatch" = yes && test "$enable_glob" = yes; then +if test "$use_fnmatch" = yes && test "$enable_glob" != no; then AC_CHECK_FUNCS(glob) fi -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Fix typos
Martin Michlmayr wrote: > Signed-off-by: Martin Michlmayr Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] redir: Retry open64 on EINTR
It is possible for open64 to block on named pipes, and therefore
it can be interrupted by signals and return EINTR. We should only
let it fail with EINTR if real signals are pending (i.e., it should
not fail on SIGCHLD if SIGCHLD has not been trapped).
This patch adds a new helper sh_open to retry the open64 call if
necessary. It also calls sh_error when appropriate.
Fixes: 3800d4934391 ("[JOBS] Fix dowait signal race")
Reported-by: Samuel Thibault
Signed-off-by: Herbert Xu
diff --git a/src/input.c b/src/input.c
index 17544e7..6103312 100644
--- a/src/input.c
+++ b/src/input.c
@@ -393,12 +393,9 @@ setinputfile(const char *fname, int flags)
int fd;
INTOFF;
- if ((fd = open64(fname, O_RDONLY)) < 0) {
- if (flags & INPUT_NOFILE_OK)
- goto out;
- exitstatus = 127;
- exerror(EXERROR, "Can't open %s", fname);
- }
+ fd = sh_open(fname, O_RDONLY, flags & INPUT_NOFILE_OK);
+ if (fd < 0)
+ goto out;
if (fd < 10)
fd = savefd(fd, fd);
setinputfd(fd, flags & INPUT_PUSH_FILE);
diff --git a/src/jobs.c b/src/jobs.c
index f65435d..95e7ec4 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -196,7 +196,7 @@ setjobctl(int on)
return;
if (on) {
int ofd;
- ofd = fd = open64(_PATH_TTY, O_RDWR);
+ ofd = fd = sh_open(_PATH_TTY, O_RDWR, 1);
if (fd < 0) {
fd += 3;
while (!isatty(fd))
@@ -887,8 +887,7 @@ static void forkchild(struct job *jp, union node *n, int
mode)
ignoresig(SIGQUIT);
if (jp->nprocs == 0) {
close(0);
- if (open64(_PATH_DEVNULL, O_RDONLY) != 0)
- sh_error("Can't open %s", _PATH_DEVNULL);
+ sh_open(_PATH_DEVNULL, O_RDONLY, 0);
}
}
if (!oldlvl && iflag) {
diff --git a/src/redir.c b/src/redir.c
index 895140c..93abba3 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -55,6 +55,7 @@
#include "output.h"
#include "memalloc.h"
#include "error.h"
+#include "trap.h"
#define EMPTY -2 /* marks an unused slot in redirtab */
@@ -180,56 +181,83 @@ redirect(union node *redir, int flags)
}
+static int sh_open_fail(const char *, int, int) __attribute__((__noreturn__));
+static int sh_open_fail(const char *pathname, int flags, int e)
+{
+ const char *word;
+ int action;
+
+ word = "open";
+ action = E_OPEN;
+ if (flags & O_CREAT) {
+ word = "create";
+ action = E_CREAT;
+ }
+
+ sh_error("cannot %s %s: %s", word, pathname, errmsg(e, action));
+}
+
+
+int sh_open(const char *pathname, int flags, int mayfail)
+{
+ int fd;
+ int e;
+
+ do {
+ fd = open64(pathname, flags, 0666);
+ e = errno;
+ } while (fd < 0 && e == EINTR && !pending_sig);
+
+ if (mayfail || fd >= 0)
+ return fd;
+
+ sh_open_fail(pathname, flags, e);
+}
+
+
STATIC int
openredirect(union node *redir)
{
struct stat64 sb;
char *fname;
+ int flags;
int f;
switch (redir->nfile.type) {
case NFROM:
- fname = redir->nfile.expfname;
- if ((f = open64(fname, O_RDONLY)) < 0)
- goto eopen;
+ flags = O_RDONLY;
+do_open:
+ f = sh_open(redir->nfile.expfname, flags, 0);
break;
case NFROMTO:
- fname = redir->nfile.expfname;
- if ((f = open64(fname, O_RDWR|O_CREAT, 0666)) < 0)
- goto ecreate;
- break;
+ flags = O_RDWR|O_CREAT;
+ goto do_open;
case NTO:
/* Take care of noclobber mode. */
if (Cflag) {
fname = redir->nfile.expfname;
if (stat64(fname, ) < 0) {
- if ((f = open64(fname, O_WRONLY|O_CREAT|O_EXCL,
0666)) < 0)
- goto ecreate;
- } else if (!S_ISREG(sb.st_mode)) {
- if ((f = open64(fname, O_WRONLY, 0666)) < 0)
- goto ecreate;
- if (!fstat64(f, ) && S_ISREG(sb.st_mode)) {
- close(f);
- errno = EEXIST;
- goto ecreate;
- }
- } else {
- errno = EEXIST;
+
[PATCH] parser: Get rid of PEOA
PEOA is a special character used to mark an alias as being finished
so that we don't enter an infinite loop with nested aliases. It
complicates the parser because we have to ensure that it is skipped
where necessary and not copied to the resulting token text.
This patch removes it and instead delays the marking of aliases
until the second pgetc. This has the same effect as the current
PEOA code while keeping the complexities within the input code.
Signed-off-by: Herbert Xu
diff --git a/src/input.c b/src/input.c
index 17544e7..cf4efdc 100644
--- a/src/input.c
+++ b/src/input.c
@@ -68,6 +68,7 @@ struct parsefile *parsefile = /* current
input file */
int whichprompt; /* 1 == PS1, 2 == PS2 */
STATIC void pushfile(void);
+static void popstring(void);
static int preadfd(void);
static void setinputfd(int fd, int push);
static int preadbuffer(void);
@@ -99,13 +100,32 @@ FORKRESET {
#endif
-/*
- * Read a character from the script, returning PEOF on end of file.
- * Nul characters in the input are silently discarded.
- */
+static void freestrings(struct strpush *sp)
+{
+ INTOFF;
+ do {
+ struct strpush *psp;
-int
-pgetc(void)
+ if (sp->ap) {
+ sp->ap->flag &= ~ALIASINUSE;
+ if (sp->ap->flag & ALIASDEAD) {
+ unalias(sp->ap->name);
+ }
+ }
+
+ psp = sp;
+ sp = sp->spfree;
+
+ if (psp != &(parsefile->basestrpush))
+ ckfree(psp);
+ } while (sp);
+
+ parsefile->spfree = NULL;
+ INTON;
+}
+
+
+static int __pgetc(void)
{
int c;
@@ -125,17 +145,18 @@ pgetc(void)
/*
- * Same as pgetc(), but ignores PEOA.
+ * Read a character from the script, returning PEOF on end of file.
+ * Nul characters in the input are silently discarded.
*/
-int
-pgetc2()
+int pgetc(void)
{
- int c;
- do {
- c = pgetc();
- } while (c == PEOA);
- return c;
+ struct strpush *sp = parsefile->spfree;
+
+ if (unlikely(sp))
+ freestrings(sp);
+
+ return __pgetc();
}
@@ -214,16 +235,8 @@ static int preadbuffer(void)
char savec;
if (unlikely(parsefile->strpush)) {
- if (
- parsefile->nleft == -1 &&
- parsefile->strpush->ap &&
- parsefile->nextc[-1] != ' ' &&
- parsefile->nextc[-1] != '\t'
- ) {
- return PEOA;
- }
popstring();
- return pgetc();
+ return __pgetc();
}
if (unlikely(parsefile->nleft == EOF_NLEFT ||
parsefile->buf == NULL))
@@ -331,7 +344,8 @@ pushstring(char *s, void *ap)
len = strlen(s);
INTOFF;
/*dprintf("*** calling pushstring: %s, %d\n", s, len);*/
- if (parsefile->strpush) {
+ if ((unsigned long)parsefile->strpush |
+ (unsigned long)parsefile->spfree) {
sp = ckmalloc(sizeof (struct strpush));
sp->prev = parsefile->strpush;
parsefile->strpush = sp;
@@ -340,6 +354,7 @@ pushstring(char *s, void *ap)
sp->prevstring = parsefile->nextc;
sp->prevnleft = parsefile->nleft;
sp->unget = parsefile->unget;
+ sp->spfree = parsefile->spfree;
memcpy(sp->lastc, parsefile->lastc, sizeof(sp->lastc));
sp->ap = (struct alias *)ap;
if (ap) {
@@ -349,11 +364,11 @@ pushstring(char *s, void *ap)
parsefile->nextc = s;
parsefile->nleft = len;
parsefile->unget = 0;
+ parsefile->spfree = NULL;
INTON;
}
-void
-popstring(void)
+static void popstring(void)
{
struct strpush *sp = parsefile->strpush;
@@ -366,10 +381,6 @@ popstring(void)
if (sp->string != sp->ap->val) {
ckfree(sp->string);
}
- sp->ap->flag &= ~ALIASINUSE;
- if (sp->ap->flag & ALIASDEAD) {
- unalias(sp->ap->name);
- }
}
parsefile->nextc = sp->prevstring;
parsefile->nleft = sp->prevnleft;
@@ -377,8 +388,7 @@ popstring(void)
memcpy(parsefile->lastc, sp->lastc, sizeof(sp->lastc));
/*dprintf("*** calling popstring: restoring to '%s'\n", parsenextc);*/
parsefile->strpush = sp->prev;
- if (sp != &(parsefile->basestrpush))
- ckfree(sp);
+ parsefile->spfree = sp;
INTON;
}
@@ -460,6 +470,7 @@ pushfile(void)
pf->prev = parsefile;
pf->fd = -1;
p
parser: Fix double-backslash nl in old-style command sub
Ron Yorston wrote:
>
> Alternatively I see that BusyBox ash did this:
>
>case '\\':
> - pc = pgetc();
> - if (pc == '\n') {
> - nlprompt();
> - /*
> -* If eating a newline, avoid putting
> -* the newline into the new character
> -* stream (via the STPUTC after the
> -* switch).
> -*/
> - continue;
> - }
> + pc = pgetc(); /* or pgetc_eatbnl()? why (example)? */
Yes this is the correct fix.
---8<---
When handling backslashes within an old-style command substitution,
we should not call pgetc_eatbnl because that would treat the next
backslash character as another escape character if it was then
followed by a new-line.
This patch fixes it by calling pgetc.
Reported-by: Matt Whitlock
Fixes: 6bbc71d84bea ("parser: use pgetc_eatbnl() in more places")
Signed-off-by: Herbert Xu
diff --git a/src/parser.c b/src/parser.c
index 3131045..03c103b 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1393,7 +1393,7 @@ parsebackq: {
goto done;
case '\\':
-pc = pgetc_eatbnl();
+pc = pgetc();
if (pc != '\\' && pc != '`' && pc != '$'
&& (!synstack->dblquote || pc != '"'))
STPUTC('\\', pout);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] parser: Save and restore heredoclist in expandstr
On Sun, May 17, 2020 at 01:19:28PM +0100, Harald van Dijk wrote:
>
> This still does not restore the state completely. It does not clean up any
> pending heredocs. I see:
>
> $ PS1='$(< src/dash: 1: Syntax error: Unterminated quoted string
> $(< >
>
> That is, after entering the ':' command, the shell is still trying to read
> the heredoc from the prompt.
This patch saves and restores the heredoclist in expandstr.
It also removes a bunch of unnecessary volatiles as those variables
are only referenced in case of a longjmp other than one started by
a signal like SIGINT.
Reported-by: Harald van Dijk
Signed-off-by: Herbert Xu
diff --git a/src/parser.c b/src/parser.c
index 3131045..54c2861 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1565,10 +1565,11 @@ setprompt(int which)
const char *
expandstr(const char *ps)
{
- struct parsefile *volatile file_stop;
+ struct parsefile *file_stop;
struct jmploc *volatile savehandler;
- const char *volatile result;
- volatile int saveprompt;
+ struct heredoc *saveheredoclist;
+ const char *result;
+ int saveprompt;
struct jmploc jmploc;
union node n;
int err;
@@ -1578,6 +1579,8 @@ expandstr(const char *ps)
/* XXX Fix (char *) cast. */
setinputstring((char *)ps);
+ saveheredoclist = heredoclist;
+ heredoclist = NULL;
saveprompt = doprompt;
doprompt = 0;
result = ps;
@@ -1603,6 +1606,7 @@ out:
doprompt = saveprompt;
unwindfiles(file_stop);
+ heredoclist = saveheredoclist;
return result;
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v3 PATCH] mktokens relative TMPDIR
On Wed, Apr 29, 2020 at 08:04:21PM -0700, Michael Greenberg wrote: > The mktokens script fails when /tmp isn't writable (e.g., when building > in a sandbox with a different TMPDIR). Replace absolute references to > /tmp to relative references to TMPDIR. If TMPDIR is unset or null, > default to /tmp. > > The mkbuiltins script was already hardened to work relative to TMPDIR, > also defaulting to /tmp. > > v2 ensures that TMPDIR is quoted. > v3 adds an extra quotation that prevents extra pathname expansions. > > Signed-off-by: Michael Greenberg Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] input: Fix compiling against libedit with -fno-common
Mike Gilbert wrote: > From: Jeroen Roovers > > With -fno-common, which will be enabled by default in GCC 10, we see > this error: > > ld: input.o:(.bss+0x0): multiple definition of `el'; > histedit.o:(.bss+0x8): first defined here > > To fix this, simply remove the definition as it is not needed. > > Signed-off-by: Jeroen Roovers > Signed-off-by: Mike Gilbert > --- > src/input.c | 4 > 1 file changed, 4 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Remove unused expandmeta() flag parameter
On Wed, Apr 29, 2020 at 08:29:53AM +0200, Denys Vlasenko wrote: > Signed-off-by: Denys Vlasenko > --- > src/expand.c | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
shell: Always use explicit large file API
There are some remaining stat/readdir calls in dash that may lead
to spurious EOVERFLOW errors on 32-bit platforms. This patch changes
them (as well as open(2)) to use the explicit large file API.
Reported-by: Tatsuki Sugiura
Signed-off-by: Herbert Xu
diff --git a/configure.ac b/configure.ac
index 5dab5aa..dbd97d8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -144,8 +144,13 @@ AC_CHECK_FUNC(stat64,, [
AC_DEFINE(fstat64, fstat, [64-bit operations are the same as 32-bit])
AC_DEFINE(lstat64, lstat, [64-bit operations are the same as 32-bit])
AC_DEFINE(stat64, stat, [64-bit operations are the same as 32-bit])
+ AC_DEFINE(readdir64, readdir,
+ [64-bit operations are the same as 32-bit])
+ AC_DEFINE(dirent64, dirent,
+ [64-bit operations are the same as 32-bit])
])
+dnl OS X apparently has stat64 but not open64.
AC_CHECK_FUNC(open64,, [
AC_DEFINE(open64, open, [64-bit operations are the same as 32-bit])
])
diff --git a/src/bltin/test.c b/src/bltin/test.c
index b7188df..c7fc479 100644
--- a/src/bltin/test.c
+++ b/src/bltin/test.c
@@ -473,17 +473,17 @@ static int isoperand(char **tp)
static int
newerf (const char *f1, const char *f2)
{
- struct stat b1, b2;
+ struct stat64 b1, b2;
#ifdef HAVE_ST_MTIM
- return (stat (f1, ) == 0 &&
- stat (f2, ) == 0 &&
+ return (stat64(f1, ) == 0 &&
+ stat64(f2, ) == 0 &&
( b1.st_mtim.tv_sec > b2.st_mtim.tv_sec ||
(b1.st_mtim.tv_sec == b2.st_mtim.tv_sec && (b1.st_mtim.tv_nsec
> b2.st_mtim.tv_nsec )))
);
#else
- return (stat (f1, ) == 0 &&
- stat (f2, ) == 0 &&
+ return (stat64(f1, ) == 0 &&
+ stat64(f2, ) == 0 &&
b1.st_mtime > b2.st_mtime);
#endif
}
@@ -491,17 +491,17 @@ newerf (const char *f1, const char *f2)
static int
olderf (const char *f1, const char *f2)
{
- struct stat b1, b2;
+ struct stat64 b1, b2;
#ifdef HAVE_ST_MTIM
- return (stat (f1, ) == 0 &&
- stat (f2, ) == 0 &&
+ return (stat64(f1, ) == 0 &&
+ stat64(f2, ) == 0 &&
(b1.st_mtim.tv_sec < b2.st_mtim.tv_sec ||
(b1.st_mtim.tv_sec == b2.st_mtim.tv_sec && (b1.st_mtim.tv_nsec
< b2.st_mtim.tv_nsec )))
);
#else
- return (stat (f1, ) == 0 &&
- stat (f2, ) == 0 &&
+ return (stat64(f1, ) == 0 &&
+ stat64(f2, ) == 0 &&
b1.st_mtime < b2.st_mtime);
#endif
}
@@ -509,10 +509,10 @@ olderf (const char *f1, const char *f2)
static int
equalf (const char *f1, const char *f2)
{
- struct stat b1, b2;
+ struct stat64 b1, b2;
- return (stat (f1, ) == 0 &&
- stat (f2, ) == 0 &&
+ return (stat64(f1, ) == 0 &&
+ stat64(f2, ) == 0 &&
b1.st_dev == b2.st_dev &&
b1.st_ino == b2.st_ino);
}
diff --git a/src/cd.c b/src/cd.c
index b6742af..1ef1dc5 100644
--- a/src/cd.c
+++ b/src/cd.c
@@ -96,7 +96,7 @@ cdcmd(int argc, char **argv)
const char *path;
const char *p;
char c;
- struct stat statb;
+ struct stat64 statb;
int flags;
int len;
@@ -132,7 +132,7 @@ dotdot:
c = *p;
p = stalloc(len);
- if (stat(p, ) >= 0 && S_ISDIR(statb.st_mode)) {
+ if (stat64(p, ) >= 0 && S_ISDIR(statb.st_mode)) {
if (c && c != ':')
flags |= CD_PRINT;
docd:
diff --git a/src/expand.c b/src/expand.c
index 4a5d75a..ecd7ee5 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -1286,7 +1286,7 @@ expmeta(char *name, unsigned name_len, unsigned
expdir_len)
int metaflag;
struct stat64 statb;
DIR *dirp;
- struct dirent *dp;
+ struct dirent64 *dp;
int atend;
int matchdot;
int esc;
@@ -1363,7 +1363,7 @@ expmeta(char *name, unsigned name_len, unsigned
expdir_len)
p++;
if (*p == '.')
matchdot++;
- while (! int_pending() && (dp = readdir(dirp)) != NULL) {
+ while (! int_pending() && (dp = readdir64(dirp)) != NULL) {
if (dp->d_name[0] == '.' && ! matchdot)
continue;
if (pmatch(start, dp->d_name)) {
diff --git a/src/input.c b/src/input.c
index 177fd0a..7d6be63 100644
--- a/src/input.c
+++ b/src/input.c
@@ -397,7 +397,7 @@ setinputfile(const char *fname, int flags)
int fd;
INTOFF;
- if ((fd = open(fname, O_RDONLY)) < 0) {
+ if ((fd = open64(fname, O_RDONLY)) < 0) {
Re: [v2 PATCH] mktokens relative TMPDIR
On Wed, Apr 29, 2020 at 10:51:41AM -0700, Michael Greenberg wrote:
> The mktokens script fails when /tmp isn't writable (e.g., when building
> in a sandbox with a different TMPDIR). Replace absolute references to
> /tmp to relative references to TMPDIR. If TMPDIR is unset or null,
> default to /tmp.
>
> The mkbuiltins script was already hardened to work relative to TMPDIR,
> also defaulting to /tmp.
>
> v2 ensures that TMPDIR is quoted.
>
> Signed-off-by: Michael Greenberg
>
> diff --git a/src/mktokens b/src/mktokens
> index cd52241..3ab7bc5 100644
> --- a/src/mktokens
> +++ b/src/mktokens
> @@ -37,7 +37,9 @@
> # token marks the end of a list. The third column is the name to print in
> # error messages.
>
> -cat > /tmp/ka$$ <<\!
> +: ${TMPDIR:=/tmp}
Could you quote this one too? Otherwise it could result in
unnecessary pattern expansion (e.g., someone does TMPDIR=/*/*/*).
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Rename DOWAIT_NORMAL to DOWAIT_NONBLOCK
On Wed, Feb 19, 2020 at 11:30:08AM +0100, Denys Vlasenko wrote: > To make it clearer what it is doing: nonblocking wait() > > Signed-off-by: Denys Vlasenko > --- > src/jobs.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Remove poplocalvars() always-zero argument, make it static
On Wed, Feb 19, 2020 at 05:39:13PM +0100, Denys Vlasenko wrote: > Signed-off-by: Denys Vlasenko > --- > src/var.c | 24 > src/var.h | 1 - > 2 files changed, 4 insertions(+), 21 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] mktokens relative TMPDIR
On Mon, Feb 17, 2020 at 07:50:50PM -0800, Michael Greenberg wrote: > The mktokens script fails when /tmp isn't writable (e.g., when building > in a sandbox with a different TMPDIR). Replace absolute references to > /tmp to relative references to TMPDIR. If TMPDIR is unset or null, > default to /tmp. > > The mkbuiltins script was already hardened to work relative to TMPDIR, > also defaulting to /tmp. > > Signed-off-by: Michael Greenberg Please make sure TMPDIR is quoted. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] parser: Fix alias expansion after heredoc or newlines
The following patch is meant to be applied on top of:
https://patchwork.kernel.org/patch/11512439/
---8<---
This script should print OK:
alias a="case x in " b=x
a
b) echo BAD;; esac
alias BEGIN={ END=}
BEGIN
cat <<- EOF > /dev/null
$(:)
EOF
END
: <<- EOF &&
$(:)
EOF
BEGIN
echo OK
END
However, because the value of checkkwd is either zeroed when it
shouldn't, or isn't zeroed when it should, dash currently gets
it wrong in every case.
This patch fixes it by saving checkkwd and zeroing it where needed.
Suggested-by: Harald van Dijk
Reported-by: Harald van Dijk
Reported-by: Martijn Dekker
Signed-off-by: Herbert Xu
diff --git a/src/parser.c b/src/parser.c
index 5c9e9a0..be84f8b 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -704,10 +704,14 @@ top:
if (kwd & CHKNL) {
while (t == TNL) {
parseheredoc();
+ checkkwd = 0;
t = xxreadtoken();
}
}
+ kwd |= checkkwd;
+ checkkwd = 0;
+
if (t != TWORD || quoteflag) {
goto out;
}
@@ -725,7 +729,7 @@ top:
}
}
- if (checkkwd & CHKALIAS) {
+ if (kwd & CHKALIAS) {
struct alias *ap;
if ((ap = lookupalias(wordtext, 1)) != NULL) {
if (*ap->val) {
@@ -735,7 +739,6 @@ top:
}
}
out:
- checkkwd = 0;
#ifdef DEBUG
if (!alreadyseen)
TRACE(("token %s %s\n", tokname[t], t == TWORD ? wordtext : ""));
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] dash: Fix stack overflow from infinite recursion in script
Martijn Dekker wrote: > > I was mistaken: both AT ksh93 and NetBSD 8.1 sh have a hard-coded > limit of 1000. Which still doesn't help you when you set the ulimit stack limit to some ridiculously small value. Such a limit in the presence of variable stack ulimits is just wrong. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Superseded patch?
Michael Greenberg wrote: > > I just noticed that an old patch of mine is marked as 'superseded' in > patchwork <https://patchwork.kernel.org/patch/9526895/>. Was this a > conscious decision not to fix the POSIX noncompliance here, an accident, > or something else? Should I resubmit the patch? It means that the patch in question has been superseded by another one, namely: https://patchwork.kernel.org/patch/10263979/ Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: GCC format-security fixes
Michael Orlitzky wrote: > Petr Šabata posted a patch a few years ago to fix the build with GCC's > format-string security errors enabled: > > https://www.spinics.net/lists/dash/msg00869.html > > We're still carrying that patch in Gentoo so I wanted to bring it up > again. I guess it just got lost? How about a resend? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
parser: Only accept single-digit parameter expansion outside of braces
On Thu, Apr 25, 2019 at 01:39:52AM +, Michael Orlitzky wrote:
> The POSIX spec says,
>
> The parameter name or symbol can be enclosed in braces, which are
> optional except for positional parameters with more than one digit or
> when parameter is a name and is followed by a character that could be
> interpreted as part of the name.
>
> However, dash seems to diverge from that behavior when we get to $10:
>
> $ cat test.sh
> echo $10
>
> $ dash ./test.sh one two three four five six seven eight nine ten
> ten
>
> $ bash ./test.sh one two three four five six seven eight nine ten
> one0
This patch should fix the problem.
Signed-off-by: Herbert Xu
diff --git a/src/parser.c b/src/parser.c
index 1f9e8ec..2f14bf3 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1268,7 +1268,7 @@ varname:
do {
STPUTC(c, out);
c = pgetc_eatbnl();
- } while (is_digit(c));
+ } while (!subtype && is_digit(c));
} else if (c != '}') {
int cc = c;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
parser: Fix old-style command substitution here-document crash
On Wed, Jul 25, 2018 at 12:38:27PM +, project-repo wrote:
> Hi,
> I am working on a project in which I use the honggfuzz fuzzer to fuzz open
> source software and I decided to fuzz dash. In doing so I discovered a
> NULL pointer dereference in src/redir.ch on line 305. Following is a
> backtrace as supplied by the address sanitizer:
>
> AddressSanitizer:DEADLYSIGNAL
> =
> ==39623==ERROR: AddressSanitizer: SEGV on unknown address 0x0010 (pc
> 0x005768ed bp 0x7ffc00273df0 sp 0x7ffc00273c60 T0)
> ==39623==The signal is caused by a READ memory access.
> ==39623==Hint: address points to the zero page.
> #0 0x5768ec in openhere /home/jfe/dash/src/redir.c:305:29
> #1 0x574d92 in openredirect /home/jfe/dash/src/redir.c:230:7
> #2 0x5737fe in redirect /home/jfe/dash/src/redir.c:121:11
> #3 0x576017 in redirectsafe /home/jfe/dash/src/redir.c:424:3
> #4 0x522326 in evalcommand /home/jfe/dash/src/eval.c:828:11
> #5 0x520010 in evaltree /home/jfe/dash/src/eval.c:288:12
> #6 0x5270da in evaltreenr /home/jfe/dash/src/eval.c:332:2
> #7 0x526f04 in evalbackcmd /home/jfe/dash/src/eval.c:640:3
> #8 0x539020 in expbackq /home/jfe/dash/src/expand.c:522:2
> #9 0x5332d7 in argstr /home/jfe/dash/src/expand.c:343:4
> #10 0x5322f7 in expandarg /home/jfe/dash/src/expand.c:196:2
> #11 0x528118 in fill_arglist /home/jfe/dash/src/eval.c:659:3
> #12 0x5213b6 in evalcommand /home/jfe/dash/src/eval.c:769:13
> #13 0x520010 in evaltree /home/jfe/dash/src/eval.c:288:12
> #14 0x554423 in cmdloop /home/jfe/dash/src/main.c:234:8
> #15 0x553bcc in main /home/jfe/dash/src/main.c:176:3
> #16 0x7f201c2b2a86 in __libc_start_main
> (/lib/x86_64-linux-gnu/libc.so.6+0x21a86)
> #17 0x41dfb9 in _start (/home/jfe/dash/src/dash+0x41dfb9)
>
> AddressSanitizer can not provide additional info.
> SUMMARY: AddressSanitizer: SEGV /home/jfe/dash/src/redir.c:305:29 in openhere
> ==39623==ABORTING
>
> This bug can be reproduced by running "dash < min" where min is þhe file
> attached. I was able to reproduce this bug with the current git version
> and the current debian version.
>
> cheers
> project-repo
>
> < `<
Fixes: 51e2d88d6e51 ("parser: Save/restore here-documents in...")
Signed-off-by: Herbert Xu
diff --git a/src/parser.c b/src/parser.c
index 1f9e8ec..4bda42e 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1451,9 +1451,9 @@ done:
if (readtoken() != TRP)
synexpect(TRP);
setinputstring(nullstr);
- parseheredoc();
}
+ parseheredoc();
heredoclist = saveheredoclist;
(*nlpp)->n = n;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: -e not ignored in substition under tests
On Fri, Jun 22, 2018 at 10:41:43PM +0800, Herbert Xu wrote: > Benedikt Becker wrote: > > Hi, > > > > Dash does not ignore the -e option in substitutions $(...) under AND/OR > > lists and conditions. For example, in the program > > > > set -e > > echo "$(echo X; false; echo Y)" || true > > > > the command `false` seems to interrupt the sequence and only X is printed > > (tested with dash 0.5.8.2 and git master). But according to my > > understanding of the POSIX specifications, `set -e` should be ignored under > > the OR list and X and Y should be printed . The same behaviour can be > > observed when using the substitution in the condition of a while, until, > > and if. In contrast, set -e is effectively ignored in normal subshells > > under AND/OR lists and conditions (e.g., `set -e; (echo X; false; echo Y) > > || true` prints X and Y). > > > > Is this a bug in dash or in my understanding? > > I'd say that it's a bug. Looking at this again it would appear that the pdksh lineage also behaves like dash (and presumably other ash shells). So I'd like to hear other opinions first before changing this. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[v2 PATCH] eval: Reset handler when entering a subshell
On Sat, Mar 02, 2019 at 01:28:44PM +, Harald van Dijk wrote:
>
> That is *a* real bug here, not *the* real bug. This leaves the buggy
> behaviour of the "command" built-in intact in the test case I included in
> the message you replied to.
I don't quite understand. Could you explain what is still buggy
after the following patch?
> For fixing the one bug, it is a sensible approach, but keep in mind that
> when a fork is omitted because of EV_EXIT, so too will the reset of handler.
> You may be able to get away with that as long as you do not propagate
> EV_EXIT in any cases where a cleanup action might cause problems.
Good point. Here is a revised patch to fix the nofork case too.
---8<--
As it is a subshell can execute code that is only meant for the
parent shell when it executes a longjmp that is caught by something
like evalcommand. This patch fixes it by resetting the handler
when entering a subshell.
Reported-by: Martijn Dekker
Signed-off-by: Herbert Xu
diff --git a/src/eval.c b/src/eval.c
index 1aad31a..6ee2e1a 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -41,6 +41,7 @@
* Evaluate a command.
*/
+#include "main.h"
#include "shell.h"
#include "nodes.h"
#include "syntax.h"
@@ -492,6 +493,7 @@ evalsubshell(union node *n, int flags)
if (backgnd)
flags &=~ EV_TESTED;
nofork:
+ reset_handler();
redirect(n->nredir.redirect, 0);
evaltreenr(n->nredir.n, flags);
/* never returns */
@@ -574,6 +576,7 @@ evalpipe(union node *n, int flags)
}
}
if (forkshell(jp, lp->n, n->npipe.backgnd) == 0) {
+ reset_handler();
INTON;
if (pip[1] >= 0) {
close(pip[0]);
@@ -630,6 +633,7 @@ evalbackcmd(union node *n, struct backcmd *result)
sh_error("Pipe call failed");
jp = makejob(n, 1);
if (forkshell(jp, n, FORK_NOJOB) == 0) {
+ reset_handler();
FORCEINTON;
close(pip[0]);
if (pip[1] != 1) {
diff --git a/src/main.c b/src/main.c
index 6b3a090..b2712cb 100644
--- a/src/main.c
+++ b/src/main.c
@@ -71,6 +71,7 @@ int *dash_errno;
short profile_buf[16384];
extern int etext();
#endif
+static struct jmploc main_handler;
STATIC void read_profile(const char *);
STATIC char *find_dot_file(char *);
@@ -90,7 +91,6 @@ main(int argc, char **argv)
{
char *shinit;
volatile int state;
- struct jmploc jmploc;
struct stackmark smark;
int login;
@@ -102,7 +102,7 @@ main(int argc, char **argv)
monitor(4, etext, profile_buf, sizeof profile_buf, 50);
#endif
state = 0;
- if (unlikely(setjmp(jmploc.loc))) {
+ if (unlikely(setjmp(main_handler.loc))) {
int e;
int s;
@@ -137,7 +137,7 @@ main(int argc, char **argv)
else
goto state4;
}
- handler =
+ handler = _handler;
#ifdef DEBUG
opentrace();
trputs("Shell args: "); trargs(argv);
@@ -353,3 +353,8 @@ exitcmd(int argc, char **argv)
exraise(EXEXIT);
/* NOTREACHED */
}
+
+void reset_handler(void)
+{
+ handler = _handler;
+}
diff --git a/src/main.h b/src/main.h
index 19e4983..51f1604 100644
--- a/src/main.h
+++ b/src/main.h
@@ -52,3 +52,4 @@ extern int *dash_errno;
void readcmdfile(char *);
int dotcmd(int, char **);
int exitcmd(int, char **);
+void reset_handler(void);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] shell: Reset handler when entering a subshell
On Fri, Jan 11, 2019 at 11:25:48PM +, Harald van Dijk wrote:
> On 10/01/2019 14:21, Martijn Dekker wrote:
> > Op 10-01-19 om 11:37 schreef Martijn Dekker:
> > > In a dot script sourced with 'command .' (which is useful to avoid
> > > exiting if the script doesn't exist), triggering a syntax error in an
> > > 'eval' in a subshell causes dash to hang at the end of the main script.
> >
> > In fact, 'eval' doesn't appear related. I can also trigger the bug by
> > triggering an error in another special builtin:
> >
> > command . /dev/stdin < > ( set -o foobar ) && echo WOOPS
> > EOF
> > echo end
>
> I do not see a hang myself, but I definitely see wrong behaviour: the output
> I get is
>
> $ command . /dev/stdin < > ( set -o foobar ) && echo WOOPS
> > EOF
> src/dash: 1: set: Illegal option -o foobar
> $ echo end
> end
> $
> WOOPS
> $
>
> This was introduced by
>
> commit 46d5a7fcea81b489819f753451c1ad2fe435f148
> Author: Herbert Xu
> Date: Tue Mar 27 00:39:35 2018 +0800
>
> eval: Restore input files in evalcommand
>
> 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.
>
> What I think it going on here, although I do not understand it completely
> yet, is:
>
> evalcommand invokes a command that modifies parsefile: that's the dot
> command. The evaluation of the dotted file involves creating a subshell, and
> because of the invalid option, that subshell exits with EXERROR. Because the
> subshell is invoked from within the command builtin, that EXERROR is eaten,
> and the input file is restored. The subshell then happily continues reading
> commands at the same time as the parent shell.
Thanks for the analysis. I think the real bug here is the fact
that subshells can "escape" their jail by a longjmp that is caught
by evalcommand.
This is something that both NetBSD/FreeBSD have already fixed.
Here is a similar patch to fix it in dash:
---8<---
As it is a subshell can execute code that is only meant for the
parent shell when it executes a longjmp that is caught by something
like evalcommand. This patch fixes it by resetting the handler
when entering a subshell.
Reported-by: Martijn Dekker
Signed-off-by: Herbert Xu
diff --git a/src/error.h b/src/error.h
index 94e30a2..c5db134 100644
--- a/src/error.h
+++ b/src/error.h
@@ -34,6 +34,9 @@
* @(#)error.h 8.2 (Berkeley) 5/4/95
*/
+#ifndef _SRC_ERROR_H
+#define _SRC_ERROR_H
+
#include
#include
@@ -127,3 +130,5 @@ void exerror(int, const char *, ...)
__attribute__((__noreturn__));
const char *errmsg(int, int);
void sh_warnx(const char *, ...);
+
+#endif /* _SRC_ERROR_H */
diff --git a/src/jobs.c b/src/jobs.c
index 26a6248..80ae742 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -952,9 +952,10 @@ forkshell(struct job *jp, union node *n, int mode)
TRACE(("forkshell(%%%d, %p, %d) called\n", jobno(jp), n, mode));
pid = fork();
- if (pid == 0)
+ if (pid == 0) {
+ handler = _handler;
forkchild(jp, n, mode);
- else
+ } else
forkparent(jp, n, mode, pid);
return pid;
diff --git a/src/main.c b/src/main.c
index 6b3a090..5346ef6 100644
--- a/src/main.c
+++ b/src/main.c
@@ -71,6 +71,7 @@ int *dash_errno;
short profile_buf[16384];
extern int etext();
#endif
+struct jmploc main_handler;
STATIC void read_profile(const char *);
STATIC char *find_dot_file(char *);
@@ -90,7 +91,6 @@ main(int argc, char **argv)
{
char *shinit;
volatile int state;
- struct jmploc jmploc;
struct stackmark smark;
int login;
@@ -102,7 +102,7 @@ main(int argc, char **argv)
monitor(4, etext, profile_buf, sizeof profile_buf, 50);
#endif
state = 0;
- if (unlikely(setjmp(jmploc.loc))) {
+ if (unlikely(setjmp(main_handler.loc))) {
int e;
int s;
@@ -137,7 +137,7 @@ main(int argc, char **argv)
else
goto state4;
}
- handler =
+ handler = _handler;
#ifdef DEBUG
opentrace();
trputs("Shell args: "); trargs(argv);
diff --git a/src/main.h b/src/main.h
index 19e4983..d54e10d 100644
--- a/src/main.h
+++ b/src/main.h
@@ -35,6 +35,7 @@
*/
#include
+#include "error.h"
/* pid of main shell */
extern int rootpid;
@@ -49,6 +50,8 @@ extern int *dash_errno;
#define errno (*dash_errno)
#endif
+extern struct jmploc main_handler;
+
void readcmdfile(char *);
int dotcmd(int, char **);
int exitcmd(int, char **);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] expand: Fix double-decrement in argstr
On Sat, Feb 16, 2019 at 10:26:23PM +, Martijn Dekker wrote:
> For unset foo:
>
> dash-git
> $ src/dash -c 'set -- ${foo- bar }; echo "[$1]"'
> [bar ]
>
> Release versions of dash and all other shells output:
> [bar]
>
> The change in behaviour appears to have been introduced by commit
> 3cd5386 ("expand: Do not reprocess data when expanding words").
Thanks for catching this!
---8<---
Due to a double decrement in argstr we may miss field separators
at the end of a word in certain situations.
Reported-by: Martijn Dekker
Fixes: 3cd538634f71 ("expand: Do not reprocess data when...")
Signed-off-by: Herbert Xu
diff --git a/src/expand.c b/src/expand.c
index af9cac9..e57efa6 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -285,7 +285,7 @@ start:
q = stnputs(p, length, expdest);
q[-1] &= end - 1;
expdest = q - (flag & EXP_WORD ? end : 0);
- newloc = expdest - (char *)stackblock() - end;
+ newloc = q - (char *)stackblock() - end;
if (breakall && !inquotes && newloc > startloc) {
recordregion(startloc, newloc, 0);
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 1/2] shell: Fix clang warnings about "string plus integer"
On Sat, Dec 15, 2018 at 06:49:31PM +0100, Antonio Ospite wrote: > Building with clang results in some warnings about integer values being > added to strings: > > --- > eval.c:1138:13: warning: adding 'int' to a string does not append to the > string [-Wstring-plus-int] > p = " %s" + (1 - sep); > ~~^~~ > eval.c:1138:13: note: use array indexing to silence this warning > p = " %s" + (1 - sep); > ^ > & [ ] > 1 warning generated. > > ... > > jobs.c:1424:16: warning: adding 'int' to a string does not append to the > string [-Wstring-plus-int] > str = "\"}" + !(quoted & 1); > ~~^~~ > jobs.c:1424:16: note: use array indexing to silence this warning > str = "\"}" + !(quoted & 1); > ^ > & [ ] > 1 warning generated. > --- > > While the code itself is fine and the warnings are indeed harmless, > fixing them also makes the semantic more explicit: what it is actually > being increased is the address which points to the start of the string > in order to skip the initial character when some conditions are met. > > Signed-off-by: Antonio Ospite > --- > src/eval.c | 3 ++- > src/jobs.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] options: Do not set commandname in procargs
On Mon, Jan 28, 2019 at 03:58:24PM +, Olivier Duclos wrote:
> Hi,
>
> This is a small cosmetic patch which fixes something that annoyed me for a
> long time: the repetition of the script name in error messages.
>
> Example:
>
> $ cat test.sh
> #!/bin/sh
> echo ${x:?}
>
> $ dash test.sh
> /home/odc/test.sh: 2: /home/odc/test.sh: x: parameter not set or null
>
> With the patch:
>
> $ dash test.sh
> /home/odc/test.sh: 2: x: parameter not set or null
>
> This is the same behavior as bash.
Thanks for the report!
How about this patch?
---8<---
We set commandname in procargs when we don't have to. This results
in a duplicated output of arg0 when an error occurs.
Reported-by: Olivier Duclos
Signed-off-by: Herbert Xu
diff --git a/src/options.c b/src/options.c
index 6f381e6..a46c23b 100644
--- a/src/options.c
+++ b/src/options.c
@@ -159,7 +159,6 @@ procargs(int argc, char **argv)
setinputfile(*xargv, 0);
setarg0:
arg0 = *xargv++;
- commandname = arg0;
}
shellparam.p = xargv;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH v4] redir: Handle nested exec within REALLY_CLOSED redirection
On Thu, Jan 17, 2019 at 11:07:23PM +0100, Martijn Dekker wrote:
> Op 16-01-19 om 14:32 schreef Herbert Xu:
> > So I'm going to go for a more complicated fix:
>
> The v3 patch introduces a bug:
>
> begin test script
> init() {
> exec 8 } >&-
>
> {
> init
> : <&8 && echo ok
> } 8<&-
> end test script
>
> Actual output:
> test2.sh: 9: test2.sh: 8: Bad file descriptor
>
> Expected output:
> ok
>
> The 'read' gets a 'bad file descriptor', so the effect of the 'exec'
> from the init function is lost.
>
> Interestingly, removing either the ">&-" from the function definition
> block, or the "8<&-" from the other braces block, or both, makes the
> error go away.
Thanks for testing this.
Yes my patch was completely broken. We need to track the current
status separately from the previous state which is what redirlist
does.
---8<---
The value of REALLY_CLOSED is used to avoid an unnecessary close(2)
call when restoring redirections. However, as it stands it can
remove a close(2) call that's actually needed. This happens when
an enclosed exec(1) command leaves an open file descriptor behind.
This patch fixes this by replacing REALLY_CLOSED with closed_redirs
to track the current status of redirected file descriptors and
leaving redirlist to only handle the previous state of redirected
file descriptors.
Reported-by: Martijn Dekker
Fixes: ce0f1900d869 ("[REDIR] Fix redirect restore on saved file...")
Signed-off-by: Herbert Xu
diff --git a/src/redir.c b/src/redir.c
index e67cc0a..6c81dd0 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -57,7 +57,6 @@
#include "error.h"
-#define REALLY_CLOSED -3 /* fd that was closed and still is */
#define EMPTY -2 /* marks an unused slot in redirtab */
#define CLOSED -1 /* fd opened for redir needs to be closed */
@@ -77,6 +76,9 @@ struct redirtab {
MKINIT struct redirtab *redirlist;
+/* Bit map of currently closed file descriptors. */
+static unsigned closed_redirs;
+
STATIC int openredirect(union node *);
#ifdef notyet
STATIC void dupredirect(union node *, int, char[10]);
@@ -86,6 +88,20 @@ STATIC void dupredirect(union node *, int);
STATIC int openhere(union node *);
+static unsigned update_closed_redirs(int fd, int nfd)
+{
+ unsigned val = closed_redirs;
+ unsigned bit = 1 << fd;
+
+ if (nfd >= 0)
+ closed_redirs &= ~bit;
+ else
+ closed_redirs |= bit;
+
+ return val & bit;
+}
+
+
/*
* Process a list of redirection commands. If the REDIR_PUSH flag is set,
* old file descriptors are stashed away so that the redirection can be
@@ -125,21 +141,21 @@ redirect(union node *redir, int flags)
fd = n->nfile.fd;
if (sv) {
+ int closed;
+
p = >renamed[fd];
i = *p;
+ closed = update_closed_redirs(fd, newfd);
+
if (likely(i == EMPTY)) {
i = CLOSED;
- if (fd != newfd) {
+ if (fd != newfd && !closed) {
i = savefd(fd, fd);
fd = -1;
}
}
- if (i == newfd)
- /* Can only happen if i == newfd == CLOSED */
- i = REALLY_CLOSED;
-
*p = i;
}
@@ -346,14 +362,18 @@ popredir(int drop)
INTOFF;
rp = redirlist;
for (i = 0 ; i < 10 ; i++) {
+ int closed;
+
+ if (rp->renamed[i] == EMPTY)
+ continue;
+
+ closed = drop ? 1 : update_closed_redirs(i, rp->renamed[i]);
+
switch (rp->renamed[i]) {
case CLOSED:
- if (!drop)
+ if (!closed)
close(i);
break;
- case EMPTY:
- case REALLY_CLOSED:
- break;
default:
if (!drop)
dup2(rp->renamed[i], i);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v3] redir: Handle nested exec within REALLY_CLOSED redirection
On Wed, Jan 16, 2019 at 10:29:50PM +, Harald van Dijk wrote: > > Still, if that's slow enough and happens commonly enough that it's worth > avoiding, it seems like it would still be simpler, shorter and probably > faster to just write > > if (fd != -1) > close(fd); > > in a few places, since we know that the only invalid fd that ever gets > passed to close() is -1. That avoids the other cases where dash already > happily calls close(-1) as well. The issue is not close(-1), it's close(8) for 8>&- where we already know that fd 8 was closed to start with (because the first dup returned -1 on it). Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH v3] redir: Handle nested exec within REALLY_CLOSED redirection
On Fri, Dec 14, 2018 at 01:01:12AM +, Martijn Dekker wrote:
>
> > So did anything happen on the bash front? I'm happy to change if
> > bash moves in the same direction.
>
> Yes. According to the bash changelog, Chet fixed it in git last 30th of
> April, meaning it'll be in bash 5.0.
>
> Patch attached, as per Harald's suggestion.
Thanks for the patch. I took a deeper look into the history of
the bug and it turned out that I added REALLY_CLOSED as an
optimisation in order to avoid an unnecessary close(2) syscall.
Removing REALLY_CLOSED would mean an unnecessary close(2) in
such cases.
So I'm going to go for a more complicated fix:
---8<---
The value of REALLY_CLOSED is used to avoid an unnecessary close(2)
call when restoring redirections. However, as it stands it can
remove a close(2) call that's actually needed. This happens when
an enclosed exec(1) command leaves an open file descriptor behind.
This patch fixes this by going up one level when popping redirections
and changing REALLY_CLOSED back to CLOSED when we encounter the
nested exec(1) command.
Reported-by: Martijn Dekker
Fixes: ce0f1900d869 ("[REDIR] Fix redirect restore on saved file...")
Signed-off-by: Herbert Xu
diff --git a/src/redir.c b/src/redir.c
index e67cc0a..4f84a80 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -128,12 +128,12 @@ redirect(union node *redir, int flags)
p = >renamed[fd];
i = *p;
- if (likely(i == EMPTY)) {
- i = CLOSED;
- if (fd != newfd) {
+ if (likely(i <= EMPTY)) {
+ if (i == EMPTY && fd != newfd) {
i = savefd(fd, fd);
fd = -1;
- }
+ } else
+ i = CLOSED;
}
if (i == newfd)
@@ -340,16 +340,20 @@ out:
void
popredir(int drop)
{
+ struct redirtab *nrp;
struct redirtab *rp;
int i;
INTOFF;
rp = redirlist;
+ nrp = rp->next;
for (i = 0 ; i < 10 ; i++) {
switch (rp->renamed[i]) {
case CLOSED:
if (!drop)
close(i);
+ else if (nrp && nrp->renamed[i] == REALLY_CLOSED)
+ nrp->renamed[i] = CLOSED;
break;
case EMPTY:
case REALLY_CLOSED:
@@ -361,7 +365,7 @@ popredir(int drop)
break;
}
}
- redirlist = rp->next;
+ redirlist = nrp;
ckfree(rp);
INTON;
}
@@ -451,8 +455,17 @@ struct redirtab *pushredir(union node *redir)
sv = ckmalloc(sizeof (struct redirtab));
sv->next = q;
redirlist = sv;
- for (i = 0; i < 10; i++)
- sv->renamed[i] = EMPTY;
+ for (i = 0; i < 10; i++) {
+ int val = EMPTY;
+
+ if (q) {
+ val = q->renamed[i];
+ if (val > EMPTY)
+ val = EMPTY;
+ }
+
+ sv->renamed[i] = val;
+ }
out:
return q;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On Fri, Dec 14, 2018 at 09:37:09AM +, Harald van Dijk wrote: > > Still, that works as well, doesn't it? The control flow is basically the > same so the logic in my other message applies. returncmd() doesn't use > exceptions, so you get to dotrap() which already resets exitstatus if > necessary. returncmd itself doesn't jump up but it may be part of a subshell which would execute a jump to the top as part of the EV_EXIT optimisation. That's where you'd need to restore the exit status. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On Fri, Dec 14, 2018 at 08:02:04AM +, Harald van Dijk wrote:
> On 14/12/2018 03:10, Herbert Xu wrote:
>
> > The reason this is needed is to support a naked return. With
> > your patch a naked return would either have to always return the
> > saved status or the new status. Neither of which is what we
> > want.
>
> I actually saw the commit you referenced already and tested that with my
> patch before sending. This is how I tested it:
>
> Returns 1:
>
> f() {
> false; return
> }
> f
That's not a naked return. A naked return is one used outside of
a function. I know this is unspecified under POSIX, but for dash
this has a specific meaning and that is the return is equivalent
to exit.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] eval: make traps work when "set -e" is enabled
Antonio Ospite wrote: > When "set -e" is enabled traps are not always executed, in particular > the EXIT trap is not executed when the shell exits on an unhandled > error. > > Consider the following test script: > > #!/bin/dash > > set -e > > trap 'ret=$?; echo "EXIT: $ret"' EXIT > trap 'exit 2' HUP INT QUIT PIPE TERM > > read variable > > By pressing Ctrl-C one would expect the EXIT trap to be called, as it is > the case with other shells (bash, zsh), but dash does not do it. > > By calling dotrap() before jumping to the exit path when checkexit is > not zero, dash behaves like other shells. > > Signed-off-by: Antonio Ospite > --- > > Hi, > > this has been reported in Debian[1] and I noticed the issue myself too, so > I tried to take a look at it. > > I am marking the patch as RFC because I don't know the dash codebase very > well, and I might not be aware of possible drawbacks of this change. It worked > in my limited testing but that's it. > > I don't know if the behavior of traps is specified when "set -e" is active, > but in case it isn't it would stll be good to behave like other shells. > > Any comment is welcome. > > Thank you, > Antonio > > > [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=779416 > > > src/eval.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 0/5] Build system updates and gcc warnings fixes
Antonio Ospite wrote: > Hi, > > here are some build system updates and some fixes for compilation > warnings with Gcc. > > After this patchset, compilation with Gcc is nice and clean, > > There are still some warnings when compiling with clang but I have not > addressed those. > > Thank you. > Antonio > > Antonio Ospite (5): > Update configure.ac with suggestions from autoupdate > Enable automake silent rules > Add some missing autotools files to the .gitignore file > Stop using deprecated function sigsetmask() > Silence compiler warning about missing parentheses > > .gitignore | 2 ++ > configure.ac| 8 +--- > src/Makefile.am | 16 > src/eval.c | 2 +- > src/system.h| 4 > 5 files changed, 16 insertions(+), 16 deletions(-) Patches 1, 2 and 5 applied. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[v3 PATCH] eval: Only restore exit status on exit/return
Herbert Xu wrote:
>
> Here is a new patch that fixes the reported issue for both signal
> traps and the EXIT trap.
This introduced a warning in trap.c. Here is an update to silence
the warning.
---8<---
We unconditionally restore the saved status in exitreset, which
is incorrect as we only want to do it for exitcmd and returncmd.
This patch fixes the problem by introducing EXEND.
Reported-by: Martijn Dekker
Fixes: da30b4b78769 ("[BUILTIN] Exit without arguments in a trap...")
Signed-off-by: Herbert Xu
diff --git a/src/error.h b/src/error.h
index 9630b56..94e30a2 100644
--- a/src/error.h
+++ b/src/error.h
@@ -66,7 +66,8 @@ extern int exception;
/* exceptions */
#define EXINT 0/* SIGINT received */
#define EXERROR 1 /* a generic error */
-#define EXEXIT 4 /* exit the shell */
+#define EXEND 3/* exit the shell */
+#define EXEXIT 4 /* exit the shell via exitcmd */
/*
diff --git a/src/eval.c b/src/eval.c
index f45e2e2..eaff657 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -114,12 +114,13 @@ STATIC const struct builtincmd bltin = {
INCLUDE "eval.h"
EXITRESET {
- evalskip = 0;
- loopnest = 0;
if (savestatus >= 0) {
- exitstatus = savestatus;
+ if (exception == EXEXIT || evalskip == SKIPFUNCDEF)
+ exitstatus = savestatus;
savestatus = -1;
}
+ evalskip = 0;
+ loopnest = 0;
}
#endif
@@ -314,7 +315,7 @@ out:
if (flags & EV_EXIT) {
exexit:
- exraise(EXEXIT);
+ exraise(EXEND);
}
return exitstatus;
diff --git a/src/exec.c b/src/exec.c
index 9d0215a..87354d4 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -143,7 +143,7 @@ shellexec(char **argv, const char *path, int idx)
exitstatus = exerrno;
TRACE(("shellexec failed for %s, errno %d, suppressint %d\n",
argv[0], e, suppressint ));
- exerror(EXEXIT, "%s: %s", argv[0], errmsg(e, E_EXEC));
+ exerror(EXEND, "%s: %s", argv[0], errmsg(e, E_EXEC));
/* NOTREACHED */
}
diff --git a/src/main.c b/src/main.c
index 6d53e00..6b3a090 100644
--- a/src/main.c
+++ b/src/main.c
@@ -111,7 +111,7 @@ main(int argc, char **argv)
e = exception;
s = state;
- if (e == EXEXIT || s == 0 || iflag == 0 || shlvl)
+ if (e == EXEND || e == EXEXIT || s == 0 || iflag == 0 || shlvl)
exitshell();
reset();
diff --git a/src/trap.c b/src/trap.c
index ab0ecd4..58a7c60 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -41,6 +41,7 @@
#include "main.h"
#include "nodes.h" /* for other headers */
#include "eval.h"
+#include "init.h"
#include "jobs.h"
#include "show.h"
#include "options.h"
@@ -397,8 +398,10 @@ exitshell(void)
trap[0] = NULL;
evalskip = 0;
evalstring(p, 0);
+ evalskip = SKIPFUNCDEF;
}
out:
+ exitreset();
/*
* Disable job control so that whoever had the foreground before we
* started can get it back.
@@ -406,7 +409,7 @@ out:
if (likely(!setjmp(loc.loc)))
setjobctl(0);
flushall();
- _exit(savestatus);
+ _exit(exitstatus);
/* NOTREACHED */
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Make mktokens accept a random TMPDIR, replace `...` with $(...).
Devin Hussey wrote: > From 31afca233f67dde67181efd7ed594cd2c25fefa6 Mon Sep 17 00:00:00 2001 > From: Devin Hussey > Date: Thu, 15 Nov 2018 10:30:05 -0500 > Subject: [PATCH] Make mktokens accept a random TMPDIR, replace `...` with > $(...). > > Sorry about the multiple commits at once. > > Signed-off-by: Devin Hussey > --- > src/mktokens | 17 ++--- > 1 file changed, 10 insertions(+), 7 deletions(-) Thanks for the patch. For whatever the current patchwork no longer accepts patches sent as replies to an existing patch. So please resubmit this patch under its own Subject. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Use a real non-cryptographic hash algorithm
Devin Hussey wrote: > From 502cbd61e386eb014035df66b032e90a9de926b7 Mon Sep 17 00:00:00 2001 > From: Devin Hussey > Date: Thu, 15 Nov 2018 09:12:24 -0500 > Subject: [PATCH] Use a real non-cryptographic hash algorithm > > dash previously used a modified version of the LoseLose algorithm. > > LoseLose is a TERRIBLE algorithm. > > It has terrible distribution, leaving many hash entries unused. > > However, more importantly, it is incredibly easy to make a > collision; even something as simple as rearranging the letters. > > For example. these all produce the hash 1652: > > Hello > Holle > Hlleo > Hoell > Haxed\n > HASH\tBAD > Hater > > Collsions in hash tables are very bad because it requires > looking through a linked list, and the benefits of O(1) time > in a hash table are completely lost. > > The FNV-1a algorithm has comparable performance and code size > while having a much better collision rate. > > While there are some other faster and more resistant hashes out there, > namely xxHash, Murmur2, and CityHash, the benefits are minimal on > short strings and they expect a length argument, unlike FNV-1a which > simply uses null-termination, and they are not in the public domain > like FNV-1a. > > Signed-off-by: Devin Hussey I'm confused. What problem are you trying to solve? If it's security against an adversary, then clearly your replacement isn't up to stratch either. For real security, you'd need a strong algorithm and periodic rehashing. If it's hash collisions with a real-world script, please provide an example. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] var: ensure variables are fully initialised when unset
Harald van Dijk wrote:
> [-- text/plain, encoding 7bit, charset: utf-8, 29 lines --]
>
> On 12/11/2018 12:53, Ron Yorston wrote:
>> When a variable is unset by calling setvar(name, 0, 0) the code
>> to initialise the new, empty variable omits the trailing '='.
>
> It's supposed to. A trailing = means the variable is set to an empty
> string. That's different from unset. You can see the difference with
> set -u, or with ${var+set}. However, ...
>
>> Attempts to read the contents of the unset variable will result
>> in the uninitialised character at the end of the string being
>> accessed.
>
> ...this is indeed a bug which I've noticed as well. The code needs two
> trailing null bytes, not just one. Because of glibc internals, the
> out-of-bounds byte being read will almost certainly be zero on x86-64,
> but it's not a guarantee, and it could probably break more easily on
> other platforms.
>
> It only affects shell-internal uses of variables, only for variables
> explicitly unset by a script (rather than unset by default), only for
> uses where the code does not explicitly check for unset beforehand. As
> far as scripts go, that just means PATH (as you found) I think, for
> interactive shells there are some more variables such as PS1/PS2/PS4/MAIL.
>
> My patch is attached.
Thanks for the patch Harald!
Could you please repost it with a new Subject line? patchwork
is no longer picking up patches posted as a reply.
Please also add a Signed-off-by tag.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On Thu, Dec 06, 2018 at 09:35:47PM +, Harald van Dijk wrote: > On 04/12/2018 23:57, Harald van Dijk wrote: > > This has the benefit of fixing one other test case, a small modification > > from one of Martijn Dekker's: > > > > $SHELL -c 'trap "set -o bad@option" INT; kill -s INT $$' && echo BUG > > Another test case, one that still fails: > > trap exit INT > trap 'true; kill -s INT $$' EXIT > false > > Here, inside the EXIT handler, "the command that executed immediately > preceding the trap action" is `false`, but inside the INT handler, it's > either `true` or `kill -s INT $$` (I think the latter, but it doesn't > matter). dash treats it as if it were still `false`. I think this makes sense. The EXIT trap trumps whatever happens inside it. FWIW ksh/mksh both do the same thing. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] eval: Only restore exit status on exit/return
On Sat, Dec 08, 2018 at 03:45:11PM +, Harald van Dijk wrote:
>
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -116,10 +116,7 @@ INCLUDE "eval.h"
> EXITRESET {
> evalskip = 0;
> loopnest = 0;
> - if (savestatus >= 0) {
> - exitstatus = savestatus;
> - savestatus = -1;
> - }
> + savestatus = -1;
> }
> #endif
The reason this is needed is to support a naked return. With
your patch a naked return would either have to always return the
saved status or the new status. Neither of which is what we
want.
Please refer to
commit 70c16dd30d4cf824aa895e9f6c095fec741c65a8
Author: Herbert Xu
Date: Mon Oct 6 21:51:26 2014 +0800
[BUILTIN] Return without arguments in a trap should use status outside traps
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] clear_traps: reset savestatus
Harald van Dijk wrote: > > I had intended it as a testcase for dash with your change applied, in > which case it fails even without making sure of that since you hit > _exit(savestatus) with savestatus reset to -1, but I like that it's so > easy to modify to also fail on unpatched dash, so thanks for that. > > By the way, my change has an unintended but possibly acceptable side effect: > > trap '(trap "echo exit" EXIT; :)' EXIT > > This prints nothing with current dash, but prints "exit" with my change. > It also prints "exit" in ksh, mksh, posh, and bosh. I'm happy to allow exit traps to be processed here. But we should be able to achieve the same result without jumping back to main, by simply jumping back to the start of exitshell instead. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[v2 PATCH] eval: Only restore exit status on exit/return
On Sat, Dec 01, 2018 at 04:45:39PM +0800, Herbert Xu wrote:
>
> Hmm, I think this breaks the following case which used to work:
>
> dash -c 'trap "(:; exit) && echo BUG" EXIT; false'
>
> I know this makes no sense but almost every other shell does it
> this way.
Here is a new patch that fixes the reported issue for both signal
traps and the EXIT trap.
---8<---
We unconditionally restore the saved status in exitreset, which
is incorrect as we only want to do it for exitcmd and returncmd.
This patch fixes the problem by introducing EXEND.
Reported-by: Martijn Dekker
Fixes: da30b4b78769 ("[BUILTIN] Exit without arguments in a trap...")
Signed-off-by: Herbert Xu
diff --git a/src/error.h b/src/error.h
index 9630b56..94e30a2 100644
--- a/src/error.h
+++ b/src/error.h
@@ -66,7 +66,8 @@ extern int exception;
/* exceptions */
#define EXINT 0/* SIGINT received */
#define EXERROR 1 /* a generic error */
-#define EXEXIT 4 /* exit the shell */
+#define EXEND 3/* exit the shell */
+#define EXEXIT 4 /* exit the shell via exitcmd */
/*
diff --git a/src/eval.c b/src/eval.c
index 546ee1b..3cea2a3 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -114,12 +114,13 @@ STATIC const struct builtincmd bltin = {
INCLUDE "eval.h"
EXITRESET {
- evalskip = 0;
- loopnest = 0;
if (savestatus >= 0) {
- exitstatus = savestatus;
+ if (exception == EXEXIT || evalskip == SKIPFUNCDEF)
+ exitstatus = savestatus;
savestatus = -1;
}
+ evalskip = 0;
+ loopnest = 0;
}
#endif
@@ -314,7 +315,7 @@ out:
if (flags & EV_EXIT) {
exexit:
- exraise(EXEXIT);
+ exraise(EXEND);
}
return exitstatus;
diff --git a/src/exec.c b/src/exec.c
index 9d0215a..87354d4 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -143,7 +143,7 @@ shellexec(char **argv, const char *path, int idx)
exitstatus = exerrno;
TRACE(("shellexec failed for %s, errno %d, suppressint %d\n",
argv[0], e, suppressint ));
- exerror(EXEXIT, "%s: %s", argv[0], errmsg(e, E_EXEC));
+ exerror(EXEND, "%s: %s", argv[0], errmsg(e, E_EXEC));
/* NOTREACHED */
}
diff --git a/src/main.c b/src/main.c
index 6d53e00..6b3a090 100644
--- a/src/main.c
+++ b/src/main.c
@@ -111,7 +111,7 @@ main(int argc, char **argv)
e = exception;
s = state;
- if (e == EXEXIT || s == 0 || iflag == 0 || shlvl)
+ if (e == EXEND || e == EXEXIT || s == 0 || iflag == 0 || shlvl)
exitshell();
reset();
diff --git a/src/trap.c b/src/trap.c
index ab0ecd4..64917fc 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -397,8 +397,10 @@ exitshell(void)
trap[0] = NULL;
evalskip = 0;
evalstring(p, 0);
+ evalskip = SKIPFUNCDEF;
}
out:
+ exitreset();
/*
* Disable job control so that whoever had the foreground before we
* started can get it back.
@@ -406,7 +408,7 @@ out:
if (likely(!setjmp(loc.loc)))
setjobctl(0);
flushall();
- _exit(savestatus);
+ _exit(exitstatus);
/* NOTREACHED */
}
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: eval: Only restore exit status on exit/return
On Thu, Nov 29, 2018 at 03:27:31PM +0100, Martijn Dekker wrote: > Op 29-11-18 om 12:33 schreef Herbert Xu: > > Thanks for the report. This patch should fix the problem: > > Doesn't work for me, in fact it seems to make no difference. > > Here are a few more test cases. > > (trap '(! :) && echo BUG1' EXIT) > (trap '(false) && echo BUG2' EXIT) > (trap 'readonly foo=bar; (foo=baz) && echo BUG3' EXIT) > (trap '(set -o bad@option) && echo BUG4' EXIT) Oops, I only tested the dotrap case, and not exitshell. The problem is that in evalsubshell we end up in exitshell again which restores the old exit status. So we need to come up with a way to differentiate the exitshell from the original shell vs. a subshell. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
eval: Only restore exit status on exit/return
Martijn Dekker wrote:
> Big bad bug: it appears that subshells always return status 0 in traps.
>
> Bug found in dash 0.5.9 and later.
>
> $ src/dash -c 'trap "(false) && echo BUG" INT; kill -s INT $$'
> BUG
> $ src/dash -c 'trap "(false) && echo BUG" EXIT'
> BUG
> $ src/dash -c 'trap "(false); echo \$?" EXIT'
> 0
>
> Workaround: if an explicit 'exit' is given, it works as expected.
>
> $ src/dash -c 'trap "(false; exit \$?); echo \$?" EXIT'
> 1
Thanks for the report. This patch should fix the problem:
---8<---
We unconditionally restore the saved status in exitreset, which
is incorrect as we only want to do it for exitcmd and returncmd.
This patch fixes the problem by introducing EXEND.
Reported-by: Martijn Dekker
Fixes: da30b4b78769 ("[BUILTIN] Exit without arguments in a trap...")
Signed-off-by: Herbert Xu
diff --git a/src/error.h b/src/error.h
index 9630b56..94e30a2 100644
--- a/src/error.h
+++ b/src/error.h
@@ -66,7 +66,8 @@ extern int exception;
/* exceptions */
#define EXINT 0/* SIGINT received */
#define EXERROR 1 /* a generic error */
-#define EXEXIT 4 /* exit the shell */
+#define EXEND 3/* exit the shell */
+#define EXEXIT 4 /* exit the shell via exitcmd */
/*
diff --git a/src/eval.c b/src/eval.c
index 546ee1b..3cea2a3 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -114,12 +114,13 @@ STATIC const struct builtincmd bltin = {
INCLUDE "eval.h"
EXITRESET {
- evalskip = 0;
- loopnest = 0;
if (savestatus >= 0) {
- exitstatus = savestatus;
+ if (exception == EXEXIT || evalskip == SKIPFUNCDEF)
+ exitstatus = savestatus;
savestatus = -1;
}
+ evalskip = 0;
+ loopnest = 0;
}
#endif
@@ -314,7 +315,7 @@ out:
if (flags & EV_EXIT) {
exexit:
- exraise(EXEXIT);
+ exraise(EXEND);
}
return exitstatus;
diff --git a/src/exec.c b/src/exec.c
index 9d0215a..87354d4 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -143,7 +143,7 @@ shellexec(char **argv, const char *path, int idx)
exitstatus = exerrno;
TRACE(("shellexec failed for %s, errno %d, suppressint %d\n",
argv[0], e, suppressint ));
- exerror(EXEXIT, "%s: %s", argv[0], errmsg(e, E_EXEC));
+ exerror(EXEND, "%s: %s", argv[0], errmsg(e, E_EXEC));
/* NOTREACHED */
}
diff --git a/src/main.c b/src/main.c
index 6d53e00..6b3a090 100644
--- a/src/main.c
+++ b/src/main.c
@@ -111,7 +111,7 @@ main(int argc, char **argv)
e = exception;
s = state;
- if (e == EXEXIT || s == 0 || iflag == 0 || shlvl)
+ if (e == EXEND || e == EXEXIT || s == 0 || iflag == 0 || shlvl)
exitshell();
reset();
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 0/5] Build system updates and gcc warnings fixes
Antonio Ospite wrote: > > can I mark 3/5 as Superseded in patchwork myself? > Or would that disrupt your workflow? > > The intent would be to have one patch less for you to worry about. I just marked it. Thanks! -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [BUG] ${#v} aborts string processing
Martijn Dekker wrote:
> Another bug introduced by 3cd5386, and not fixed in current git.
> String length expansion ${#v} aborts string processing.
>
> $ ./dash -c 'v=abc; echo ab${#v}cd'
> ab3
>
> Expected output: ab3cd
This should be fixed by
https://patchwork.kernel.org/patch/10688471/
I will push it out soon.
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[v2 PATCH] system: Disable glibc warning on sigsetmask
On Tue, Nov 20, 2018 at 11:48:20PM +0100, Antonio Ospite wrote:
>
> Just for the record, compiling with clang (CC=clang ./configure && make)
> still gives a warning:
OK, we could disable it completely unless it's gcc:
---8<---
As sigsetmask is set as deprecated in glibc this patch adds the
pragmas to disable the warning in gcc around our one and only use
of sigsetmask.
It also disables it completely for non-gcc compilers and older
gcc compilers as they may generate a warning too.
Reported-by: Antonio Ospite
Signed-off-by: Herbert Xu
diff --git a/src/system.h b/src/system.h
index a8d09b3..007952c 100644
--- a/src/system.h
+++ b/src/system.h
@@ -36,8 +36,17 @@
static inline void sigclearmask(void)
{
-#ifdef HAVE_SIGSETMASK
+#if defined(HAVE_SIGSETMASK) && \
+(!defined(__GLIBC__) || \
+ (defined(__GNUC__) && (__GNUC__ * 1000 + __GNUC_MINOR__) >= 4006))
+#ifdef __GLIBC__
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+#endif
sigsetmask(0);
+#ifdef __GLIBC__
+#pragma GCC diagnostic pop
+#endif
#else
sigset_t set;
sigemptyset();
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] parser: preserve characters on heap in backquote parsing
Ron Yorston wrote: > > Sure, but the same problem existing in other places isn't a reason not > to fix it here. If this were a real script then I might agree with you. But given that this came from a deliberately crafted script to breach the 8MB size limit, it would be pointless to fix it in just one spot because you could easily craft another script to exceed the size limit in any of the other places where we use alloca. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
eval: Use sh_warnx instead of warnx
Antonio Ospite wrote:
>
> BTW a new warning was introduced by commit 8e43729 (eval: Report I/O
> error on stdout, 2018-09-07):
>
> CC eval.o
> eval.c: In function ‘evalbltin’:
> eval.c:956:3: warning: implicit declaration of function ‘warnx’; did you mean
> ‘sh_warnx’? [-Wimplicit-function-declaration]
> warnx("%s: I/O error", commandname);
> ^
> sh_warnx
Thanks for the heads up.
---8<---
This patch fixes a typo in evalbltin where warnx was used instead
of sh_warnx.
Reported-by: Antonio Ospite
Fixes: 8e43729547b5 ("eval: Report I/O error on stdout")
diff --git a/src/eval.c b/src/eval.c
index 546ee1b..c27bc35 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -953,7 +953,7 @@ evalbltin(const struct builtincmd *cmd, int argc, char
**argv, int flags)
status = (*cmd->builtin)(argc, argv);
flushall();
if (outerr(out1))
- warnx("%s: I/O error", commandname);
+ sh_warnx("%s: I/O error", commandname);
status |= outerr(out1);
exitstatus = status;
cmddone:
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] parser: preserve characters on heap in backquote parsing
Ron Yorston wrote: > This bug report for BusyBox ash also applies to dash: > > https://bugs.busybox.net/show_bug.cgi?id=9246 > > With an 8MB stack the test case results in a segfault. > > Instead of using alloca() to preserve characters keep them on the > memalloc stack. With this change the test case returns: > > $ dash test_case > test_case: 3141: test_case: Syntax error: Unterminated quoted string > > If the heap is reduced to the same size as the stack, 8MB: > > $ ulimit -S -d 8192 > $ dash test_case > test_case: 0: test_case: Out of space > > Signed-off-by: Ron Yorston Sorry, but this is not the only place where dash relies on alloca. So you're bound to run into this problem again if you have a script that attempts to push dash to use more than 8MB in places like this. So I'm not going to accept this patch as alloca makes things a lot simpler in some cases. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
system: Disable gcc warning on sigsetmask
Jilles Tjoelker wrote:
>
> The git history starts (in 2005) after HAVE_SIGSETMASK was added, but I
> expect it was done this way to save a few bytes in the executable. With
> ProPolice, the effect may be a bit more since a local variable of type
> sigset_t often contains an array and may cause functions to be compiled
> with stack protection when they otherwise wouldn't (note that
> sigclearmask() is inline).
>
> Both FreeBSD and NetBSD simply changed the sigsetmask() call to a
> sigprocmask() call very early on (1995-1996).
>
> Personally, I think clean code that compiles without warnings is more
> important than making the executable as small as possible, but the
> maintainer may not agree.
I agree with getting rid of the warning, and this seems to work
for me. Please let me know if you still a get warning on some
other platform. Thanks!
---8<---
As sigsetmask is set as deprecated in glibc this patch adds the
pragmas to disable the warning in gcc around our one and only use
of sigsetmask.
Reported-by: Antonio Ospite
Signed-off-by: Herbert Xu
diff --git a/src/system.h b/src/system.h
index a8d09b3..f3aa930 100644
--- a/src/system.h
+++ b/src/system.h
@@ -37,7 +37,14 @@
static inline void sigclearmask(void)
{
#ifdef HAVE_SIGSETMASK
+#if defined(__GNUC__) && (__GNUC__ * 1000 + __GNUC_MINOR__) >= 4006
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+#endif
sigsetmask(0);
+#if defined(__GNUC__) && (__GNUC__ * 1000 + __GNUC_MINOR__) >= 4006
+#pragma GCC diagnostic pop
+#endif
#else
sigset_t set;
sigemptyset();
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
parser: Do not push token back before parseheredoc
Sorry for the repost, I'm trying to get this into patchwork.
---8<---
When we read the first token in list() we use peektoken instead
of readtoken as the following code needs to use the same token
again. However, this is wrong when we're in a here-document as
it will clobber the saved token without resetting the tokpushback
flag.
This patch fixes it by doing the tokpushback after parseheredoc
and setting lasttoken again if parseheredoc was called.
Reported-by: Ron Yorston
Fixes: 7c245aa8ed33 ("[PARSER] Simplify EOF/newline handling in...")
Fixes: ee5cbe9fd6bc ("[SHELL] Optimize dash -c "command" to avoid a fork")
Signed-off-by: Herbert Xu
diff --git a/src/parser.c b/src/parser.c
index c4e6378..1f9e8ec 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -166,7 +166,7 @@ list(int nlflag)
n1 = NULL;
for (;;) {
- switch (peektoken()) {
+ switch (readtoken()) {
case TNL:
if (!(nlflag & 1))
break;
@@ -177,9 +177,12 @@ list(int nlflag)
if (!n1 && (nlflag & 1))
n1 = NEOF;
parseheredoc();
+ tokpushback++;
+ lasttoken = TEOF;
return n1;
}
+ tokpushback++;
checkkwd = CHKNL | CHKKWD | CHKALIAS;
if (nlflag == 2 && tokendlist[peektoken()])
return n1;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH] expand: Eat closing brace for length parameter expansion
Apologies for the reposts, but I'm having problems making this
get into patchwork.
---8<---
When we are doing VSLENGTH expansion, the closing brace is currently
not removed in evalvar. This causes the caller argstr to terminate
prematurely as it would interpret the closing brace as one that
belongs to a parameter expansion at the outer level.
This patch fixes it.
Reported-by: Martijn Dekker
Fixes: 3cd538634f71 ("expand: Do not reprocess data when...")
Signed-off-by: Herbert Xu
diff --git a/src/expand.c b/src/expand.c
index 856b7a9..af9cac9 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -745,6 +745,7 @@ again:
varunset(p, var, 0, 0);
if (subtype == VSLENGTH) {
+ p++;
if (flag & EXP_DISCARD)
return p;
cvtnum(varlen > 0 ? varlen : 0, flag);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Add the seq builtin and improve some things in printf.c
Devin Hussey wrote: > From: Devin Hussey > > I added the seq builtin. > > usage: seq [-w] [-f format] [-s string] [-t string] [first [incr]] last > > Some notes: >- 100% from scratch, aside from the small num_width algorithm I took from > StackOverflow and credited accordingly. >- I optimize the math by using integer math if we don't have any decimals, > intmax_t is large enough to support the number range, and if the user > didn't specify a custom format. GNU Coreutils also does this. > - If we use integers, we precalculate the width and snprintf a format >specifer from that for the -w option. >- I reuse some of the printf code to act as the validator for the format > string. I want to separate this more for the best possible const > correctness, though (I use a cast), and so the conditionals are a little > more clear. >- It functionally matches the one supplied with macOS and presumably BSD, > but it's faster: It is reliably slightly faster on my system (a wimpy > 2009 MacBook) to call this > ./dash -c 'seq 1 1' > than it is to call > seq 1 1 > directly, even multiple times, from a bash shell. > > Other changes: >- I fixed a few silly gotos in the file, replacing them with negated > conditional blocks or early returns. >- I isolated the printf parsing into its own function so we can reuse it. >- I reordered the static functions to go above the main functions. This > makes it easier to follow. >- I changed conv_escape to return the number of characters read instead > of the string itself so it can accept a constant string without any > ugly casting. > > Signed-off-by: Devin Hussey Sorry, I'm not adding this builtin to dash as this conflicts with the goal of dash trying to be minimal. Perhaps bash or mksh would be a better option? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Command substitution in here-documents
On Fri, Aug 17, 2018 at 09:08:35PM +0800, Herbert Xu wrote:
> Ron Yorston wrote:
> > I've been continuing to worry about this.
> >
> > In the old code (prior to commit 7c245aa) the TNL token that was
> > detected in list() had actually been read. The current code uses
> > peektoken().
> >
> > When parseheredoc() is called in interactive mode the last call to
> > readtoken() is in the code to set the prompt. Even though the text
> > of that token is long gone it's used anyway. Exactly what then happens
> > on a given platform depends on how memory allocation is handled.
> >
> > I can make the error go away by explicitly reading the TNL token when
> > peektoken() detects a newline and we're about to call parseheredoc().
> >
> > Of course, I may be wrong.
>
> The real problem is the fact that we call expandstr which reenters
> the parser and the parser is not reentrant.
>
> We need to save the parser state and restore it around expandstr.
> I'm working on a fix.
This patch should fix the problem:
---8<---
parser: Do not push token back before parseheredoc
When we read the first token in list() we use peektoken instead
of readtoken as the following code needs to use the same token
again. However, this is wrong when we're in a here-document as
it will clobber the saved token without resetting the tokpushback
flag.
This patch fixes it by doing the tokpushback after parseheredoc
and setting lasttoken again if parseheredoc was called.
Reported-by: Ron Yorston
Fixes: 7c245aa8ed33 ("[PARSER] Simplify EOF/newline handling in...")
Fixes: ee5cbe9fd6bc ("[SHELL] Optimize dash -c "command" to avoid a fork")
Signed-off-by: Herbert Xu
diff --git a/src/parser.c b/src/parser.c
index c4e6378..1f9e8ec 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -166,7 +166,7 @@ list(int nlflag)
n1 = NULL;
for (;;) {
- switch (peektoken()) {
+ switch (readtoken()) {
case TNL:
if (!(nlflag & 1))
break;
@@ -177,9 +177,12 @@ list(int nlflag)
if (!n1 && (nlflag & 1))
n1 = NEOF;
parseheredoc();
+ tokpushback++;
+ lasttoken = TEOF;
return n1;
}
+ tokpushback++;
checkkwd = CHKNL | CHKKWD | CHKALIAS;
if (nlflag == 2 && tokendlist[peektoken()])
return n1;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
eval: Use the correct expansion mode for fd redirection
It has been reported that
echo test >&$EMPTY_VARIABLE
causes dash to segfault. This is a symptom of the bigger problem
that dash tries to perform pathname expansion as well as field
splitting on the word after >& and <&. This is wrong and this
patch fixes it to use the same expansions as done on a normal
redirection.
Reported-by: Andrej Shadura
Signed-off-by: Herbert Xu
diff --git a/src/eval.c b/src/eval.c
index 6185db4..328fde3 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -529,7 +529,7 @@ expredir(union node *n)
case NFROMFD:
case NTOFD:
if (redir->ndup.vname) {
- expandarg(redir->ndup.vname, , EXP_FULL |
EXP_TILDE);
+ expandarg(redir->ndup.vname, , EXP_TILDE |
EXP_REDIR);
fixredir(redir, fn.list->text, 1);
}
break;
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Revert 78a00a7 and 3cd5386, they are not ready yet.
Devin Hussey wrote:
> From 65f9c258658b50aeb67947f5b54e926538560488 Mon Sep 17 00:00:00 2001
> From: Devin Hussey
> Date: Thu, 15 Nov 2018 08:29:33 -0500
> Subject: [PATCH] Revert 78a00a7 and 3cd5386, they are not ready yet.
>
> They are not ready, as simply running dash on its own configure
> script will reveal (note: config.cache does not exist):
>
>checking for a BSD-compatible install... (cached)
>checking whether build environment is sane... yes
>configure: 1: eval: --is-lightweight: not found
>configure: WARNING: 'missing' script is too old or missing
>checking for a thread-safe mkdir -p... (cached) -p
>checking for gawk... (cached) no
>checking for mawk... (cached) no
>checking for nawk... (cached) no
>checking for awk... (cached) no
>checking whether make sets $(MAKE)... (cached) configure: 1: test:
> =: unexpected operator
>no
>checking whether make supports nested variables... (cached)
>configure: 2878: test: =: unexpected operator
>checking for gcc... (cached) no
>checking for cc... (cached) no
>checking for cl.exe... (cached) no
>configure: error: in `/Users/Devin/dash2':
>configure: error: no acceptable C compiler found in $PATH
>See `config.log' for more details
>
> 3cd5386 completely broke expansion.
>
> I got the configure script to run by changing
>
>return p - 1;
>
> to
>
>return p;
>
> in argstr, but variable expansion is still broken.
>
> The most broken thing is ${FOO+BAR}:
>
>$ bash -c 'unset FOO; echo ${FOO+BAR}'
>
>$ ./dash -c 'unset FOO; echo ${FOO+BAR}'
>BAR
>$
Please look at the pending patches first. These issues have
been fixed months ago:
https://patchwork.kernel.org/patch/10596841/
Unfortunately I haven't the time to push out the fix to the
git tree. I'll do to that now.
Thanks,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2] expand: Fix multiple issues with EXP_DISCARD in evalvar
On Wed, Nov 14, 2018 at 11:15:36PM +0100, Martijn Dekker wrote:
> I encountered another bug, introduced by 3cd5386 and not fixed by v2 of this
> patch: the presence of a length-counting expansion like ${#foo} in a string
> causes the rest of the string to be discarded.
>
> $ src/dash -c 'foo=bar; echo "baz ${#foo} quux"'
> baz 3
> $ src/dash -c 'foo=bar; echo baz\ ${#foo}\ quux'
> baz 3
>
> (expected outout: baz 3 quux)
Thanks. This is an unrelated issue where we fail to eat the
closing brace for $# expansion. This causes the outer argstr
to termiante prematurely.
---8<---
expand: Eat closing brace for length parameter expansion
When we are doing VSLENGTH expansion, the closing brace is currently
not removed in evalvar. This causes the caller argstr to terminate
prematurely as it would interpret the closing brace as one that
belongs to a parameter expansion at the outer level.
This patch fixes it.
Reported-by: Martijn Dekker
Fixes: 3cd538634f71 ("expand: Do not reprocess data when...")
Signed-off-by: Herbert Xu
diff --git a/src/expand.c b/src/expand.c
index 856b7a9..af9cac9 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -745,6 +745,7 @@ again:
varunset(p, var, 0, 0);
if (subtype == VSLENGTH) {
+ p++;
if (flag & EXP_DISCARD)
return p;
cvtnum(varlen > 0 ? varlen : 0, flag);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [BUG] failure to push/restore closed file descriptor
Harald van Dijk wrote:
> On 23/04/2018 19:56, Martijn Dekker wrote:
>> $ dash -c '{ exec 8> Output: "oops, still open"
>> Expected output: Bad file descriptor
>>
>> Apparently, dash either fails to push the file descriptor onto the stack
>> at '} 8<&-', or fails to restore it.
>>
>> Same bug with loops ending in "done 8<&-".
>>
>> Confirmed in all dash versions down to 0.5.5.1.
>
> What surprises me most is that dash has code written specifically to
> keep the fd closed. dash would be smaller and simpler if it behaved the
> way you expected and the way most other shells behave: just remove all
> traces of REALLY_CLOSED.
So did anything happen on the bash front? I'm happy to change if
bash moves in the same direction.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH v2] expand: Fix multiple issues with EXP_DISCARD in evalvar
On Fri, Sep 07, 2018 at 06:55:29AM +0200, Martijn Dekker wrote:
>
> But now I'm encountering another, similar bug, that was a bit harder to
> track down:
>
> $ src/dash -c 'foo=bar; echo ${foo=BUG}; echo $foo'
> barBUG
> bar
> $ src/dash -c 'foo=bar; echo ${foo:=BUG}; echo $foo'
> barBUG
> bar
>
> Expected output: 'bar' twice in both cases. The ${foo=BUG} and ${foo:=BUG}
> expansions fail to discard the word 'BUG' if foo is set.
Thanks for the update. In this case we didn't call the parsing
function subevalvar at all when we should have called it with
EXP_DISCARD.
As patchwork was having issues I've rolled all three patches into
one.
Cheers,
---8<---
The commit 3cd538634f71538370f5af239f342aec48b7470b broke parameter
expansion in multiple ways because the EXP_DISCARD flag wasn't set
or tested for various cases:
$ src/dash -c 'var=; echo ${var:+nonempty}'
nonempty
$ src/dash -u -c 'unset foo bar; echo ${foo+${bar}}'
dash: 1: bar: parameter not set
$ src/dash -c 'foo=bar; echo ${foo=BUG}; echo $foo'
barBUG
bar
$
This patch fixes them by introducing a new discard variable that
tracks whether the extra word should be discarded or not when it
is parsed.
Reported-by: Martijn Dekker
Fixes: 3cd538634f71 ("expand: Do not reprocess data when...")
Signed-off-by: Herbert Xu
diff --git a/src/expand.c b/src/expand.c
index 14daa63..856b7a9 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -698,6 +698,7 @@ evalvar(char *p, int flag)
int patloc;
int startloc;
ssize_t varlen;
+ int discard;
int quoted;
varflags = *p++;
@@ -713,41 +714,41 @@ again:
if (varflags & VSNUL)
varlen--;
+ discard = varlen < 0 ? EXP_DISCARD : 0;
+
switch (subtype) {
case VSPLUS:
- varlen = -1 - varlen;
+ discard ^= EXP_DISCARD;
/* fall through */
case 0:
case VSMINUS:
- p = argstr(p, flag | EXP_TILDE | EXP_WORD);
- if (varlen < 0)
- return p;
+ p = argstr(p, flag | EXP_TILDE | EXP_WORD |
+ (discard ^ EXP_DISCARD));
goto record;
case VSASSIGN:
case VSQUESTION:
- if (varlen >= 0)
- goto record;
-
p = subevalvar(p, var, 0, startloc, varflags,
- flag & ~QUOTES_ESC);
+ (flag & ~QUOTES_ESC) |
+ (discard ^ EXP_DISCARD));
- if (flag & EXP_DISCARD)
- return p;
+ if ((flag | ~discard) & EXP_DISCARD)
+ goto record;
varflags &= ~VSNUL;
+ subtype = VSNORMAL;
goto again;
}
- if (varlen < 0 && uflag)
+ if ((discard & ~flag) && uflag)
varunset(p, var, 0, 0);
if (subtype == VSLENGTH) {
if (flag & EXP_DISCARD)
return p;
cvtnum(varlen > 0 ? varlen : 0, flag);
- goto record;
+ goto really_record;
}
if (subtype == VSNORMAL)
@@ -765,7 +766,7 @@ again:
}
#endif
- flag |= varlen < 0 ? EXP_DISCARD : 0;
+ flag |= discard;
if (!(flag & EXP_DISCARD)) {
/*
* Terminate the string and start recording the pattern
@@ -778,9 +779,10 @@ again:
p = subevalvar(p, NULL, patloc, startloc, varflags, flag);
record:
- if (flag & EXP_DISCARD)
+ if ((flag | discard) & EXP_DISCARD)
return p;
+really_record:
if (quoted) {
quoted = *var == '@' && shellparam.nparam;
if (!quoted)
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: ${var+set}, ${var:+nonempty} broken in current git
On Wed, Sep 05, 2018 at 06:21:36PM +0200, Martijn Dekker wrote:
>
> With this patch applied, the following breakage still occurs:
> $ src/dash -u -c 'unset foo bar; echo ${foo+${bar}}'
> src/dash: 1: bar: parameter not set
> (expected: empty line, no error)
> ...which seems to suggest that ${bar} is evaluated even though foo is unset.
This patch should fix it.
---8<---
Subject: expand: Only call varunset when EXP_DISCARD is not set
When EXP_DISCARD is set, we must not generate a real error such
as varunset because we're only parsing the text and not evaluating
it.
For example:
$ dash -u -c 'unset foo bar; echo ${foo+${bar}}'
dash: 1: bar: parameter not set
This should not fail.
This patch fixes it by checking the EXP_DISCARD bit in the caller
flag before calling varunset.
Reported-by: Martijn Dekker
Fixes: 3cd538634f71 ("expand: Do not reprocess data when...")
Signed-off-by: Herbert Xu
diff --git a/src/expand.c b/src/expand.c
index e890052..d3e66c8 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -744,7 +744,7 @@ again:
goto again;
}
- if (discard && uflag)
+ if ((discard & ~flag) && uflag)
varunset(p, var, 0, 0);
if (subtype == VSLENGTH) {
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: ${var+set}, ${var:+nonempty} broken in current git
On Sun, Sep 02, 2018 at 10:15:50PM +, Martijn Dekker wrote:
> Commit 3cd538634f71538370f5af239f342aec48b7470b broke these:
>
> $ src/dash -c 'unset var; echo ${var+set}'
> set
> $ src/dash -c 'var=; echo ${var:+nonempty}'
> nonempty
Thanks for the report! I forgot to discard the result of the unused
expansion.
---8<---
Subject: expand: Discard result of unused expansion in evalvar
The commit 3cd538634f71538370f5af239f342aec48b7470b broke parameter
expansion where the nested expanded word needs to be discarded.
This is because the EXP_DISCARD flag wasn't set where it should
have been:
$ src/dash -c 'var=; echo ${var:+nonempty}'
nonempty
This patch fixes it by setting it where needed.
Reported-by: Martijn Dekker
Fixes: 3cd538634f71 ("expand: Do not reprocess data when...")
Signed-off-by: Herbert Xu
diff --git a/src/expand.c b/src/expand.c
index 14daa63..e890052 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -698,6 +698,7 @@ evalvar(char *p, int flag)
int patloc;
int startloc;
ssize_t varlen;
+ int discard;
int quoted;
varflags = *p++;
@@ -713,21 +714,24 @@ again:
if (varflags & VSNUL)
varlen--;
+ discard = varlen < 0 ? EXP_DISCARD : 0;
+
switch (subtype) {
case VSPLUS:
- varlen = -1 - varlen;
+ discard ^= EXP_DISCARD;
/* fall through */
case 0:
case VSMINUS:
- p = argstr(p, flag | EXP_TILDE | EXP_WORD);
- if (varlen < 0)
+ p = argstr(p, flag | EXP_TILDE | EXP_WORD |
+ (discard ^ EXP_DISCARD));
+ if (discard)
return p;
goto record;
case VSASSIGN:
case VSQUESTION:
- if (varlen >= 0)
+ if (!discard)
goto record;
p = subevalvar(p, var, 0, startloc, varflags,
@@ -740,7 +744,7 @@ again:
goto again;
}
- if (varlen < 0 && uflag)
+ if (discard && uflag)
varunset(p, var, 0, 0);
if (subtype == VSLENGTH) {
@@ -765,7 +769,7 @@ again:
}
#endif
- flag |= varlen < 0 ? EXP_DISCARD : 0;
+ flag |= discard;
if (!(flag & EXP_DISCARD)) {
/*
* Terminate the string and start recording the pattern
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Possible error in Escape Character (Backslash) handling
Stan Senotrusov wrote: > Hello! > > I discovered the changes in how dash handles backslash escape character. I > wonder is that change intentional? > > # (Previous version) Dash version 0.5.8.2.1ubuntu2 on Ubuntu > $ dash -c 'exec echo \a\b\c' > abc > > # (Current version) Dash version 0.5.10.2 on MacOS 10.13.4 installed by > homebrew > $ dash -c 'exec echo \a\b' > \a\b > > $ dash -c 'exec echo \a\b\c' > \a\b This appears to be the same issue as https://www.mail-archive.com/dash@vger.kernel.org/msg01700.html Please see my reply to that thread and follow up over there. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: Possible regression in Dash v0.5.10 on OSX / home-brew
Robbie Gates wrote: > Hi All, > > I hope this is the right place to report this: > > It looks like something is up with processing of certain characters > inside quoted strings when --enable-fnmatch --enable-glob are enabled, > as they are in the homebrew installation of dash on OSX: > > ( git checkout v0.5.10 && git clean -dfx && ./autogen.sh && > ./configure --enable-fnmatch --enable-glob && make && ./src/dash -c > "printf '%s\n' 'a/?*[/b'" ) 2>/dev/null | tail -1 > > produces > > a\/\?\*\[\/b\n > > with no terminating newline. I believe that dash is adding an extra > backslash in front of some special characters inside quoted strings. > The \n and missing newline above is because dash is doubling the > backslash, so printf is then seeing \\n, printing \ and n (and no > newline). This theory is backed up by switching out printf for a > simple c program which just dumps argv to stdout. > > The behaviour is present on master for me, at git commit > 03cbaba30d7f6e4f89b6e881b6b909cb45924025 Thanks for the report. I presume this is the result of commit 6900ff60ef7347a8c1445853a8f4689808e0976e Author: Herbert Xu Date: Mon Mar 26 17:50:24 2018 +0800 expand: Fix glibc glob(3) support Are you using glibc or is this a different glob(3) implementation? If it is a different implementation then we will simply need to add some build-time checks to detect the glob(3) implementation. If this is glibc may I ask what version you're using? Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
