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

[Andrew Gregory]

On 05/14/20 at 11:30pm, Anatol Pomozov wrote:
> Hi
> 
> On Wed, May 13, 2020 at 7:54 PM Andrew Gregory
> <andrew.gregory.8 at gmail.com> wrote:
> >
> > 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

You are looking at the wrong line, move down 3.

Also, that function needs to be fixed to meet our style guidelines.

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

I know how assert works.  Like I said, if that assert gets compiled
out and we hit a malloc failure at the right time the results are
undefined and anything can happen.

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

Why the rush?  If we take a second and settle on a decent API first,
things that link to alpm can update and we can get that much more
testing and have a smoother update when we're ready for final release.
Otherwise, I'm not even going to be able to install a beta release
myself because I have no intention of making multiple updates to the
rest of my software while we figure out what the API should be.

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

You're only passing data for whatever particular download item saw an
event, so the callback has to maintain its own list of the full set of
download objects.  We should give it the complete list each time.