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

Update files.c #79

Closed
wants to merge 1 commit into from
Closed

Conversation

bryonglodencissp
Copy link

Checking ../honggfuzz-master/files.c: __clang__...
[../honggfuzz-master/files.c:144]: (error) Memory leak: buf
[../honggfuzz-master/files.c:371]: (error) Memory leak: inFileBuf

Found by https://github.com/bryongloden/cppcheck

Checking ../honggfuzz-master/files.c: __clang__...
[../honggfuzz-master/files.c:144]: (error) Memory leak: buf
[../honggfuzz-master/files.c:371]: (error) Memory leak: inFileBuf

Found by https://github.com/bryongloden/cppcheck
@robertswiecki
Copy link
Collaborator

Probably too many intermediate changes to merge it now. I'll go ahead and take a look individually for those reports.

@bryonglodencissp
Copy link
Author

We can sync with the upstream master then reapply our patch.
At that point merge in our fix so we get credit for authoring.
Sound okay @robertswiecki?

@anestisb
Copy link
Contributor

anestisb commented Sep 3, 2016

I think that cppcheck is not picking the defer blocks thus missing the free() calls triggered when function loses scope. For both reported cases the code inside the defer block will trigger the mentioned heap buffer frees. As such, I believe that they're both false positives.

I've run with valgrind under Android and haven't noticed any reported leaks. Do you have any different evidence from any memory inspection tool? If such, it means that defer blocks are not working as expected under certain cases / setups?

@bryonglodencissp
Copy link
Author

The defer construct is new to us, but if it's triggered after losing scope, it's logical that memory will be freed in both cases. We've had to adopt some unsound techniques in our tool because sound and complete static analysis is shown to be undecidable. Notwithstanding the logic of defer, manual inspection should have detected this false positive. Our apology for not catching that. Valgrind is a dynamic analysis tool, cppcheck is not. No other security testing tools were used for this bug scan.

@robertswiecki
Copy link
Collaborator

So... I guess we can close it?:)

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