Cgl
Cgl copied to clipboard
fix undefined behavour due to memcpy(ptr, NULL, 0)
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 ;)
Partially fixed by https://github.com/coin-or/Cgl/commit/e5adc40d6a72b74a0df881e0dba2208e4933f545.
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.