Page MenuHomePhabricator

[analyzer] MallocOverflow should consider comparisons only preceding malloc
ClosedPublic

Authored by steakhal on Aug 10 2021, 2:03 AM.

Details

Summary

MallocOverflow works in two phases:

  1. Collects suspicious malloc calls, whose argument is a multiplication
  2. Filters the aggregated list of suspicious malloc calls by iterating over the BasicBlocks of the CFG looking for comparison binary operators over the variable constituting in any suspicious malloc.

Consequently, it suppressed true-positive cases when the comparison check was after the malloc call.
In this patch, the checker will consider the relative position of the relation check to the malloc call.

E.g.:

void *check_after_malloc(int n, int x) {
  int *p = NULL;
  if (x == 42)
    p = malloc(n * sizeof(int)); // Previously **no** warning, now it
                                 // warns about this.

  // The check is after the allocation!
  if (n > 10) {
    // Do something conditionally.
  }
  return p;
}

Diff Detail

Event Timeline

steakhal created this revision.Aug 10 2021, 2:03 AM
balazske added inline comments.Aug 11 2021, 8:00 AM
clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
163

Is it safe to check for position in source code, instead of the execution path?

165

This is not a PredTrue any more.

steakhal added inline comments.Aug 11 2021, 8:16 AM
clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
163

I don't think so. But it's already better than outright suppressing any finding.

165

Oh sure, I'm gonna fix it!

steakhal updated this revision to Diff 365921.Aug 12 2021, 12:05 AM
steakhal marked 2 inline comments as done.

PredTrue -> PrecedesMalloc

Okay, I think this change is aligned with the checker's original logic. I.e. we have a heuristic, so we do not warn if we see a check of the involved variable...
I was digging a bit and found a related SEI CERT rule. MEM35-C Perhaps this checker could warn on the second code example?

On the other hand, on the long term I'd like to see this checker completely removed without any trace. I mean, generally, we should be able to warn if the result of any binary operation may not be represented in the underlying type. I think, this should be handled with an orchestration of a clever constraint solver and the engine.

Okay, I think this change is aligned with the checker's original logic. I.e. we have a heuristic, so we do not warn if we see a check of the involved variable...
I was digging a bit and found a related SEI CERT rule. MEM35-C Perhaps this checker could warn on the second code example?

The checker in it's current form, would warn for the very first example, and only for that example:

void *alloc_blocks(size_t num_blocks) {
  if (num_blocks == 0)
    return NULL;
  unsigned long long alloc = num_blocks * BLOCKSIZE ;
  return (alloc < UINT_MAX) ? malloc(num_blocks * BLOCKSIZE ) : NULL;
  // expected-warning@-1 {{the computation of the size of the memory allocation may overflow}}
}

It wouldn't warn for the rest of the examples because those have some sort of comparison checks on the variable.

On the other hand, on the long term I'd like to see this checker completely removed without any trace. I mean, generally, we should be able to warn if the result of any binary operation may not be represented in the underlying type. I think this should be handled with an orchestration of a clever constraint solver and the engine.

We cannot have a witness for overflow if we calculate on unknown symbolic values, like top-level function parameters, etc. In that sense, the checker relaxes the bug condition to look for potential overflowing multiplications in a specific context, in malloc parameters. That being said, a smarter constraint solver wouldn't help much.

martong added inline comments.Aug 18 2021, 9:39 AM
clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
163

Do we somehow assure that the two locations are inside the same stack frame? I mean @balazske has a point and I think this solution is okay if we compare two locations inside the same function.

clang/test/Analysis/malloc-overflow.c
115–124

What happens if the check happens in an inlined function? The location is above the malloc in the TU, isn't it?

int check(int n) 
  return (n > 10);
}

void *check_before_malloc(int n, int x) {
  int *p = NULL;
  if (x == 42)
    p = malloc(n * sizeof(int)); // no-warning?

  if (check(n))
    return NULL;

  // Do some other stuff, e.g. initialize the memory.
  return p;
}
steakhal updated this revision to Diff 368603.Aug 25 2021, 4:10 AM
steakhal marked an inline comment as done.

Add a test using macros. It demonstrates that the source locations are properly resolved.
@martong Let me know if you have anything.

clang/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
163

I would assert this fact, but I'm not exactly sure how to acquire the enclosing function of a SourceLocation.

clang/test/Analysis/malloc-overflow.c
115–124

The checker conducts a simple AST-matching, thus it wouldn't resolve the check(n) call. So, the checker will fire regardless of where you put the check(n) call, before or after the malloc().


Even in case of macros, it still works. Check the updated test file.

steakhal updated this revision to Diff 368679.Aug 25 2021, 10:37 AM

Pin the -triple to x86_64

martong requested changes to this revision.Aug 26 2021, 6:37 AM

Upsz-upsz, something went terribly off here! Flexible array members related changes come in with the last diff. For example, the file flexible-array-members.c came in. It might be perhaps a rebase issue if the FAM related change had been commited already, could you please rebase this patch?

This revision now requires changes to proceed.Aug 26 2021, 6:37 AM
steakhal updated this revision to Diff 368864.Aug 26 2021, 6:51 AM

The right diff this time :D

martong accepted this revision.Aug 26 2021, 8:26 AM

Thanks, LGTM!

This revision is now accepted and ready to land.Aug 26 2021, 8:26 AM
steakhal updated this revision to Diff 369031.Aug 26 2021, 11:39 PM
steakhal marked an inline comment as done.

Rebased. Removed the given limitation from the user-facing documentation, that this patch fixes.

Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2021, 5:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Probably it is too late, but the title of this change is misleading. I think is should be either MallocOverflow should consider comparisons AFTER malloc or MallocOverflow should NOT consider comparisons only preceding malloc.