[pacman-dev] [PATCH] Avoid depending on side effects in assert(...) expressions

[Anatol Pomozov]

Hi

On Wed, May 13, 2020 at 7:54 PM Andrew Gregory
<andrew.gregory.8 at gmail.com> wrote:
>
> On 05/13/20 at 12:08pm, Anatol Pomozov wrote:
> > > ---
> > > It's perhaps worth mentioning that nowhere else in the ALPM codebase
> > > do we use assert().
> >
> > I quite like the idea of defensive programming. This is something that
> > I learnt the hard way when I was working with chips firmware.
> > So I often add additional checks across the codebase and it saves me
> > time during active phase of development/refactoring.
> ...
> > A bit of context for this assert(). Both "progress" and "complete"
> > events should always have a corresponding "init" event where
> > progressbar structure is initialized.
> >
> > If callback.c received a "progress" event for a non-existent
> > progressbar then it is a programming error.
>
> The problem is not defensive programming, it's that assert gets
> compiled out under -DNDEBUG, so all your defenses disappear.

True, assert() does *not* evaluate parameters if -DNDEBUG is used.
I was confused by asserts at other systems like Linux's BUG_ON() that
*does* evaluate
parameters even if BUG() support is disabled.

Dave's patch makes what I intended to do - evaluate the statement and
then call assert()
to check the results. It is OK to disable asserts and it should not
affect the application
functionality.

> You say
> that the only way there would not be a corresponding progress bar is
> if the progress event is called without an init event; that's not
> true.  If there's a memory allocation failure in the init event there
> won't be a progress object.

If there is a malloc failure in the init callback then application
will crash with SIGSEGV while
trying to access the NULL pointer

https://github.com/anatol/pacman/blob/b96e0df4dceaa2677baa1a3563211950708d3e63/src/pacman/callback.c#L850

> At that point bar and index are
> indeterminate and the best we can hope for is a quick segfault.

an application crash is pretty much what assert() does here. It
basically checks the pointer before we start using it.

In some sense assert() is a form of a self-documenting check
"this situation should never happen, but if it does then it is a bug
in the app logic. so let's crash the app to bring
maximum attention to it".

These asserts() are extra-checks but they are not required. It can be
disabled or even removed without
loosing functionality.

> The
> callback api should be fixed so that callbacks can indicate an error
> that should cancel the download.

It sounds like a good idea to return an error code from the callback.
In this case we can
handle malloc() failure in init() without crashing the application.

But I do not think it should block the multi-download testing. It
would be great to get people testing the new functionality
to discover issues as soon as possible.

> I'd also say we should be passing the full list of download items to
> the callback so that it's possible to write at least a very simple
> multi-download callback without having to maintain a bunch of
> complicated state information.

I am not sure I fully understand this idea. Could you please expand more on it?