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

Dave Reisner

13 May 2020 13 May '20

6:43 p.m.

When building with -DNDEBUG, assert statements are compiled out to no-ops. Thus, we can't depend on assignments or other computations occurring inside the assert(). --- It's perhaps worth mentioning that nowhere else in the ALPM codebase do we use assert(). src/pacman/callback.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 25909e02..4240a779 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -862,12 +862,14 @@ static void dload_progress_event(const char *filename, alpm_download_event_progr int64_t curr_time = get_time_ms(); double last_chunk_rate; int64_t timediff; + bool ok; if(!dload_progressbar_enabled()) { return; } - assert(find_bar_for_filename(filename, &index, &bar)); + ok = find_bar_for_filename(filename, &index, &bar); + assert(ok); /* compute current average values */ timediff = curr_time - bar->sync_time; @@ -902,12 +904,14 @@ static void dload_complete_event(const char *filename, alpm_download_event_compl int index; struct pacman_progress_bar *bar; int64_t timediff; + bool ok; if(!dload_progressbar_enabled()) { return; } - assert(find_bar_for_filename(filename, &index, &bar)); + ok = find_bar_for_filename(filename, &index, &bar); + assert(ok); bar->completed = true; /* This may not have been initialized if the download finished before -- 2.26.2

Show replies by date

Anatol Pomozov

13 May 13 May

7:08 p.m.

Hi The change looks good to me. On Wed, May 13, 2020 at 11:44 AM Dave Reisner wrote:

...

When building with -DNDEBUG, assert statements are compiled out to no-ops. Thus, we can't depend on assignments or other computations occurring inside the assert(). --- 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.

...

src/pacman/callback.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 25909e02..4240a779 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -862,12 +862,14 @@ static void dload_progress_event(const char *filename, alpm_download_event_progr int64_t curr_time = get_time_ms(); double last_chunk_rate; int64_t timediff; + bool ok;

if(!dload_progressbar_enabled()) { return; }

- assert(find_bar_for_filename(filename, &index, &bar)); + ok = find_bar_for_filename(filename, &index, &bar); + assert(ok);

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.

...

/* compute current average values */ timediff = curr_time - bar->sync_time; @@ -902,12 +904,14 @@ static void dload_complete_event(const char *filename, alpm_download_event_compl int index; struct pacman_progress_bar *bar; int64_t timediff; + bool ok;

if(!dload_progressbar_enabled()) { return; }

- assert(find_bar_for_filename(filename, &index, &bar)); + ok = find_bar_for_filename(filename, &index, &bar); + assert(ok); bar->completed = true;

/* This may not have been initialized if the download finished before -- 2.26.2

Andrew Gregory

14 May 14 May

2:54 a.m.

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. 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. At that point bar and index are indeterminate and the best we can hope for is a quick segfault. The callback api should be fixed so that callbacks can indicate an error that should cancel the download. 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.

Allan McRae

5:41 a.m.

On 14/5/20 12:54 pm, Andrew Gregory wrote: <snip>

...

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

Probably - there are a lot of points in the codebase where we pass an error back but don't really do much with it... Or don't pass back an error.

...

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 was fairly happy with the multi-download callback. It was at a point that it worked and was functionally correct (as far as I could tell), but there are likely ways to make it better such as your idea. However, I'm not letting that stop anything good and working being committed to the tree as improvements can happen later. Perfect being the enemy of good and all that. A

Anatol Pomozov

15 May 15 May

6:30 a.m.

Hi On Wed, May 13, 2020 at 7:54 PM Andrew Gregory 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/b96e0df4dceaa2677baa1a3563211950708d3e...

...

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?

Andrew Gregory

7:41 a.m.

On 05/14/20 at 11:30pm, Anatol Pomozov wrote:

...

Hi

On Wed, May 13, 2020 at 7:54 PM Andrew Gregory 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/b96e0df4dceaa2677baa1a3563211950708d3e...

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.

Allan McRae

14 May 14 May

5:44 a.m.

On 14/5/20 4:43 am, Dave Reisner wrote:

...

When building with -DNDEBUG, assert statements are compiled out to no-ops. Thus, we can't depend on assignments or other computations occurring inside the assert(). ---

Thanks.

...

It's perhaps worth mentioning that nowhere else in the ALPM codebase do we use assert().

Yes - but we really don't have a way to pass back from our callback to libalpm. So I am happy with the "shit hit the fan" approach until that changes.

...

src/pacman/callback.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 25909e02..4240a779 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -862,12 +862,14 @@ static void dload_progress_event(const char *filename, alpm_download_event_progr int64_t curr_time = get_time_ms(); double last_chunk_rate; int64_t timediff; + bool ok;

if(!dload_progressbar_enabled()) { return; }

- assert(find_bar_for_filename(filename, &index, &bar)); + ok = find_bar_for_filename(filename, &index, &bar); + assert(ok);

/* compute current average values */ timediff = curr_time - bar->sync_time; @@ -902,12 +904,14 @@ static void dload_complete_event(const char *filename, alpm_download_event_compl int index; struct pacman_progress_bar *bar; int64_t timediff; + bool ok;

if(!dload_progressbar_enabled()) { return; }

- assert(find_bar_for_filename(filename, &index, &bar)); + ok = find_bar_for_filename(filename, &index, &bar); + assert(ok); bar->completed = true;

/* This may not have been initialized if the download finished before

1453

Age (days ago)

1455

Last active (days ago)

List overview

Download

6 comments

4 participants

participants (4)

Allan McRae
Anatol Pomozov
Andrew Gregory
Dave Reisner