This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Taught the analyzer about Glib API to check Memory-leak
ClosedPublic

Authored by xiangzhai on Jan 5 2017, 1:08 AM.

Details

Summary

Hi LLVM developers,

I have taught the analyzer about Glib API to check Memory-leak issue, please review my patch, thanks a lot!

Regards,
Leslie Zhai

Diff Detail

Repository
rL LLVM

Event Timeline

xiangzhai updated this revision to Diff 83202.Jan 5 2017, 1:08 AM
xiangzhai retitled this revision from to Taught the analyzer about Glib API to check Memory-leak.
xiangzhai updated this object.
xiangzhai added reviewers: zaks.anna, dcoughlin, rnk.
xiangzhai set the repository for this revision to rL LLVM.
zaks.anna edited edge metadata.Jan 5 2017, 9:37 AM

Thanks for the patch!

Could you, please, resubmit the patch with context as described here http://llvm.org/docs/Phabricator.html
Also, please, add tests. See test/Analysis/ for examples.

xiangzhai retitled this revision from Taught the analyzer about Glib API to check Memory-leak to [analyzer] Taught the analyzer about Glib API to check Memory-leak.Jan 5 2017, 6:06 PM
xiangzhai edited edge metadata.
xiangzhai updated this revision to Diff 83339.Jan 5 2017, 7:15 PM

Hi Anna,

Thanks for your review!

I resubmit the patch with context, please check is it correct, thanks a lot!
And I add testcase: test/Analysis/gmalloc.c

Phabricator still says that context is not available. Please, pass -U9999 when generating the patch.

Thanks!
Anna

test/Analysis/gmalloc.c
1

Please, just include core checkers and the Malloc checker. There is no need for UnreachableCode, CastSize and ExprInspection here.

43

This test will fail. For example, I am getting:
$ /Volumes/Data/ws/build/Ninja-DebugAssert+cmark-ReleaseAssert/llvm-macosx-x86_64/./bin/clang -cc1 -internal-isystem /Volumes/Data/ws/build/Ninja-DebugAssert+cmark-ReleaseAssert/llvm-macosx-x86_64/bin/../lib/clang/4.0.0/include -nostdsysteminc -analyze -analyzer-checker=osx.SecKeychainAPI,unix.Malloc -fblocks /Volumes/Data/ws/clang/test/Analysis/gmalloc.c -verify
error: no expected directives found: consider use of 'expected-no-diagnostics'
error: 'warning' diagnostics seen but not expected:

File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 43: Attempt to free released memory
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 70: Use of memory after it is freed
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 79: Potential leak of memory pointed to by 'g4'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 82: Potential leak of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 83: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 85: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 87: Potential leak of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 106: Potential leak of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 107: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 109: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 111: Potential leak of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 130: Potential leak of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 131: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 133: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 135: Potential leak of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 155: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 157: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 159: Potential leak of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 179: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 181: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 183: Potential leak of memory pointed to by 'g7'

22 errors generated.

The "-verify" option you pass on command line to the test checks that every warning/analyzer issue matches a comment with a specific pattern:
// expected-warning {"Text of warning here"}

You should run the tests to make sure all of them pass after your patch. If you build with ninja, you's run "ninja check-clang" if you build with make, you can follow instructions here http://clang-analyzer.llvm.org/checker_dev_manual.html#testing

xiangzhai updated this revision to Diff 83587.Jan 8 2017, 11:29 PM

Hi Anna,

Thanks for your review!

I use:

svn diff --diff-cmd=diff -x -U999999 lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/gmalloc.c > Glib-MallocChecker.patch

to generate the patch, please check is it correct.

And I run:

clang -cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-store=region -verify gmalloc.c

There is *NO* issue, please check it, thanks a lot!

Regards,
Leslie Zhai

xiangzhai updated this revision to Diff 83758.EditedJan 9 2017, 5:49 PM
xiangzhai marked 2 inline comments as done.

Make II_g_*malloc*_n more sensible!

zaks.anna added inline comments.Jan 12 2017, 4:15 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
810

Why did you remove the old behavior here and below?

I would expect this patch to be strictly additive. If gmalloc APIs take a different number of arguments, please, process them separately. You might need to factor out the processing code to avoid code duplication.

838

Should this be conditional on the number of arguments instead of adding two calls to ProcessZeroAllocation?

846

This change in how calloc is handled broke the Analysis/malloc.c test.

xiangzhai updated this revision to Diff 84209.Jan 12 2017, 6:13 PM

Hi Anna,

Thanks for your review!

I process them (for example, g_malloc_n, take a different number of arguments) separately.

And I run the testcases for Analysis/malloc.c and Analysis/gmalloc.c

Regards,
Leslie Zhai

kalev added a subscriber: kalev.Feb 8 2017, 12:01 AM
zaks.anna added inline comments.Feb 17 2017, 4:56 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
885

I am not sure this is correct as the third argument is "size of allocation", which in this case would be the value of CE->getArg(0) times the value of CE->getArg(2).

The current implementation of MallocMemAux would need to be extended to incorporate this:
` // Set the region's extent equal to the Size parameter.

const SymbolicRegion *R =
    dyn_cast_or_null<SymbolicRegion>(RetVal.getAsRegion());
if (!R)
  return nullptr;
if (Optional<DefinedOrUnknownSVal> DefinedSize =
        Size.getAs<DefinedOrUnknownSVal>()) {
  SValBuilder &svalBuilder = C.getSValBuilder();
  DefinedOrUnknownSVal Extent = R->getExtent(svalBuilder);
  DefinedOrUnknownSVal extentMatchesSize =
      svalBuilder.evalEQ(State, Extent, *DefinedSize);

  State = State->assume(extentMatchesSize, true);
  assert(State);
}`

