[PATCH] Warning on unevaluated expression contexts with side effects

Richard Smith richard at metafoo.co.uk
Wed Dec 3 11:40:36 PST 2014 

-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.


> >> Have you run this warning across any codebases of significant size and
> found
> >> bugs or false positives? For instance, does it issue any warnings in a
> >> bootstrap build of Clang?
> >
> > I will test to see (it may take me a while, bootstrapping on Windows
> > does not happen easily). Alternatively, this could go under a flag
> > aside from -Wunused-value, in case people want to handle this
> > separately, if that would assuage fears?
>
> I applied my patch and bootstrapped Clang, and it did not yield any
> additional diagnostics (false positives or bugs).
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141203/7c870b1d/attachment.html>

More information about the cfe-commits mailing list