Cgl icon indicating copy to clipboard operation
Cgl copied to clipboard

fix undefined behavour due to memcpy(ptr, NULL, 0)

Open tosttost opened this issue 4 years ago • 2 comments

passing NULL to memcpy is undefined behaviour. But: Does memcpy actually causes trouble? probably not - it's hard to image an implementation that does not 'accidently' gets it right. I have no hint that any coin-or software misbehaves.

Why fix this?

  • if you run an application that uses Cgl (e.g. through Cbc) with UBSAN activated (remember that those sanitizers recomment to activate it for all used libraries, too), you get a lot of warnings about memcpy(ptr, NULL, 0) within Cgl. That is at least confusing.
  • Modern compiler are 'clever' and might deduce that all pointers passed to memcpy are not NULL and drop explicit NULL-checks as dead code. Example
#include <iostream>
#include <cstring>

void __attribute__ ((noinline)) foo(int* a, int* b, int n)
{
   memcpy(b, a, n);
   
   if(a == NULL)
   {
      std::cout << "a is NULL" << std::endl;
   }
}

int main()
{
   int* a = NULL;
   int b[3];
   foo(a, b, 0);

   return 0;
}

compiled with gcc without optimization, it prints the message, if you add "-O3" it doesn't.... The no inline is only needed to keep gcc from const-folding. So basically we are betting that the code around memcpy is not miscompiled/'optimized' by some clever compiler.

Open questions

  • I know 1 similar error in ClpParameters and 2 in CbcModel. Shall I move the helper function to CoinUtils and reuse it everywhere? There might be even more instances of the "resize array but don't use std::vector"-pattern.
  • I tried to keep as close to the original code as possible, so I didn't switch to std::vector. If the mantainers of Cgl have a different opinion: let me know, I can do the grunt work ;)

tosttost avatar Feb 21 '21 18:02 tosttost

Partially fixed by https://github.com/coin-or/Cgl/commit/e5adc40d6a72b74a0df881e0dba2208e4933f545.

thesamesam avatar Aug 22 '22 04:08 thesamesam

Moving append() into CoinUtils (https://github.com/coin-or/CoinUtils/blob/master/src/CoinHelperFunctions.hpp), renaming it to something CoinRealloc... like, and using it here would be nice. Having a function append() in the global namespace could be confusing.

svigerske avatar Aug 16 '24 10:08 svigerske