Fix call to LocalFree() on uninit ptr inside getUidFromSid() #6579
Conversation
|
Good catch. It seems like we ought to be able to review the whole codebase for instances of uninitialized pointer use. Or use a static analyzer to catch these. |
|
Agreed. I've not been a Windows developer in a long time but there's probably some way to get Visual Studio to print warnings on uninit var use. I'm fairly sure GCC would catch this. |
There was a problem hiding this comment.
@uptycs-rmack Thanks for spotting this!
I was thinking though that this is somewhat a common pattern, not initialize the variable, when you know that an API is going to return something in it.
Also 9/10 times unless stated otherwise the free method shouldn't be called if the API failed (so here I would think that the issue is the unnecessary LocalFree call on error).
But this is really me just nitpicking and maybe leaving some thoughts if we want to tackle this in a different way.
This fix ok.
As for catching this issues in the future, static analysis might help yes, although it's not feasible to run it with the CI since it greatly slows down the build speed (and I don't think we could reliably have it fail the build if issues are encountered).
On Windows we do build with all warnings up to level 3 active (level 4 should be more of the recommendation kind).
On a developer machine one could use scan-build for Linux and macOS and Visual Studio Code Analyzer on Windows.
The Code Analyzer on Windows is freely available with the Community version of Visual Studio 2019.
One thing I know about that code analyzer though is that in some cases, especially with uninitialized stuff, it's a bit noisy and may contain a lot of false positives, but it would be still useful to use and have a look at osquery source code.
|
Yeah I wasn't really sure whether WIN32 APIs have any guarantees that they would clean up after themselves on a failure mode or it's possible they would allocate the memory before failing. I looked at the docs for this API call and it said nothing about whether we had to free on error or not, and LocalFree said it would happily ignore a NULL pointer, so here we are :-) |
On my windows test host I was encountering falling calls to ConvertSidToStringSidA() inside getUidFromSid(). Following a failed call we call LocalFree() on a potentially uninitialized pointer sidString. This was resulting in a segfault. Initializing the pointer to null fixes the crash.