[PATCH] Warning on unevaluated expression contexts with side effects

[Aaron Ballman]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: UnevaluatedExpr_v3.patch
Type: application/octet-stream
Size: 25120 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141204/013946d0/attachment.obj>