PostgreSQL's approach to assertion usage: seeking best practices

From: Alexander Kuznetsov <kuznetsovam(at)altlinux(dot)org>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: nickel(at)altlinux(dot)org, egori(at)altlinux(dot)org
Subject: PostgreSQL's approach to assertion usage: seeking best practices
Date: 2024-08-09 10:01:41
Message-ID: 5273c2cb-1bde-413b-8bec-a87ec6bfc6b5@altlinux.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello everyone,

The ALT Linux Team has recently initiated a focused testing effort on PostgreSQL, with an emphasis on its security aspects.

As part of this effort, we conducted static analysis using Svace, which raised some questions regarding the use of the Assert() function.
We were unable to find clear answers in the documentation or previous discussions and would greatly appreciate your insights,
as these will influence how we approach future patch submissions:

1. General purpose of Assert() function:
- Is the primary purpose of the Assert() function to:
- Inform developers of the assumptions made in the code,
- Facilitate testing of new builds with assertions enabled,
- Accelerate development by temporarily placing assertions where defensive code may later be required,
- Or does it serve some other purpose?

2. Assertions and defensive code:
We have observed patches where assertions were added to defensive code (e.g., [1]) and where defensive code was added to assertions.
Is it generally advisable and encouraged to incorporate defensive code into assertions' assumptions?

For instance, we encountered cases where a null pointer dereference occurs immediately after an assertion that the pointer is not null.
In assertion-enabled builds, this results in an assertion failure, while in non-assert builds, it leads to a dereference, which we aim to avoid.
However, it is unclear to us whether the use of an assertion in such cases indicates that this flaw is known and will not be addressed specifically,
or if additional protective code should be introduced.

A clearer understanding of whether assertions should signal potential failure points that might later be rewritten or protected would assist us
in quickly developing fixes for such cases, particularly where we believe the issue could occur in practice.

3. Approach to fixing flaws:
How should we generally fix the flaws - by adding protection code, or by adding assertions?
I previously submitted two patches and encountered some confusion based on the feedback: in one instance,
I added protective code but was asked whether an assertion would suffice [2],
and in another, I added an assertion but was questioned on its necessity, given that it would cause a failure regardless, which I agreed with [3].

Your guidance on these matters would be invaluable in helping us align our patch submissions with the community’s best practices.

Thank you in advance for your time and assistance.

1. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=30aaab26e52144097a1a5bbb0bb66ea1ebc0cb81
2. https://www.postgresql.org/message-id/e22df993-cdb4-4d0a-b629-42211ebed582%40altlinux.org
3. https://www.postgresql.org/message-id/6d0323c3-3f5d-4137-af73-98a5ab90e77c%40altlinux.org

--
Best regards,
Alexander Kuznetsov

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Junwang Zhao 2024-08-09 10:19:11 Re: [Patch] remove duplicated smgrclose
Previous Message Michael Banck 2024-08-09 09:44:04 Re: Improve error message for ICU libraries if pkg-config is absent