Assertions are misused

[cperciva]

Quote from hitch.c:

#define AZ(foo)     do { assert((foo) == 0); } while (0)
...
            if (CONFIG->UID >= 0)
                AZ(setgroups(0, NULL));
            if (CONFIG->GID >= 0)
                AZ(setgid(CONFIG->GID));
            if (CONFIG->UID >= 0)
                AZ(setuid(CONFIG->UID));

This will break horribly if NDEBUG is declared, since that makes the assertions vanish -- along with their contents! While building with -DNDEBUG is generally a bad idea, since it turns off assertions, it should never result in privileges not being dropped. (There are other, less lethal, ways that functional code is placed inside assert() elsewhere.)

Either stop misusing assertions, add some code to throw an error if NDEBUG is defined, or fix AZ / AN to run the code first and then assert the value later, e.g.,
#define AZ(foo) do { int success = (foo) == 0; assert(success); } while (0)

[dridi]

This is something we do a lot in Varnish Cache, except that in Varnish you can safely put side effects in assert macros, there's no NDEBUG dependence.

I'd rather drop the standard assert.h and bake an in-house assert macro rather than making variants like AZ more convoluted. We may use the plain assert macros with side effects too.

However this is not my call, so I'll just thank you for reporting it :)

[daghf]

Hi Colin,

Thanks a lot for bringing this to our attention.

I echo what Dridi is saying, and I now have a patch prepared that borrows Varnish's assert handling code.