Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

On 2020-01-07 at 02:04:25, Jonathan Nieder wrote:
> brian m. carlson wrote:
> 
> > In this function, we free the pointer we get from locate_in_PATH and
> > then check whether it's NULL.  However, this is undefined behavior if
> > the pointer is non-NULL, since the C standard no longer permits us to
> > use a valid pointer after freeing it.
> [...]
> > Noticed-by: Miriam R. <mirucam@gmail.com>
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> >  run-command.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> This API that forces the caller to deal with the allocated result when
> they never asked for it seems a bit inconvenient.  Should we clean it up
> a little?  Something like this (on top):
> 
> -- >8 --
> Subject: run-command: let caller pass in buffer to locate_in_PATH
> 
> Instead of returning a buffer that the caller is responsible for
> freeing, use a strbuf output parameter to record the path to the
> searched-for program.
> 
> This makes ownership a little easier to reason about, since the owning
> code declares the buffer.  It's a good habit to follow because it
> allows buffer reuse when calling such a function in a loop.
> 
> It also allows the caller exists_in_PATH that does not care about the
> path to the command to be slightly simplified, by allowing a NULL
> output parameter that means that locate_in_PATH should take care of
> allocating and freeing its temporary buffer.

Sure, I think this is a nice improvement.  The user can reuse the buffer
in a loop if they want by resetting it, which as you point out would be
convenient (and potentially more efficient).  And we're already using
one internally, so there's no reason not to just pass one in.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]