undefbehavior.ExitInDtor

Hi.

The List of potential checkers[1] posted recently contains this item,
which looked like a low hanging fruit, ideal for getting my feet wet
with matchers:

   Undefined behavior: std::exit() is called to end the program during
   the destruction of an object with static storage duration.

   Source: C++11 3.6.1p4.

#include <cstdlib>

class A {
public:
  ~A() {
    std::exit(1); // warn
  }
};

I took this as an exercise, and was very surprised to see how easy this
was to do, and how little time it took to get done, given that my
experience with the AST prior to yesterday was virtually zero.

I have two questions:

1. What would be an approriate name / category for this check?
2. Is it worth submitting?

DestructorExitCheck.patch (5.12 KB)

In article <8761bjucor.fsf@fx.delysid.org>,
    Mario Lang <mlang@delysid.org> writes:

The List of potential checkers[1] posted recently contains this item,
which looked like a low hanging fruit, ideal for getting my feet wet
with matchers:

   Undefined behavior: std::exit() is called to end the program during
   the destruction of an object with static storage duration.

   Source: C++11 3.6.1p4.

#include <cstdlib>

class A {
public:
  ~A() {
    std::exit(1); // warn
  }
};

Don't forget that you also need:

static A a;

I took this as an exercise, and was very surprised to see how easy this
was to do, and how little time it took to get done, given that my
experience with the AST prior to yesterday was virtually zero.

Yeah, it's fun playing with matchers and the AST!

I have two questions:

1. What would be an approriate name / category for this check?
2. Is it worth submitting?

The answer to your second question is yes.

I'm unsure if this should be in clang-tidy or the static analyzer. I
am still unclear which checks should go where. This one feels
slightly more like the static analyzer should get it instead of
clang-tidy.

clang-tidy web page says:

  "clang-tidy is a clang-based C++ linter tool. Its purpose is to
  provide an extensible framework for diagnosing and fixing typical
  programming errors, like style violations, interface misuse, or
  bugs that can be deduced via static analysis."

First, we don't have an automatic "fix" we can apply to correct the
problem.

Second, we are detecting undefined behavior, which isn't in the spirit
of a style violation or interface misuse.

Third, my gut feeling is that clang's static analyzer has more regular
usage than clang-tidy and we'd like checks for undefined behavior to
find their way into as many hands as possible.

Finally, this is just my opinion so try and get some guidance from
someone that is a regular contributor to the static analyzer for
better advice :-).

In article <8761bjucor.fsf@fx.delysid.org>,
    Mario Lang <mlang@delysid.org> writes:

> The List of potential checkers[1] posted recently contains this item,
> which looked like a low hanging fruit, ideal for getting my feet wet
> with matchers:
>
> Undefined behavior: std::exit() is called to end the program during
> the destruction of an object with static storage duration.
>
> Source: C++11 3.6.1p4.
>
> #include <cstdlib>
>
> class A {
> public:
> ~A() {
> std::exit(1); // warn
> }
> };

Don't forget that you also need:

static A a;

> I took this as an exercise, and was very surprised to see how easy this
> was to do, and how little time it took to get done, given that my
> experience with the AST prior to yesterday was virtually zero.

Yeah, it's fun playing with matchers and the AST!

> I have two questions:
>
> 1. What would be an approriate name / category for this check?
> 2. Is it worth submitting?

The answer to your second question is yes.

I'm unsure if this should be in clang-tidy or the static analyzer.

Proper implementation of this check needs some inter-function flow analysis
(consider exit() called indirectly from a destructor of a global object)
which may be non-trivial (if even possible) to implement statically. If you
want to limit the scope of the check to direct calls to exit() from a
global destructor, it should be easy to do in both clang-tidy and static
analyzer. I suspect, that an ast matcher-based implementation could be
easier to write, so it may be somewhat more convenient to add a check to
clang-tidy.

Maybe adding a dynamic check to ubsan (-fsanitize=undefined) also makes
sense. Richard, WDYT?

  I
am still unclear which checks should go where. This one feels
slightly more like the static analyzer should get it instead of
clang-tidy.

clang-tidy web page says:

        "clang-tidy is a clang-based C++ linter tool. Its purpose is to
        provide an extensible framework for diagnosing and fixing typical
        programming errors, like style violations, interface misuse, or
        bugs that can be deduced via static analysis."

First, we don't have an automatic "fix" we can apply to correct the
problem.

Second, we are detecting undefined behavior, which isn't in the spirit
of a style violation or interface misuse.

Third, my gut feeling is that clang's static analyzer has more regular
usage than clang-tidy and we'd like checks for undefined behavior to
find their way into as many hands as possible.

Finally, this is just my opinion so try and get some guidance from
someone that is a regular contributor to the static analyzer for
better advice :-).

I have a feeling that static analyzer is more suitable for path-sensitive
checks. Everything that is expressible in terms of AST is probably easier
to implement in clang-tidy. Also, static analyzer doesn't have a way to
provide automated fixes, so all checks that could provide fixes are usually
more suitable for clang-tidy.