Re: [PATCH 6/5] run-command: avoid potential dangers in forked child

[Brandon Williams <bmwill@google.com>]

On 04/11, Eric Wong wrote:
> Brandon Williams <bmwill@google.com> wrote:
> > On 04/11, Eric Wong wrote:
> > > Hi Brandon, this series tickles an old itch of mine, so I
> > > started working off of it.  I'm only somewhat concerned
> > > with the path resolution in execvp(e) pontentially calling
> > > malloc on some libcs; but I suppose that's a separate patch
> > > for another time.
> > > 
> > > Only lightly-tested at the moment, but things seem to work...
> > 
> > Thanks Eric! I'll spend some time looking at this patch later today.  As
> > for the path resolution in execvp(e), I guess we could completely avoid
> > that if we did the path resolution ourselves, prior to forking, and then
> > just use execv(e) since it shouldn't have any calls to malloc in them
> > correct?
> 
> Yeah.  I spent some time looking at it last night, but emulating
> the existing ENOENT / EACCESS / ENOTDIR mapping made my head
> hurt.
> 
> And I'm not sure if I introduced any off-by-one errors in
> exists_in_PATH when removing strbuf usage; string manipulation
> in plain C scares me :x   Since memcpy/strcpy/getenv in there
> are not specified as async-signal safe, they could
> theoretically take locks and cause breakage inside a child.

Well if we move away from the (p) variant of exec, and do that
resolution ourselves, then we can use the existing
exists_in_PATH/locate_in_PATH logic (using strbufs!) and avoid needing
to do that string manipulation in plain C.  Of course we would then need
to adjust some of the errno handling (e.g. if it doesn't exist in the
path, we would then know that prior to forking so really no need to
handle that in the child anymore).

-- 
Brandon Williams