Only rely on our own assert()

[kinkie]

The interactions between our own assert() implementation
(in compat/assert.h) and the system-level assert()
are unclear and implementation-dependent.
Also, due to historic reasons, sometimes assert()
is used to validate user input, making building with debug
disabled a risky endeavour.

Remove references to system assert, and make assert()
unconditional

[rousskov]

I support this PR in principle. Thank you for starting this work. It will help dealing with vulnerability reports.

I will come back to this to change PR title/description in order to emphasize that we are changing both Squid functionality (when built with certain macros set) and setting or confirming the policy (that we can rely on our asserts always being there, in any supported builds).

[yadij]

We do need to still honor these PURIFY and NODEBUG build environment expectations. Just add the #undef condition above the existing #if PURIFY line.

[yadij]

We could also #include <cassert> in this compat file before replacing the system assert definition to ensure the appropriate file wrappers are defined to prevent re-include issues.

[rousskov]

We do need to still honor these PURIFY and NODEBUG build environment expectations.

I strongly disagree with a "need to honor" opinion and doubt that relevant "environment expectations" actually exist:

• NODEBUG is a typo (the well-known macro is spelled NDEBUG). We do not need to honor typos, even if somebody is using them. I am not aware of any production NODEBUG Squid use, and, more importantly, such use would result in a malfunctioning Squid because Squid makes essential calls wrapped in assert().

• PURIFY is a macro recognized by an ancient memory checker called purify. I could not find any signs of active purify or "purify plus" use today (for anything). Public documentation that I could find has not been updated since 2003, contains broken links, and resides on what looks like an "archived" or "forgotten" web site. Even if somebody is using that tool for Squid development today, Squid itself no longer has any non-assertion code that is conditioned on that macro, such macro-protected code did not even compile since before September 2006 (see commit 3a85184), and ignoring memory leaks/problems in assertion code is itself a bad idea if production code runs with assertions.

Just add the #undef condition above the existing #if PURIFY line.

This PR is valuable if it removes all PURIFY, NDEBUG, and NODEBUG support (among other things). That removal is inline with what was discussed at the last Board meeting that prompted this PR creation. FWIW, I would continue to support and actively participate in this PR if it retains its originally intended scope. Otherwise, this PR scope has to be changed (from what was discussed at the last meeting); I do not know what the new scope of this PR is going to be, but this PR description should clarify the new scope to facilitate future re-reviews.

[kinkie]

I agree with Alex here (obviously). Over time, we have used assert not only to check API contracts and internal invariants, but also possibly to perform user input validations. While it should be fine to not always check invariants, it would be dangerous to give up on user input validation, even if it is in rare corner cases.

[rousskov]

The most important problem solved by this PR is removing uncertainty for future development without paying a hefty price for that luxury. Today, in this context, we are facing this choice:

• Change nothing. Continue to witness clashes among developers and among vulnerability reporters/developers on the meaning of assertions in Squid. Continue to develop on a shaky/uncertain foundation and dealing with confusing code.

• Fix NODEBUG typos. Reduce PURIFY and define assert code duplication. Audit thousands of assertions and adjust a few to bring Squid code in compliance with the "assertions are optional" model.

• Merge the finalized version of this PR to bring Squid code in compliance with the "assertions are not optional" model.

Keeping the current status quo is clearly the worst long-term approach IMO, so I will focus on the remaining two options. Both models are reasonable. Both are used by modern projects with more development expertise. In Squid context, "assertions are not optional" model is simpler, much safer, and a lot cheaper in terms of our most precious resource (most development work has been done there already!). Thus, I think we should adopt "assertions are not optional" model and finalize/merge this PR.

P.S. I am OK with a "... but still do not put essential calls inside assertions" addendum that would eventually allow us to measure performance overhead of assertions and, if really justified, switch to the "assertions are optional" model in a distant future where associated safety loss is no longer such a significant concern.

[yadij]

We do need to still honor these PURIFY and NODEBUG build environment expectations.

I strongly disagree with a "need to honor" opinion and doubt that relevant "environment expectations" actually exist:

* `NODEBUG` is a typo (the well-known macro is spelled `NDEBUG`). We do not need to honor typos, even if somebody is using them. I am not aware of any production `NODEBUG` Squid use, and,

I am aware of multiple squid-users discussions where we have had to ask people to rebuild their Squid with assertions enabled. So there is definitely an expectation that the behaviour is supported.

You are right the macro in current Squid code having a typo. That could/should be fixed. But is separate from the need to keep supporting the assert() suppression. There is a weak argument for keeping the typo on grounds that the people using it now expect that instead of NDEBUG - I am not asking for that though. I am asking to retain ability to suppress assert() from some builds.

The standards document says:

