[cfe-commits] unix.Malloc static checker improvement: memory.LeakPtrValChanged

[Branden Archer]

>
> Basically, you need to pass a pointer which we are tracking to a function
> call indirectly (ex: as a field in a struct..). You should pass it to a
> function which is known not to free memory or close stream. Finally, you
> leak that resource/pointer.
>
> Previously, we would have a false negative - no leak would be reported.
> Now, we should be catching the leak.
>

Ah, got you. See the first attached patch for these added cases.

- Branden

On Thu, Jan 31, 2013 at 2:04 AM, Anna Zaks <ganna at apple.com> wrote:

>
> On Jan 28, 2013, at 7:31 PM, Branden Archer <b.m.archer4 at gmail.com> wrote:
>
> I'm still not sure why this enum needs any explicit integer values (the "=
>> 1").
>>
>
> No reason really. Easy enough to remove.
>
> No need for the comment - we are no longer getting the super region.
>>
>
> Oops.
>
> Also, please divide by ASTContext's getCharWidth(), not 8.
>>
>
> Is getCharWidth() guaranteed to always be 8? If not, the result will be
> incorrect. RegionOffset.getOffset() returns a number in bits, and if the
> char width on the system is more than 8 bits then the printed result will
> not make sense.
>
> "Argument to free() is offset by # bytes from the start of memory
>> allocated by malloc()"
>>
>
> I like it!
>
> We need to add a test case to the first patch that exercises the new
>> behavior.
>>
>
> I am not sure what sort of test case would be appropriate. I have added
> two test cases, one for the malloc checker and the other for the simple
> stream checker, for the case where the reference of interest is passed to a
> function that accepts it as a const. Is this maybe what you had in mind?
>
> The test case needs to exercise this change:
> -        SymbolsIndirectlyInvalidated, /*CallEvent*/ 0);
> +        SymbolsIndirectlyInvalidated, Call, PSK_IndirectEscapeOnCall);
>
> In conjunction with this :
> -  if (Call && guaranteedNotToCloseFile(*Call))
> +  if ((Kind == PSK_DirectEscapeOnCall ||
> +       Kind == PSK_IndirectEscapeOnCall) &&
> +      guaranteedNotToCloseFile(*Call)) {
>      return State;
> +  }
>
> Basically, you need to pass a pointer which we are tracking to a function
> call indirectly (ex: as a field in a struct..). You should pass it to a
> function which is known not to free memory or close stream. Finally, you
> leak that resource/pointer.
>
> Previously, we would have a false negative - no leak would be reported.
> Now, we should be catching the leak.
>
> Anna.
>
> - Branden
>
>
> On Fri, Jan 25, 2013 at 10:44 PM, Anna Zaks <ganna at apple.com> wrote:
>
>>
>> On Jan 25, 2013, at 6:31 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>
>> Almost there!
>>
>> +/// \brief Describes the different reasons a pointer escapes
>> +/// during analysis.
>> +enum PointerEscapeKind {
>> +  /// A pointer escapes due to binding its value to a location
>> +  /// that the analyzer cannot track.
>> +  PSK_EscapeOnBind = 1,
>> +
>> +  /// The pointer has been passed to a function call directly.
>> +  PSK_DirectEscapeOnCall,
>> +
>> +  /// The pointer has been passed to a function indirectly.
>> +  /// For example, the pointer is accessible through an
>> +  /// argument to a function.
>> +  PSK_IndirectEscapeOnCall,
>> +
>> +  /// The reason for pointer escape is unknown. For example,
>> +  /// a region containing this pointer is invalidated.
>> +  PSK_EscapeOther
>> +};
>>
>> I'm still not sure why this enum needs any explicit integer values (the
>> "= 1").
>>
>>
>> -        (RS->isReleased() ? "Attempt to free released memory" :
>> +        (RsBase->isReleased() ? "Attempt to free released memory" :
>>                              "Attempt to free non-owned memory"), N);
>>
>> I was going to say that the following line needs to be lined up as well,
>> but really I think this is an odd case where the prevailing style is to put
>> the : on the next line and line it up with the ?.
>>
>>
>> +      Offset.hasSymbolicOffset() == false &&
>>
>> Should just be !Offset.hasSymbolicOffset()
>>
>>
>> +void MallocChecker::ReportOffsetFree(CheckerContext &C, SVal ArgVal,
>> +                                     SourceRange range) const {
>>
>> Capital "range", please!
>>
>>
>> +    const MemRegion *MR = ArgVal.getAsRegion();
>> +    assert((MR) && "Only MemRegion based symbols can have offset free
>> errors");
>>
>> Good assertion; conventionally no parens around a single assertion
>> condition.
>>
>>
>> +    // Get the offset into the memory region before
>> +    // getting the super region, as retrieving the
>> +    // super region will ignore the offset.
>>
>> The comment's (still?) wrapped very aggressively here; you can unwind it
>> to 80 columns.
>>
>>
>> No need for the comment - we are no longer getting the super region.
>>
>>
>> +    os << "Argument to free() was not allocated by malloc(), but instead
>> is " <<
>> +          "a pointer offset by " << Offset.getOffset()/8  <<
>> +          " byte(s) from the start of allocated memory";
>>
>> Another case of operator inconsistency in the LLVM standards. We tend to
>> break before << and then line them up with the previous <<.
>>
>> Also, please divide by ASTContext's getCharWidth(), not 8.
>>
>> The message emitted from ReportOffsetFree is a little different from the
>> one that was emitted in ReportBadFree. Take a look and see if you agree
>> with its phrasing and the information it is providing.
>>
>>
>> I like the increased information! I think you can go even further,
>> though, and just directly say what's wrong, rather than leading with "was
>> not allocated by malloc()".
>>
>> "Argument to free() is offset by # bytes from the start of allocated
>> memory"
>>
>>
>> +1
>>
>> Or, if we want to mention "malloc".
>>
>> "Argument to free() is offset by # bytes from the start of memory
>> allocated by malloc()"
>>
>>
>> (Also, we can totally get "byte"/"bytes" correct!)
>>
>>
>> However, no expected-warning comment resulted in a failure. Is there a
>> special format for the expected-warning comment that I am missing, or is
>> the comparison that is taking place not quite right?
>>
>>
>> You were very close! expected-warning deliberately matches a *substring* of
>> the actual warning emitted, so that you can choose how sensitive it is
>> about the particular phrasing of the warning. Since this is the first test
>> for the offset warning, you probably want to use the full string.
>>
>>
>>  I sort of agree with part of both of the possibilities. I would rather
>> the assert be its own contained unit, but I do not like inverting the logic
>> in the assert, as it may make it a little harder to parse. How about this?
>>
>>   assert((Call != NULL ||
>>           (Kind != PSK_DirectEscapeOnCall &&
>>            Kind != PSK_IndirectEscapeOnCall)) &&
>>          "Call must not be NULL when escaping on call");
>>
>> I will probably be called on spacing rules though, as I tried to indent
>> pass the ( only if within the same set of (). That is, the Kinds should
>> line up, but the Call and "Call should not, as the first Call is within an
>> extra ().
>>
>>
>> That's probably the best we're going to get (and the best indentation),
>> and Anna agrees with you on preferring to keep everything inside the
>> assert. Let's go with this.
>>
>> After this I think it'll be ready to commit! Anna?
>>
>>
>> We need to add a test case to the first patch that exercises the new
>> behavior.
>>
>>  Otherwise, looks good.
>> Thanks Branden!
>>
>>
>> Jordan
>>
>>
>>
> <02-update-malloc-checker.patch><01-modify-checkPointerEscape.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/c44c8b2d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-modify-checkPointerEscape.patch
Type: application/octet-stream
Size: 12180 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/c44c8b2d/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02-update-malloc-checker.patch
Type: application/octet-stream
Size: 9683 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/c44c8b2d/attachment-0001.obj>