Re: [PATCH] gpg-interface.c: Fix potentially freeing NULL values

["Michał Górny" <mgorny@gentoo.org>]

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

On Fri, 2018-08-17 at 05:28 -0400, Eric Sunshine wrote:
> On Fri, Aug 17, 2018 at 5:17 AM Michał Górny <mgorny@gentoo.org> wrote:
> > Fix signature_check_clear() to free only values that are non-NULL.  This
> > especially applies to 'key' and 'signer' members that can be NULL during
> > normal operations, depending on exact GnuPG output.  While at it, also
> > allow other members to be NULL to make the function easier to use,
> > even if there is no real need to account for that right now.
> 
> free(NULL) is valid behavior[1] and much of the Git codebase relies upon it.
> 
> Did you run into a case where it misbehaved?

Nope.  I was actually wondering if it's expected, so I did a quick grep
to check whether git is checking pointers for non-NULL before free()ing,
and found at least one:

blame.c-static void drop_origin_blob(struct blame_origin *o)
blame.c-{
blame.c-        if (o->file.ptr) {
blame.c:                FREE_AND_NULL(o->file.ptr);
blame.c-        }
blame.c-}

So I wrongly presumed it might be desirable.  If it's not, that's fine
by me.

> 
> [1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html
> 
> > Signed-off-by: Michał Górny <mgorny@gentoo.org>
> > ---
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 35c25106a..9aedaf464 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -15,9 +15,14 @@ static const char *gpg_program = "gpg";
> >  void signature_check_clear(struct signature_check *sigc)
> >  {
> > -       FREE_AND_NULL(sigc->payload);
> > -       FREE_AND_NULL(sigc->gpg_output);
> > -       FREE_AND_NULL(sigc->gpg_status);
> > -       FREE_AND_NULL(sigc->signer);
> > -       FREE_AND_NULL(sigc->key);
> > +       if (sigc->payload)
> > +               FREE_AND_NULL(sigc->payload);
> > +       if (sigc->gpg_output)
> > +               FREE_AND_NULL(sigc->gpg_output);
> > +       if (sigc->gpg_status)
> > +               FREE_AND_NULL(sigc->gpg_status);
> > +       if (sigc->signer)
> > +               FREE_AND_NULL(sigc->signer);
> > +       if (sigc->key)
> > +               FREE_AND_NULL(sigc->key);
> >  }

-- 
Best regards,
Michał Górny

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 963 bytes --]