Page MenuHomePhabricator

Add kfree( ) to MallocChecker.cpp
Needs ReviewPublic

Authored by andrewmw94 on Aug 10 2016, 2:39 PM.

Details

Summary

Adds the kfree( ) function to MallocChecker.cpp

Diff Detail

Event Timeline

andrewmw94 updated this revision to Diff 67609.Aug 10 2016, 2:39 PM
andrewmw94 retitled this revision from to Add kfree( ) to MallocChecker.cpp.
andrewmw94 updated this object.
andrewmw94 set the repository for this revision to rL LLVM.
andrewmw94 added a project: Restricted Project.
andrewmw94 added a subscriber: cfe-commits.
andrewmw94 updated this revision to Diff 67610.Aug 10 2016, 2:42 PM

Change the test file kmalloc-linux.c to use kfree( ). Sorry if there was a way to put both patch files together; I wasn't able to find one.

dcoughlin commandeered this revision.Aug 12 2016, 10:56 AM
dcoughlin edited reviewers, added: andrewmw94; removed: dcoughlin.

Thanks for the patches! I've commandeered this revision to be able to update the diff to merge the two patch files into one. You should commandeer it back by using the 'Commandeer Revision' action.

By the way, there are instructions for how to create and upload diffs using Phabricator at http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

dcoughlin updated this revision to Diff 67862.Aug 12 2016, 10:57 AM
dcoughlin removed rL LLVM as the repository for this revision.

Merge the two patches into one diff.

dcoughlin added inline comments.Aug 12 2016, 11:36 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
593

It looks like you are using tabs here. The LLVM style guide calls for spaces. http://llvm.org/docs/CodingStandards.html#use-spaces-instead-of-tabs

If you haven't seen it, the clang-format tool can help automatically format code to conform to LLVM's expected style.

950

It looks like this method has recently been renamed "parameters()" and so your patch doesn't compile. Are you using top of trunk from the llvm.org repo?

1296

I think it would good to add a test that covers this code. There are analogous tests in test/Analysis/free.c -- that would be a good place for this added test.

1309

Should there be a test for this case, as well? This would be exercised if code is allocated with kmalloc() and then freed in some way other than kfree (such as free() or delete). There are analogous tests in the MismatchedDeallocator test files.

2206

There are tabs on these two lines, as well.

NoQ added a subscriber: NoQ.Aug 12 2016, 12:47 PM
andrewmw94 commandeered this revision.Aug 15 2016, 4:46 PM
andrewmw94 edited reviewers, added: dcoughlin; removed: andrewmw94.

I agree that more tests would be good. Where should I put them? Should it be in a separate file or should I just have kmalloc/kfree tests next to the malloc/free ones?

dcoughlin edited edge metadata.Aug 16 2016, 11:41 AM

I would recommend putting the tests in existing test files next to tests that for similar diagnostics for other allocation/free schemes. So the test for printExpectedAllocName() should go in test/Analysis/free.c and the test for printExpectedDeallocName() should probably go in test/Analysis/MismatchedDeallocator-checker-test.mm.

One more thing, obviously it should mimic malloc/free's behavior in complaining about delete/new being used. Should it also complain about free/malloc being used? I can't imagine that would be something people would usually intend to do, but I don't think it's really "incorrect." Thought?

One more thing, obviously it should mimic malloc/free's behavior in complaining about delete/new being used. Should it also complain about free/malloc being used? I can't imagine that would be something people would usually intend to do, but I don't think it's really "incorrect." Thought?

I'm not familiar with linux kernel programming. Is free() available in the kernel? If someone did define their own free() and used it with kmalloc'd memory, would they consider this a false positive?