[PATCH] Warning on unevaluated expression contexts with side effects

[Aaron Ballman]

Ping

On Thu, Dec 4, 2014 at 10:35 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Wed, Dec 3, 2014 at 2:40 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> -bool Expr::HasSideEffects(const ASTContext &Ctx) const {
>> +bool Expr::HasSideEffects(const ASTContext &Ctx,
>> +                          bool OnlyDefiniteEffects) const {
>>
>> I would reverse the sense of this flag, so that a 'true' input and a 'true'
>> output mean the same thing.
>>
>> +  bool DefiniteEffectsResult = !OnlyDefiniteEffects;
>>
>> This seems like a bad name, since it's the result you return when you're not
>> sure whether there's a side-effect or not. UnknownResult or similar would be
>> better.
>>
>> +    // These depend on whether we are determining side effects for the
>> purposes
>> +    // of unevaluated expression operands, like sizeof(). For instance, a
>> +    // function call expression is assumed to be acceptable because the
>> user is
>> +    // interested in the results of the call, not expecting side effects
>> from
>> +    // the call, as in: sizeof(f());
>>
>> This comment is too specific to the user of this function rather than
>> talking about this function's actual purpose. Just "// We don't know whether
>> a call has side effects." would be fine.
>>
>> +    return DefiniteEffectsResult;
>>
>> We shouldn't return at this point if OnlyDefiniteEffects is true; we should
>> break out of the switch so that we go on to analyze the operands.
>>
>> +  case UnresolvedLookupExprClass:
>> +    // This should *only* be reachable when determining side effects for
>> the
>> +    // purposes of unevaluated expression operands for decltype(). This can
>> +    // happen in situations where the decltype expression looks up an
>> overloaded
>> +    // function, or when the function is an uninstantiated template.
>> +    assert(OnlyDefiniteEffects);
>> +    return false;
>>
>> Just return UnknownResult here? We don't know whether this expression would
>> have side-effects. If this function is supposed to now be usable on
>> unresolved expressions, we should make it handle them in all cases, not just
>> most cases.
>>
>> +  // so side effects are likely result in unintended consequences.
>>
>> Grammar error here; missing word? Also, "are likely" seems to be overstating
>> the case.
>>
>> +  else if (!PotentiallyEvaluated && E->HasSideEffects(Context, true)) {
>>
>> I think we should still warn in the potentially-evaluated case; that's
>> probably more likely to be a bug, because people probably expect the operand
>> of typeid to be unevaluated at least as often as they expect it to be
>> evaluated.
>>
>> On Tue, Dec 2, 2014 at 6:46 AM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> On Mon, Dec 1, 2014 at 4:39 PM, Aaron Ballman <aaron at aaronballman.com>
>>> wrote:
>>> > On Mon, Dec 1, 2014 at 2:04 PM, Richard Smith <richard at metafoo.co.uk>
>>> > wrote:
>>> >> On Mon, Dec 1, 2014 at 10:33 AM, Aaron Ballman <aaron at aaronballman.com>
>>> >> wrote:
>>> >>>
>>> >>> On Wed, Nov 26, 2014 at 9:09 PM, Richard Smith <richard at metafoo.co.uk>
>>> >>> wrote:
>>> >>> > On Tue, Nov 25, 2014 at 6:51 AM, Aaron Ballman
>>> >>> > <aaron at aaronballman.com>
>>> >>> > wrote:
>>> >>> >>
>>> >>> >> The attached patch adds a -Wunused-value warning when an expression
>>> >>> >> with side effects is used in an unevaluated expression context,
>>> >>> >> such
>>> >>> >> as sizeof(), or decltype(). It mostly reuses the logic from
>>> >>> >> Expr::HasSideEffects, except with a flag that treats certain
>>> >>> >> expressions as not having side effects -- mostly
>>> >>> >> instantiation-dependent contexts, and function call-like
>>> >>> >> operations.
>>> >>> >>
>>> >>> >> int f();
>>> >>> >> int j = 0;
>>> >>> >>
>>> >>> >> (void)sizeof(f()); // Ok
>>> >>> >> (void)sizeof(++j); // Warn
>>> >>> >> (void)sizeof(int[++j]); // Ok
>>> >>> >>
>>> >>> >> I've added support for: sizeof, typeid, _Generic, _Alignof,
>>> >>> >> noexcept,
>>> >>> >> and decltype.
>>> >>> >
>>> >>> >
>>> >>> > It is a very common idiom to use arbitrary expressions in sizeof,
>>> >>> > decltype,
>>> >>> > or noexcept expressions; in the former two cases, for SFINAE, and in
>>> >>> > the
>>> >>> > last case to compute an exception specification for an arbitrary
>>> >>> > function.
>>> >>> > In all these cases, expressions with side-effects seem completely
>>> >>> > reasonable. How does your patch behave in those cases?
>>> >>>
>>> >>> I believe it behaves sensibly, but am happy to get counter-examples
>>> >>> where I can tweak something.
>>> >>
>>> >>
>>> >> You could try building boost and eigen with -Werror=<this warning>. If
>>> >> they're both fine with it, then I'm happy to assume it's OK =) But see
>>> >> below.
>>> >
>>> > Will those build with a clang built with MSVC on Windows? I didn't
>>> > think we were there quite yet (and it's all I have access to ATM).
>>> >
>>> >>
>>> >>>
>>> >>> Basically, if the side-effect involves
>>> >>> something function-like (including overloaded operators), we ignore it
>>> >>> as being side-effecting. I've found that this cuts the signal-to-noise
>>> >>> ratio down considerably. I think this also matches the intent of
>>> >>> unevaluated contexts -- when the compiler needs a declaration, but not
>>> >>> a definition, it's safe to ignore side-effects. Eg)
>>> >>>
>>> >>> struct S {
>>> >>>   S operator++(int);
>>> >>>   S(int i);
>>> >>>   S();
>>> >>>
>>> >>>   int& f();
>>> >>>   S g();
>>> >>> };
>>> >>>
>>> >>> void f() {
>>> >>>   S s;
>>> >>>   int i = 0;
>>> >>>   (void)noexcept(s++); // Ok, because of operator++()
>>> >>>   (void)noexcept(i++); // Not Ok, because this doesn't involve a
>>> >>> function
>>> >>> call
>>> >>>   (void)noexcept(i = 5); // Not Ok, because this doesn't involve a
>>> >>> function call
>>> >>>   (void)noexcept(s = 5); // Ok, because of copy constructor call
>>> >>>
>>> >>>   (void)sizeof(s.f()); // Ok, because of f()
>>> >>>   (void)sizeof(s.f() = 5); // Not Ok, because f() returns an int&, and
>>> >>> this assignment could reasonably be side-effecting
>>> >>>   (void)noexcept(s.g() = 5); // Ok, because of copy constructor call
>>> >>> }
>>> >>
>>> >>
>>> >> Here's a SFINAE idiom for detecting incrementability:
>>> >>
>>> >>   template<typename T> static one
>>> >> &is_incrementable_helper(decltype(declval<T>()++) *p);
>>> >>   template<typename T> static two &is_incrementable_helper(...);
>>> >>
>>> >>   template<typename T> using is_incrementable =
>>> >>     std::integer_constant<bool,
>>> >> sizeof(is_incrementable_helper<T>(nullptr))
>>> >> == sizeof(one)>;
>>> >>
>>> >> I think your warning would warn on uses of
>>> >> 'is_incrementable<int>::value'?
>>> >
>>> > It compiles without warning, actually. The use of T within decltype is
>>> > instantiation-dependent, which my patch treats as only being possibly
>>> > side-effecting, instead of definitely.
>>> >
>>> >> Here's some code in boost that uses a different approach, but that will
>>> >> probably also elicit a warning when applied to a type that uses the
>>> >> builtin
>>> >> operator++:
>>> >> http://www.boost.org/doc/libs/1_55_0/boost/detail/is_incrementable.hpp
>>> >
>>> > This also compiles without warning.
>>> >
>>> >>
>>> >> One way to avoid these cases would be to always suppress this warning
>>> >> if it
>>> >> occurs while instantiating a template.
>>> >
>>> > if (isInstantiationDependent())
>>> >   return DefiniteEffectsResult;
>>
>>
>> That will affect the behavior when parsing the template, not when
>> instantiating it. During / after template instantiation, your expression
>> will not be instantiation dependent.
>>
>> It looks like you're not seeing issues for decltype because you put the
>> check in ActOnDecltypeExpression rather than BuildDecltypeType (so it's only
>> called from the parser and not from template instantiation). I would expect
>> you'll get warnings during template instantiation for all the other forms of
>> unevaluated operand.
>>
>>> > ;-)
>>> >
>>> >>
>>> >>> >
>>> >>> > Does this actually catch bugs in practice?
>>> >>>
>>> >>> I believe so. There are two CERT advisories against it, which is why I
>>> >>> am implementing the functionality.
>>> >>
>>> >>
>>> >> Does that actually imply that this is a real-world problem? (The CERT
>>> >> advisory (for C++) says "Severity: Low, Likelihood: Unlikely". Are CERT
>>> >> advisories always based on actual events, or can they be just
>>> >> theoretical?)
>>> >
>>> > They can definitely be theoretical, but many are based on actual
>>> > events. We try to ensure the rules are based on plausible, real world
>>> > scenarios that could result in a security vulnerability, and this
>>> > definitely met our criteria (twice). A quick browse of CVE did not
>>> > bring up major known vulns from this.
>>
>>
>> If the true positive rate is effectively zero, it is wasteful to spend
>> compile time checking for this (and to incur the maintenance cost of the
>> extra code). We should run this over some larger codebases before going
>> ahead with it.
>
> Attached is an updated patch based on this feedback, and further
> feedback from IRC. Of special note, this patch now handled typeids
> which are evaluated as a separate warning case (since it's equally
> likely that the user would expect the expression operand of typeid to
> be unevaluated when it actually turns out to be evaluated).
>
> I reran my bootstrapping tests against Clang, and got no false
> positives or true positives.
>
> Regarding the true positive rate -- I would expect that to be
> relatively low for established code bases. People generally find this
> sort of bug because they expect the side effect to happen but it
> doesn't. However, I think it is a valid warning to justify the cost;
> we're not the only vendor who implements such a diagnostic (ECLAIR,
> LDRA, PC-Lint, and PRQA all implement it as well from a quick check).
> I could see a case for moving this to the static analyzer, but it's
> also not path-sensitive, and really doesn't gain us much in terms of
> code maintenance.
>
> If you, or rtrieu, have some other large code bases to test this
> against, I would greatly appreciate it.
>
> Thanks!
>
> ~Aaron