The definition of the macro `assert` depends on another macro, `NDEBUG`, which is not defined by the standard library. 

That forms the expectation, and defines the API symbols which we are supposed to be providing logic for.

more importantly, such use would result in a malfunctioning Squid because Squid makes essential calls wrapped in assert().

That is a major C/C++ standard violation.

Circa Squid-3.2, Henrik and I did a full audit of squid3 codebase and removed all such assert(...) calls. If someone has been sneaking them in again past review, those bad code changes need to be fixed immediately. C/C++ code MUST run properly when the assert(...) parameter is dropped entirely from existence by the compiler.
That means no hidden side effects. Or, if some do happen, they are "harmless" things like garbage collection that could trigger at any time regardless of the assert().

FWIW, I have just finished an quick check of git grep assert output. Everything still looks good to me, no sign of the "essential calls" claimed to exist.

* `PURIFY` is a macro [recognized](https://public.dhe.ibm.com/software/rational/docs/v2003/purify/html/ht_api_misc.htm#MID_IFDEF) by an ancient memory checker called `purify`.

IIRC there was something else we found using PURIFY instead of NDEBUG. I you want to drop PURIFY and see what breaks now, I can accept that.

Just add the #undef condition above the existing #if PURIFY line.

This PR is valuable if it removes all PURIFY, NDEBUG, and NODEBUG support (among other things).

I still object to removing the NODEBUG/NDEBUG support. Fix the typo so we don't have to argue about the naming again - but yes there are people relying on builds without assert() defined.

That removal is inline with what was discussed at the last Board meeting that prompted this PR creation.

There was a board meeting? I've only see invites to developer chats and core meetings. Can you point me at the minutes please.

if really justified, switch to the "assertions are optional" model in a distant future where associated safety loss is no longer such a significant concern.

Er, assertions have always been optional in C/C++ at the language level. AFAIK there is nothing to "switch to" - just bad coding mistakes that we eradicated over a decade ago from Squid code.

[yadij]

We do need to still honor these PURIFY and NODEBUG build environment expectations.

I strongly disagree with a "need to honor" opinion and doubt that relevant "environment expectations" actually exist:

* `NODEBUG` is a typo (the well-known macro is spelled `NDEBUG`). We do not need to honor typos, even if somebody is using them. I am not aware of any production `NODEBUG` Squid use, and,

I am aware of multiple squid-users discussions where we have had to ask people to rebuild their Squid with assertions enabled. So there is definitely an expectation that the behaviour is supported.

You are right the macro in current Squid code having a typo. That could/should be fixed. But is separate from the need to keep supporting the assert() suppression. There is a weak argument for keeping the typo on grounds that the people using it now expect that instead of NDEBUG - I am not asking for that though. I am asking to retain ability to suppress assert() from some builds.

The standards document says:

The definition of the macro `assert` depends on another macro, `NDEBUG`, which is not defined by the standard library. 

That forms the expectation, and defines the API symbols which we are supposed to be providing logic for.

more importantly, such use would result in a malfunctioning Squid because Squid makes essential calls wrapped in assert().

That is a major C/C++ standard violation.

Circa Squid-3.2, Henrik and I did a full audit of squid3 codebase and removed all such assert(...) calls. If someone has been sneaking them in again past review, those bad code changes need to be fixed immediately. C/C++ code MUST run properly when the assert(...) parameter is dropped entirely from existence by the compiler.
That means no hidden side effects. Or, if some do happen, they are "harmless" things like garbage collection that could trigger at any time regardless of the assert().

FWIW, I have just finished an quick check of git grep assert output. Everything still looks good to me, no sign of the "essential calls" claimed to exist.

* `PURIFY` is a macro [recognized](https://public.dhe.ibm.com/software/rational/docs/v2003/purify/html/ht_api_misc.htm#MID_IFDEF) by an ancient memory checker called `purify`.

IIRC there was something else we found using PURIFY instead of NDEBUG. I you want to drop PURIFY and see what breaks now, I can accept that.

Just add the #undef condition above the existing #if PURIFY line.

This PR is valuable if it removes all PURIFY, NDEBUG, and NODEBUG support (among other things).

I still object to removing the NODEBUG/NDEBUG support. Fix the typo so we don't have to argue about the naming again - but yes there are people relying on builds without assert() defined.

That removal is inline with what was discussed at the last Board meeting that prompted this PR creation.

There was a board meeting? I've only see invites to developer chats and core meetings. Can you point me at the minutes please.

if really justified, switch to the "assertions are optional" model in a distant future where associated safety loss is no longer such a significant concern.

Er, assertions have always been optional in C/C++ at the language level. AFAIK there is nothing to "switch to" - just bad coding mistakes that we eradicated over a decade ago from Squid code.