| Message ID | 20190116133231.mx53nyjhgrfiflch@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v3] redir: Handle nested exec within REALLY_CLOSED redirection | expand |
Op 16-01-19 om 14:32 schreef Herbert Xu: > 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. Does this actually save cycles? I'm probably missing something, but that code looks to me like it probably uses more cycles than close(2) going 'descriptor not open, return'. - Martijn
Op 16-01-19 om 22:39 schreef Martijn Dekker: > Op 16-01-19 om 14:32 schreef Herbert Xu: >> 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. > > Does this actually save cycles? I'm probably missing something, but that > code looks to me like it probably uses more cycles than close(2) going > 'descriptor not open, return'. Never mind, stupid question that I should have googled before asking it. The answer is that a switch to kernel mode and back is expensive. - M.
On 16/01/2019 21:53, Martijn Dekker wrote: > Op 16-01-19 om 22:39 schreef Martijn Dekker: >> Op 16-01-19 om 14:32 schreef Herbert Xu: >>> 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. >> >> Does this actually save cycles? I'm probably missing something, but that >> code looks to me like it probably uses more cycles than close(2) going >> 'descriptor not open, return'. > > Never mind, stupid question that I should have googled before asking it. > The answer is that a switch to kernel mode and back is expensive. 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. Cheers, Harald van Dijk
On Wed, Jan 16, 2019 at 10:29:50PM +0000, 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,
On 17/01/2019 07:01, Herbert Xu wrote: > On Wed, Jan 16, 2019 at 10:29:50PM +0000, 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). Oh, okay. In that case, the fact that there are code paths in which dash ends up calling close(-1) may be another issue to look at, since that is known before the call to be a wasted syscall as well. Cheers, Harald van Dijk
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</dev/null
} >&-
{
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.
- Martijn
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 = &sv->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;