Skip to content

Only rely on our own assert() #2473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kinkie wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Only rely on our own assert() #2473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
10bdbe6
remove PURIFY and NODEBUG
kinkie Aug 11, 2026
a887576
compat/assert.h is only used in c++ code
kinkie Aug 11, 2026
8d50e01
remove references to NDEBUG
kinkie Aug 11, 2026
c49c3b2
remove <cassert> references
kinkie Aug 11, 2026
41bbc56
Also use compat/assert.h from C files
kinkie Aug 11, 2026
956fa7a
no need to include compat/assert.g in debug/Stream.g
kinkie Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions compat/assert.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@
#ifndef SQUID_COMPAT_ASSERT_H
#define SQUID_COMPAT_ASSERT_H

#if PURIFY

@yadij yadij Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 yadij Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rousskov Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 kinkie Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rousskov Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 yadij Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rousskov rousskov Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Alex: 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,

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

I see no logical connection between "somebody had disable assertions in the past (and ran into trouble)" and "we need to support disabling assertions".

As for "PURIFY and NODEBUG build environment expectations", I would need to see specific squid-users discussions to confirm that they actually imply such expectations, but even if they do, it would clearly be isolated/rare incidents that should not affect the decision we are trying to make here!

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.

The typo is not separate from the "need to still honor ...NODEBUG build environment expectations" claim. If we fix that typo, we will no longer those specific expectations!

There is a weak argument for keeping the typo on grounds that the people using it now expect that instead of NDEBUG

Needless to say, I do not think we should support such alleged use cases, even if they exist.

I am asking to retain ability to suppress assert() from some builds.

Yes, that is clear. IMO, that request should be rejected (for the reasons detailed earlier).

The standards document says:

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

Squid assert() macro violates that standard today. This PR essentially says that Squid assert() is not the standard assert(). That name clash is bad, but it is better than the alternative. Someday, after the backlog is dealt with, we should rename Squid assert() to xassert() or some such to remove that name clash. Today, in this PR, we are codifying the meaning of Squid assert(), not the standard one.

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.

Regardless of whether Squid bugs violate any standards, disabling Squid assertions would result in a malfunctioning Squid today. There are at least two ways to address that: Do not support disabling Squid assertions and keep fixing difficult-to-find bugs. This PR proposes the former, for several important reasons, none of which have been invalidated in your blocking review.

Circa Squid-3.2, Henrik and I did a full audit of squid3 codebase and removed all such assert(...) calls.

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.

Both audits/checks missed at least one such bug. This should not be surprising -- it is very easy for humans to miss such bugs. As you probably know, a recent fix has added another assertion that, if disabled, resurrects a security vulnerability. There are probably more such cases, which is one of the primary reasons we should prohibit disabling Squid assertions: Disabling Squid assertions cripples Squid; the benefits of disabling assertions do not outweigh that and other associated drawbacks.

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.

I doubt such people exist, and no concrete evidence was offered to support this "relying" claim, but my argument is much stronger than "nobody uses this feature". I argue that Squid would be much better off, on several levels, if the (currently broken) feature is removed, even if there are people that use it today. Moreover, those "people", if they do exist, will most likely not be harmed by this change!

