clang static analyzer: potential memory leaks and unitialized values

[floooh]

When running the code through the clang static analyzer (for instance in Xcode -> Product -> Analyzer), there's a small number of warnings (mostly uncritical like 'Values stored to X is never read'), but there are a couple of more critical warning in glslangValidator/StandAlone.cpp where memory is not freed, and a function is potentially called with an unitialized value (very likely a false positive because the code should return, but easy to fix).

Here's a list of the critical warnings (everything that is not a 'is never read' warning):

Potential leak of memory pointed to by 'config' in StandAlone.cpp/ProcessConfigFile():

If there's no config file given, a default 'config' is used which is allocated here: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L239, the static analyzer thinks that is is never freed.

Apart from that warning, there are several allocations happening in ReadFileData(), which don't seem to have a corresponding free() call (for instance here: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L965.

Also in FreeFileData() it looks like only the 'node pointers' are freed, not the array that contains the pointers allocated here: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L953

Potential leak of memory pointed to by 'program':

The object created here: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L619

...is not deleted when this if is taken: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L627

Potential leak of memory pointed to by 'return_data':

The memory allocated here: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L953

Is not freed when this 'if' is taken: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L956

Function call argument is an unitialized value:

This is very likely a false positive, but easy to fix: clang analyzer says that the 'FILE* in' here: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L948

...can remain unitialized if the call to fopen_s returns with an error when fgetc() is called here: https://github.com/KhronosGroup/glslang/blob/master/StandAlone/StandAlone.cpp#L960

This is the list of 'Value stored in X is never read', some of these may point to actual bugs, not sure:

Value stored to 'ch' is never read:

https://github.com/KhronosGroup/glslang/blob/master/glslang/MachineIndependent/preprocessor/PpContext.h#L411

Value stored to 'fragOutHasLocation' is never read:

https://github.com/KhronosGroup/glslang/blob/master/glslang/MachineIndependent/linkValidate.cpp#L557

Value stored to 'token' is never read:

https://github.com/KhronosGroup/glslang/blob/master/glslang/MachineIndependent/preprocessor/Pp.cpp#L831

Value stored to 'isVersion' is never read:

https://github.com/KhronosGroup/glslang/blob/master/glslang/MachineIndependent/preprocessor/Pp.cpp#L867

Value stored to 'exp' is never read:

https://github.com/KhronosGroup/glslang/blob/master/glslang/MachineIndependent/preprocessor/PpScanner.cpp#L182

Value stored to 'word' is never read:

https://github.com/KhronosGroup/glslang/blob/master/SPIRV/SPVRemapper.cpp#L457
https://github.com/KhronosGroup/glslang/blob/master/SPIRV/SPVRemapper.cpp#L469

Value stored to 'nextInst' during its initialization is never read:

https://github.com/KhronosGroup/glslang/blob/master/SPIRV/SPVRemapper.cpp#L469

And that is all :) I can provide a pull request for most of these if you want (the only part that's a bit more complex is the allocation/free stuff around ReadFileData / FreeFileData).

[johnkslang]

Tear down in the standalone app has not been a priority (since it exits), but I understand it is better to have it run clean. (Of far greater concern would be issues within the exposed programmatic interface.) I see ReadFileData() is pretty ugly archaic code. I fixed most of these as I read through your note (don't agree yet with the "'token' is never read" one, and the fragOutHasLocation is possibly useful when the functionality is finished). If there are others remaining, feel free to submit a pull request. (Except for SPVRemapper, someone is looking at that.)

Note that the original purpose of maxSourceString was to test splitting tokens across multiple strings, which makes the ReadFileData() code appear overly complex.

Thanks.

[johnkslang]

The SPVRemapper has addressed this now. I think the key issues have been addressed. Will leave open a bit longer for any addition related issues, and will then close (or floooh can close).

[floooh]

Just did a quick analyzer run, there are a couple of uncritical 'value stored to xxx is never read' left, and no new warnings, so I'm closing this ticket :)