Comment by ryao

21 hours ago

If they prove a NULL check is always false, it means you have dead code.

For example:

  if (p == NULL) return;

  if (p == NULL) doSomething();

It is safe to delete the second one. Even if it is not deleted, it will never be executed.

What is problematic is when they remove something like memset() right before a free operation, when the memset() is needed to sanitize sensitive data like encryption keys. There are ways of forcing compilers to retain the memset(), such as using functions designed not to be optimized out, such as explicit_bzero(). You can see how we took care of this problem in OpenZFS here:

https://github.com/openzfs/zfs/pull/14544

8 comments

ryao

Dylan16807 14 hours ago

If they prove a check is always false, it means you have dead code or you made a mistake.

It is very very hard to write C without mistakes.

When not-actually-dead code gets removed, the consequences of many mistakes get orders of magnitudes worse.

matheusmoreira 19 hours ago

They just think you have lots of dead code because of silly undefined behavior nonsense.

  char *allocate_a_string_please(int n)
  {
      if (n + 1 < n)
          return 0; // overflow

      return malloc(n + 1); // space for the NUL
  }

This code seems okay at first glance, it's a simple integer overflow check that makes sense to anyone who reads it. The addition will overflow when n equals INT_MAX, it's going to wrap around and the function will return NULL. Reasonable.

Unfortunately, we cannot have nice things because of optimizing compilers and the holy C standard.

The compiler "knows" that signed integer overflow is undefined. In practice, it just assumes that integer overflow cannot ever happen and uses this "fact" to "optimize" this program. Since signed integers "cannot" overflow, it "proves" that the condition always evaluates to false. This leads it to conclude that both the condition and the consequent are dead code.

Then it just deletes the safety check and introduces potential security vulnerabilities into the software.

They had to add literal compiler builtins to let people detect overflow conditions and make the compiler actually generate the code they want it to generate.

Fighting the compiler's assumptions and axioms gets annoying at some point and people eventually discover the mercy of compiler flags such as -fwrapv and -fno-strict-aliasing. Anyone doing systems programming with strict aliasing enabled is probably doing it wrong. Can't even cast pointers without the compiler screwing things up.

ryao 7 hours ago

I would consider the use of signed integers for sizes to be wrong, but if you insist on using them in this example, just test for (n == INT_MAX). malloc itself uses size_t, which is unsigned.
I have been known to write patches converting signed integers to unsigned integers in places where signed arithmetic makes no sense.
oguz-ismail 18 hours ago
>if (n + 1 < n)
No one does this
- matheusmoreira 11 hours ago
  
  Oh people absolutely do this.
  Here's a 2018 example.
  https://github.com/mruby/mruby/commit/180f39bf4c5246ff77ef71...
  https://github.com/mruby/mruby/issues/4062
  while (l >= bsiz - blen) { bsiz *= 2; if (bsiz < 0) mrb_raise(mrb, E_ARGUMENT_ERROR, "too big specifier"); }
  > bsiz*=2 can become negative.
  > However with -O2 the mrb_raise is never triggered, since bsiz is a signed integer.
  > Signed integer overflows are undefined behaviour and thus gcc removes the check.
  People have even categorized this as a compiler vulnerability.
  https://www.kb.cert.org/vuls/id/162289
  > C compilers may silently discard some wraparound checks
  And they aren't wrong.
  The programmer wrote reasonable code that makes sense and perfectly aligns with their mental model of the machine.
  The compiler took this code and screwed it up because it violates compiler assumptions about some abstract C machine nobody really cares about.
  
  3 replies →