coin-or / Cgl

Cut Generator Library
Other
24 stars 16 forks source link

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

Open tosttost opened 3 years ago

tosttost commented 3 years ago

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?

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 ;)
thesamesam commented 2 years ago

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

svigerske commented 1 month ago

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.