Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tosttost
Copy link
Contributor

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 ;)

@thesamesam
Copy link

Partially fixed by e5adc40.

@svigerske
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants