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

From: Martin Ågren <martin.agren@gmail.com>
Date: Aug 11, 2020, 04:28 UTC
Message-ID: <CAN0heSotdXH50ssd01b7fFuqEuOs+X0f1Zpj+nhxuO=TRNStUg@mail.gmail.com>
In-Reply-To: <CAPig+cTsxAW=s1iXcK_-Kn+xiSNCm_u_o_Q2xm-0+a_v4qc5_w@mail.gmail.com>

On Mon, 10 Aug 2020 at 23:27, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 16 quoted lines

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

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

Show 5 quoted lines

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

Martin