My suggestion is to submit the patch without the 'n' variants and extend MallocMemAux to deal with them as a follow up patch.

889

Should this be 'getNumArgs() < 3' ?

891

Unfortunately, ReallocMem also assumes a single size argument:

` // Get the size argument. If there is no size arg then give up.

const Expr *Arg1 = CE->getArg(1);
if (!Arg1)
  return nullptr;`
xiangzhai marked 3 inline comments as done.Feb 19 2017, 7:14 PM

Hi Anna,

Thank you so much for the review!

I will try to implement extend MallocMemAux and ReallocMem in the follow up patch.

Regards,
Leslie Zhai

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
885

Hi Anna,

Thanks for your review!

I am not sure this is correct as the third argument is "size of allocation", which in this case would be the value of CE->getArg(0) times the value of CE->getArg(2).

g_malloc_n``` just use two arguments https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-malloc-n

so in this case would be the value of CE->getArg(0) x CE->getArg(1)?

> My suggestion is to submit the patch without the 'n' variants and extend MallocMemAux to deal with them as a follow up patch.

I will drop XXX_n from this patch, then implement them until extend MallocMemAux and ReallocMem implemented in the follow up patch.

Regards,
Leslie Zhai
xiangzhai added a comment.EditedFeb 19 2017, 8:28 PM

Hi Anna,

I am not clear why need to calculate the precise allocated size?

For example:

void *ptr = malloc(size);
void *gptr = g_malloc_n(n_blocks, n_block_bytes);

Memory-leak and Use-after-free issues' MallocChecker need the *size* or *n_blocks* x *n_block_bytes*, but if the size is inaccurate, the checker is also *able* to detect the issues https://bugzilla.gnome.org/show_bug.cgi?id=777077

Regards,
Leslie Zhai

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
889

sorry typo...

xiangzhai updated this revision to Diff 89331.EditedFeb 22 2017, 12:48 AM

Hi Anna,

I added svalBinMulOp to take BO_Mul evalBinOp for g_malloc_n's two arguments: State->getSVal(CE->getArg(0), C.getLocationContext()) MUL State->getSVal(CE->getArg(1), C.getLocationContext()), so it does NOT need to change MallocMemAux any more, but just use it in this way:

State = MallocMemAux(C, CE,  
  svalBinMulOp(C, State->getSVal(CE->getArg(0), C.getLocationContext()), 
  State->getSVal(CE->getArg(1), C.getLocationContext()), State), 
  UndefinedVal(), State);

And I also added ReallocMemN for g_realloc_n, it also use svalBinMulOp for !StateSizeIsZero check.

Please indicate my mistake, thanks a lot!

Regards,
Leslie Zhai

xiangzhai updated this revision to Diff 89333.Feb 22 2017, 1:19 AM

Fixed the confused

State->getSVal(CE->getArg(1), C.getLocationContext());

with

CE->getArg(1)

issue.

Could you please split the patch into two - one with the previously reviewed support for functions that take a single size value and another patch that models the two size arguments (num and size). It's easier to review patches if they do not grow new functionality. Splitting the patch would also play nicely with the incremental development policy of LLVM.

Thanks!

xiangzhai updated this revision to Diff 89460.Feb 22 2017, 7:31 PM

Hi Anna,

Thanks for your suggest!

Firstly I uploaded Glib-MallocChecker-single-size-value.patch for code review, if submitted to UPSTREAM, then upload another one, correct?

Regards,
Leslie Zhai

Firstly I uploaded Glib-MallocChecker-single-size-value.patch for code review, if submitted to UPSTREAM, then upload another one, correct?

Yes. By the way, you can model XXX_n variants similarly to how calloc is modeled (see CallocMem).

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
785

g_malloc0 needs to be initialized with zeros, not UndefinedVal(). See the relevant part in MallocChecker::CallocMem:

SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy);
return MallocMemAux(C, CE, TotalSize, zeroVal, State);
xiangzhai updated this revision to Diff 89825.EditedFeb 26 2017, 5:50 PM

Hi Anna,

Thanks for your review!

I updated the Glib-MallocChecker-single-size-value.patch to use zeroVal for g_malloc0 and g_try_malloc0

and Yes, I refer CallocMem to implement SValBinMulOp in the Glib-MallocChecker-all-in-one.patch if Glib-MallocChecker-single-size-value.patch could be submitted to UPSTREAM, then I create another patch for XXX_n, thanks for your suggest!

Regards,
Leslie Zhai

zaks.anna accepted this revision.Mar 1 2017, 2:01 PM

I am not clear why need to calculate the precise allocated size?

The information generated by this checker is used for array bounds checking. For example, see https://reviews.llvm.org/D24307

This patch looks good. Do you have commit access or should I commit it on your behalf?

This revision is now accepted and ready to land.Mar 1 2017, 2:01 PM

Hi Anna,

Thanks for your review!

The information generated by this checker is used for array bounds checking. For example, see https://reviews.llvm.org/D24307

I will read carefully about that ;-)

This patch looks good. Do you have commit access or should I commit it on your behalf?

Please commit it on my behalf, thanks a lot!

Regards,
Leslie Zhai

This revision was automatically updated to reflect the committed changes.