Page MenuHomePhabricator

Add attribute for return values that shouldn't be cast to bool
Needs ReviewPublic

Authored by urusant on Sep 13 2016, 7:32 AM.

Details

Summary

Hi,

I am interested in feedback on a patch I was working on that adds a new attribute (warn_impcast_to_bool) to indicate that the return value of a function shouldn't be used as a boolean, as well as a compile warning and a StaticAnalyzer checker to warn about misusing functions with this attribute. I'd also appreciate any suggestions for how to deal with a class of false positives that the static analysis checker produces.

The change was originally inspired by CVE-2008-5077 [1], which was the result of an odd design choice in OpenSSL: having an API that returns 1 for success, 0 for failure... and -1 for catastrophic failure. Various API users fell into the trap of treating the return value as a boolean, so the patch adds an attribute to allow this to trigger a warning.

As well as generating a compile-time warning, the patch also includes a new static analyzer checker to catch more indirect uses, where the non-boolean integer value gets propagated via function wrappers or local variables. However, it gives a false positive for cases when the code using the return value actually checks the value in a non-boolean way (because the SVal doesn't reflect the fact that the value has been further constrained). I couldn't see an obvious way to get anything relevant from the RangeConstraintManager; any suggestions?

To test the check (beyond the included unit tests), I annotated dangerous OpenSSL functions and tried building 8 OpenSSL-using codebases with it. So far, this didn't give many results for them: the only possible problem was found in ruby2.1, which was already fixed a few months ago. However, this change is still potentially useful - even 7 years after the original CVE, there are still codebases that fall into OpenSSL's API trap.

[1] https://www.openssl.org/news/secadv/20090107.txt

Diff Detail

Repository
rL LLVM

Event Timeline

urusant updated this revision to Diff 71157.Sep 13 2016, 7:32 AM
urusant retitled this revision from to Add attribute for return values that shouldn't be cast to bool.
urusant updated this object.
urusant set the repository for this revision to rL LLVM.
urusant added subscribers: cfe-commits, daviddrysdale.
aaron.ballman edited edge metadata.Sep 15 2016, 2:07 PM

Thank you for working on this check! A few comments:

The patch is missing Sema tests for the attribute (that it only applies to declarations you expect, accepts no args, etc).

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration? Right now, the diagnostic you receive on a conversion may be spatially separated from where the user called the function (including crossing translation unit boundaries, which the static analyzer doesn't currently handle). By putting the attribute on the type, it carries more obvious semantic meaning and means the check can happen entirely in the frontend (no static analyzer required). For instance: typedef __attribute__((warn_impcast_to_bool)) IntNotABool; I'm not certain if this is a better design or not, but I am wondering if it was something you had considered.

include/clang/Basic/Attr.td
1138

This should not use a GCC spelling because it's not an attribute that GCC supports. You should probably use GNU instead, since I suspect this attribute will be useful in C as well as C++.

1140

No need to specify the WarnDiag or ExpectedFunctionOrMethod arguments; they will be handled automatically.

include/clang/Basic/AttrDocs.td
2058

You should manually wrap this to roughly the 80 col limit.

Instead of "he", can you use "they" please?

include/clang/Basic/DiagnosticGroups.td
57 ↗(On Diff #71157)

I'm not certain this requires its own diagnostic group. This can probably be handled under BoolConversion

include/clang/Basic/DiagnosticSemaKinds.td
2259

How about: ...only applies to integer return types?

2883

I don't think this should be a DefaultIgnore diagnostic -- if the user wrote the attribute, they should get the diagnostic when appropriate.

lib/Sema/SemaChecking.cpp
8262

Should use if (const auto *CE = dyn_cast<CallExpr>(E)) {

8263–8264

Then you can do if (const auto *Fn = CE->getDirectCallee()) {

8269

You can pass in fn directly, the diagnostics engine will properly get the name out of it because it's derived from NamedDecl.

lib/Sema/SemaDeclAttr.cpp
1316

Formatting seems off -- you should run the patch through clang-format. Also, why are you passing an empty SourceRange?

test/ReturnNonBoolTestCompileTime.cpp
40 ↗(On Diff #71157)

Can you end the file with a newline?

zaks.anna added inline comments.Sep 15 2016, 9:34 PM
include/clang/Basic/AttrDocs.td
2055

You probably need to "propose" the attribute to the clang community. I'd send an email to the cfe-dev as it might not have enough attention if it's just the patch.

test/ReturnNonBoolTest.c
74 ↗(On Diff #71157)

I do not understand why this is a false positive. In restricted_wrap, r can be any value. You only return '0' if r is '-1', but it could be '-2' or '100', which are also not bool and this values would just get returned.

You should be able to query the state to check if a value is a zero or one using code like this from CStringChecker.cpp:
"

SValBuilder &svalBuilder = C.getSValBuilder();
DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
return state->assume(svalBuilder.evalEQ(state, *val, zero))

"

urusant updated this revision to Diff 71807.Sep 19 2016, 5:44 AM
urusant edited edge metadata.

Made some changes based on the comments. Please refer to the replies below.

Thank you for the feedback.

The patch is missing Sema tests for the attribute (that it only applies to declarations you expect, accepts no args, etc).

There is one test case for that in test/ReturnNonBoolTestCompileTime.cpp. I've added another one for attribute accepting no args, so now the last two test cases in this file are those you were asking about. Can you think of any other cases of invalid attribute usage?

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration?

No, I hadn't. On a quick look though, I couldn't find a way to simplify my solution using this idea, because as far as I understand, the type attribute isn't inherited, so, for example, if I have something like int r = X509_verify_cert(...) and the function X509_verify_cert has a return type with attribute, r won't have the attribute. If that is correct, we still need to backtrace the value to the function declaration. Is there something I am missing?

include/clang/Basic/Attr.td
1138

Yeah, makes sense.

1140

I didn't know that, thanks.

include/clang/Basic/AttrDocs.td
2055

OK, will do.

2058

OK, I did that. However, 80 col limit in this case feels a bit inconsistent with the rest of the file to me because most of other similar descriptions don't follow it.

include/clang/Basic/DiagnosticGroups.td
57 ↗(On Diff #71157)

OK.

include/clang/Basic/DiagnosticSemaKinds.td
2259

Yeah, that sounds better.

2883

Makes sense.

lib/Sema/SemaChecking.cpp
8262

Done.

8263–8264

Done.

8269

Thanks, didn't notice that.

lib/Sema/SemaDeclAttr.cpp
1316

Ok, I ran clang-format.
Good spot, it seems that I don't need that SourceRange.

test/ReturnNonBoolTest.c
74 ↗(On Diff #71157)

I have replaced this test case with another one that illustrates the problem I am referring to clearer. Ideally it would be great to have some indicator to tell the StaticAnalyzer that we have handled all the dangerous return values, and from this point it is safe to use it as a boolean. You can use explicit cast to bool or rc != 0 every time you want to use it, but it is not very convenient. Do you have any suggestions on this matter?

As for your proposal, it is not very difficult to add, however, it is not very likely to be useful in real codebases for the same reason as in the testcase. Do you still think it should be added?

test/ReturnNonBoolTestCompileTime.cpp
40 ↗(On Diff #71157)

Done.

Thank you for the feedback.

The patch is missing Sema tests for the attribute (that it only applies to declarations you expect, accepts no args, etc).

There is one test case for that in test/ReturnNonBoolTestCompileTime.cpp. I've added another one for attribute accepting no args, so now the last two test cases in this file are those you were asking about. Can you think of any other cases of invalid attribute usage?

We try to keep our tests segregated by functionality. e.g., tests relating to the way the attribute is handled (what it appertains to, args, etc) should live in Sema, tests relating to the static analyzer behavior should live in test/Analysis, etc.

Tests that are still missing are: applying to a non-function type, applying to a member function, applying to an Obj-C method. For member functions, what should happen if the function is virtual? What if the overriders do not specify the attribute? What if an override specifies the attribute but the base does not?

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration?

No, I hadn't. On a quick look though, I couldn't find a way to simplify my solution using this idea, because as far as I understand, the type attribute isn't inherited, so, for example, if I have something like int r = X509_verify_cert(...) and the function X509_verify_cert has a return type with attribute, r won't have the attribute. If that is correct, we still need to backtrace the value to the function declaration. Is there something I am missing?

I was thinking it would be diagnosed if you attempted to assign from your attributed type to a type that is not compatible. However, that may still be problematic because it raises other questions (can you SFINAE on it? Overload? etc).

urusant updated this revision to Diff 71921.Sep 20 2016, 6:22 AM

We try to keep our tests segregated by functionality. e.g., tests relating to the way the attribute is handled (what it appertains to, args, etc) should live in Sema, tests relating to the static analyzer behavior should live in test/Analysis, etc.

Tests that are still missing are: applying to a non-function type, applying to a member function, applying to an Obj-C method. For member functions, what should happen if the function is virtual? What if the overriders do not specify the attribute? What if an override specifies the attribute but the base does not?

I have added the test cases about member functions.
As for ObjC methods, I didn't pay much attention to them while developing the check as ObjC wasn't the primary target. I tried to make a test case for it, and it turned out that it is OK to put an attribute on ObjC method, but you wouldn't get neither compiler warning nor StaticAnalyzer report. That is why I removed ObjC methods from the attribute subjects and replaced the ObjC test case with another one that shows that you cannot apply the attribute to ObjC methods (not sure if it is still necessary, because it seems not very different from applying the attribute to a non-function variable - in both cases we get the same warning). Do you think it's worth digging into how to make it work with ObjC? In this case I might need some help because I don't really speak Objective C.

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration?

No, I hadn't. On a quick look though, I couldn't find a way to simplify my solution using this idea, because as far as I understand, the type attribute isn't inherited, so, for example, if I have something like int r = X509_verify_cert(...) and the function X509_verify_cert has a return type with attribute, r won't have the attribute. If that is correct, we still need to backtrace the value to the function declaration. Is there something I am missing?

I was thinking it would be diagnosed if you attempted to assign from your attributed type to a type that is not compatible. However, that may still be problematic because it raises other questions (can you SFINAE on it? Overload? etc).

This might also make the check itself easier (as we don't need path-sensitive analysis), however, it would make the use more complicated as all the users of the dangerous function would have to change their code (even if they are using it correctly). For example, if we refer to the original motivation, annotating dangerous OpenSSL functions would allow us to protect dozens of codebases using them without changing every one of them.

danielmarjamaki added inline comments.
include/clang/Basic/AttrDocs.td
2055

I saw your email on cfe-dev. This sounds like a good idea to me.

lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp
50

It seems you need to run clang-format on this file also.

test/ReturnNonBoolTest.c
7 ↗(On Diff #71807)

sorry but why do you have a #ifdef clang isn't it always defined?

urusant updated this revision to Diff 71927.Sep 20 2016, 7:13 AM
urusant added inline comments.
lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp
51

I have just noticed that I didn't specify the style option when I ran it the first time. Now it should be fine.

test/ReturnNonBoolTest.c
7 ↗(On Diff #71807)

If I were to add the attribute to a function in some real codebase, I would probably want to save different compilers compatibility. However, it might not be necessary for the testcases.

zaks.anna added inline comments.Sep 20 2016, 11:14 AM
test/Analysis/ReturnNonBoolTest.c
67

How about addressing this as follows: in checkBranchCondition, you check for any comparisons of the tracked value other than comparisons to bool. If you see such a comparison, you assume that the error handling has occurred and remove the symbol from the set of tracked symbols. This will ensure that any code after the cleansing condition (error handling) can cast the return value to bool.

The warning will still get triggered if the error handling is after the comparison to bool. That could be avoided as well, but the solution would be more complicated. I am thinking something along the lines of tracking all comparisons until the symbol goes out of scope. For each symbol, you'd track it's state (for example, "performedErrorHandling | comparedToBoolAndNoErrorHandling | notSeen"). You can draw the automaton to see what the transitions should be. When the symbol goes out of scope, you'd check if it's state is "comparedToBoolAndNoErrorHandling". Further, we'd need to walk up the path to find the location where we compared the symbol and use that for error reporting.