Page MenuHomePhabricator

make -Winteger-overflow find overflows in function arguments
ClosedPublic

Authored by nlewycky on Apr 7 2017, 7:21 PM.

Details

Reviewers
rsmith
Summary

When checkingForOverflow(), look through call arguments (and the callee itself if constexpr).

Diff Detail

Event Timeline

nlewycky created this revision.Apr 7 2017, 7:21 PM

Is it possible to fix ObjCMessageExpr too while you are in here?

I think clang should issue a warning when compiling the following code:

@protocol NSObject
@end

@interface NSObject<NSObject>
@end

@interface C1 : NSObject
- (void)foo:(int)i;
@end
@implementation C1
- (void)foo:(int)i {
}
@end

void test1(C1 *c) {
  [c foo:(4068 * 1024 * 1024)];
}

Is it possible to fix ObjCMessageExpr too while you are in here?

I looked into this, but it turns out to be different enough to belong in a separate patch. An ObjCMessageExpr has void type which means that we bail very early in the expression evaluator since void isn't a literal type. I think the original design of the code was to turn an Expr* into an APValue, and as we push it past that original purpose we're going to need to restructure it a bit.

I think clang should issue a warning when compiling the following code:

@protocol NSObject
@end

@interface NSObject<NSObject>
@end

@interface C1 : NSObject
- (void)foo:(int)i;
@end
@implementation C1
- (void)foo:(int)i {
}
@end

void test1(C1 *c) {
  [c foo:(4068 * 1024 * 1024)];
}

OK, thanks for looking into it. Warnings for ObjCMessageExpr can probably be implemented in a separate patch.

It looks like clang still doesn't issue overflow warnings when the called functions have a void return. Should we try to fix it in this patch too?

void foo(int);

void test0() {
  foo(4068 * 1024 * 1024); // no warnings
}

OK, thanks for looking into it. Warnings for ObjCMessageExpr can probably be implemented in a separate patch.

It looks like clang still doesn't issue overflow warnings when the called functions have a void return. Should we try to fix it in this patch too?

void foo(int);

void test0() {
  foo(4068 * 1024 * 1024); // no warnings
}

That testcase and the ObjCMessageExpr can go together in another patch where we fix visiting of non-literal-type expressions. This patch is really about inconsistent visiting of the arguments of a CallExpr.

There's a problem with this patch, we sometimes revisit nodes leading to exponential time. I've written a fix to that locally but it's not upstreamable quality yet.

nlewycky updated this revision to Diff 97253.Apr 30 2017, 7:04 PM

Use an RAII object to always evaluate the arguments, except if HandleFunctionCall does it.

nlewycky edited the summary of this revision. (Show Details)May 1 2017, 9:31 PM
rsmith accepted this revision.May 17 2017, 5:07 PM
This revision is now accepted and ready to land.May 17 2017, 5:07 PM
xbolva00 added a subscriber: xbolva00.EditedOct 27 2019, 7:47 AM

Clang already implements this behaviour.

xbolva00 closed this revision.Oct 27 2019, 7:52 AM