@kinkie, this PR is unfortunately dead AFAICT -- my original arguments in its defense were not invalidated, and I doubt I can convince Amos that we are better of with always-on assertions that are, unfortunately, spelled like the optional standard ones. Sorry for the time wasted. I bet we will now have to spend more time on fixing the feature that should be removed :-(.

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.

To me, those are Board meeting invites.

Can you point me at the minutes please.

No minutes were taken -- it was an informal discussion. Please let me know if you want us to record those meetings. I recommend showing up; we often discuss important Project matters, and most of these meetings are scheduled for the time that the three of us have agreed on (and that time can be adjusted if needed).

#define assert(EX) ((void)0)
#elif defined(NODEBUG)
#define assert(EX) ((void)0)
#else
#define assert(EX) ((EX)?((void)0):xassert( # EX , __FILE__, __LINE__))
#if defined(assert)
#undef assert
#endif

#define assert(EX) ((EX)?((void)0):xassert( # EX , __FILE__, __LINE__))

#ifdef __cplusplus
extern "C" void
#else
Expand All @@ -25,4 +23,3 @@ extern void
xassert(const char *, const char *, int);

#endif /* SQUID_COMPAT_ASSERT_H */

2 changes: 0 additions & 2 deletions compat/mswindows.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

#define sys_nerr _sys_nerr

#undef assert
#include <cassert>
#include <cstring>
#include <fcntl.h>
#include <memory>
Expand Down
1 change: 0 additions & 1 deletion lib/hash.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "squid.h"
#include "hash.h"

#include <cassert>
#include <cmath>
#include <cstdlib>
#include <cstring>
Expand Down
3 changes: 0 additions & 3 deletions lib/heap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@
#if HAVE_STDLIB_H
#include <stdlib.h>
#endif
#if HAVE_ASSERT_H
#include <assert.h>
#endif
#if HAVE_STRING_H
#include <string.h>
#endif
Expand Down
3 changes: 0 additions & 3 deletions lib/snmplib/parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,6 @@ SOFTWARE.
#if HAVE_NETDB_H
#include <netdb.h>
#endif
#if HAVE_ASSERT_H
#include <assert.h>
#endif
#if HAVE_ERRNO_H
#include <errno.h>
#endif
Expand Down
3 changes: 0 additions & 3 deletions lib/snmplib/snmp_msg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,6 @@
#if HAVE_NETDB_H
#include <netdb.h>
#endif
#if HAVE_ASSERT_H
#include <assert.h>
#endif

#include "asn1.h"
#include "snmp.h"
Expand Down
1 change: 0 additions & 1 deletion lib/tests/testRFC1738.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include "squid.h"
#include "unitTestMain.h"

#include <cassert>
Comment thread
kinkie marked this conversation as resolved.
#include <cppunit/extensions/HelperMacros.h>

/* Being a C library code it is best bodily included and tested with C++ type-safe techniques. */
Expand Down
10 changes: 0 additions & 10 deletions src/base/Assure.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
while (!(condition)) \
ReportAndThrow_((debugLevel), (description), (location))

#if !defined(NDEBUG)

/// Like assert() but throws an exception instead of aborting the process. Use
/// this macro to detect code logic mistakes (i.e. bugs) where aborting the
/// current AsyncJob or a similar task is unlikely to jeopardize Squid service
Expand All @@ -40,13 +38,5 @@
#define Assure2(condition, description) \
Assure_(0, (condition), ("assurance failed: " description), Here())

#else

/* do-nothing implementations for NDEBUG builds */
#define Assure(condition) ((void)0)
#define Assure2(condition, description) ((void)0)

#endif /* NDEBUG */

#endif /* SQUID_SRC_BASE_ASSURE_H */

1 change: 0 additions & 1 deletion src/base/JobWait.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "base/AsyncJobCalls.h"
#include "base/JobWait.h"

#include <cassert>
#include <iostream>

JobWaitBase::JobWaitBase() = default;
Expand Down
4 changes: 2 additions & 2 deletions src/base/TextException.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ std::ostream &operator <<(std::ostream &, const TextException &);
Assure_(3, (condition), ("check failed: " description), (location))

/// Like Assure() but only logs the exception if level-3 debugging is enabled
/// and runs even when NDEBUG macro is defined. Deprecated: Use Assure() for
/// code logic checks and throw explicitly when input validation fails.
/// Deprecated: Use Assure() for code logic checks and throw
/// explicitly when input validation fails.
#define Must(condition) Must3((condition), #condition, Here())

/// Reports and swallows all exceptions to prevent compiler warnings and runtime
Expand Down
11 changes: 0 additions & 11 deletions src/debug/Stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,8 @@
#include "mem/AllocatorProxy.h"

#include <iostream>
#undef assert
#include <sstream>
#include <iomanip>
#if defined(assert)
#undef assert
#endif
#if PURIFY
#define assert(EX) ((void)0)
#elif defined(NODEBUG)
#define assert(EX) ((void)0)
#else
#define assert(EX) ((EX)?((void)0):xassert( # EX , __FILE__, __LINE__))
#endif

/* defined debug section limits */
#define MAX_DEBUG_SECTIONS 100
Expand Down
1 change: 0 additions & 1 deletion src/dns/rfc1035.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include "dns/rfc2671.h"
#include "util.h"

#include <cassert>
#include <cstring>
#if HAVE_UNISTD_H
#include <unistd.h>
Expand Down
1 change: 0 additions & 1 deletion src/dns/rfc3596.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#if HAVE_MEMORY_H
#include <memory.h>
#endif
#include <cassert>
#if HAVE_NETINET_IN_H
#include <netinet/in.h>
#endif
Expand Down
1 change: 0 additions & 1 deletion src/ip/Address.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "ip/tools.h"
#include "util.h"

#include <cassert>
#include <cstring>
#if HAVE_ARPA_INET_H
/* for inet_ntoa() */
Expand Down
1 change: 0 additions & 1 deletion src/log/file/log_file_daemon.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

#include "compat/unistd.h"

#include <cassert>
#include <cerrno>
#include <csignal>
#include <cstring>
Expand Down
1 change: 0 additions & 1 deletion src/mem/Pool.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "mem/PoolMalloc.h"
#include "mem/Stats.h"

#include <cassert>
#include <cstring>

extern time_t squid_curtime;
Expand Down
1 change: 0 additions & 1 deletion src/mem/PoolChunked.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "mem/PoolChunked.h"
#include "mem/Stats.h"

#include <cassert>
#include <cstring>

#define MEM_MAX_MMAP_CHUNKS 2048
Expand Down
1 change: 0 additions & 1 deletion src/mem/PoolMalloc.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include "mem/PoolMalloc.h"
#include "mem/Stats.h"

#include <cassert>
#include <cstring>

extern time_t squid_curtime;
Expand Down
2 changes: 0 additions & 2 deletions src/tests/testRFC1035.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
#include "dns/rfc1035.h"
#include "unitTestMain.h"

// #include <cassert>

/*
* test the DNS resolver RFC 1035 Engine
*/
Expand Down
4 changes: 0 additions & 4 deletions test-suite/waiter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@

#include "squid.h"

#if HAVE_ASSERT_H
#include <assert.h>
#endif

int
main(int argc, char *argv[])
{
Expand Down
Loading