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

From: Michał Górny <mgorny@gentoo.org>
Date: Aug 17, 2018, 09:40 UTC
Message-ID: <1534498806.1262.8.camel@gentoo.org>
In-Reply-To: <CAPig+cQVWY3+2aarYw=uXti0=1SW8boMPoYj1zatw1KKKVOqnQ@mail.gmail.com>

On Fri, 2018-08-17 at 05:28 -0400, Eric Sunshine wrote:

Show 10 quoted lines

> 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.

Show 28 quoted lines

> 
> [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