Re: [PATCH] progress: don't dereference before checking for NULL

[Taylor Blau <me@ttaylorr.com>]

On Tue, Aug 11, 2020 at 06:28:30AM +0200, Martin Ågren wrote:
> On Mon, 10 Aug 2020 at 23:27, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >
> > On Mon, Aug 10, 2020 at 3:48 PM Martin Ågren <martin.agren@gmail.com> wrote:
> > >  void stop_progress(struct progress **p_progress)
> > >  {
> > > +       if (!p_progress)
> > > +               BUG("don't provide NULL to stop_progress");
> > > +
> > >         finish_if_sparse(*p_progress);
> >
> > I'm wondering what this really buys us over simply crashing due to the
> > NULL dereference (aside from the slightly more informative diagnostic
> > message). Either way, it's going to crash, as it should because
> > passing NULL is indeed a programmer error for these two functions. I'm
> > pretty sure that it is more common in this project simply to allow a
> > programmer error like this simply to crash on its own rather than
> > adding code to make it crash explicitly.
>
> I'm not a big fan of undefined behavior. In general, I don't buy the
> "but in practice it will do what we want" argumentation.

I think that this is good reasoning; having the guard around
'p_progress' being non-NULL makes the implementation have no undefined
behavior, which is worth a lot.

> Before 98a1364740 ("trace2: log progress time and throughput",
> 2020-05-12), we didn't check for NULL in this function. Then that commit
> tried to do so. It would feel wrong for me to say "that commit didn't
> get it quite right -- rip out the check". Then, to be honest, I'd much
> rather just leave it in place. At least that way, someone else might
> spot it a year from now.
>
> I could add an early return (instead of an early BUG). That would
> gracefully handle NULL. Grepping around suggests there are other `if (!p)
> BUG();`. Even Documentation/CodingGuidelines BUGs on a NULL-pointer,
> although in the context of checking for NULL (as opposed to "how to
> BUG").
>
> > > -       if (p_progress && *p_progress) {
> > > +       if (*p_progress) {
> >
> > In other words, I think the entire patch can be reduced to just this
> > change here (and a simplified commit message).
>
> I started with this, but then I felt terrible for just sweeping the
> whole thing under the rug.

Yep, I'm a fan of the direction you ended up taking. Thanks.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

> Martin

Thanks,
Taylor