mc-imperial / dredd

A mutation testing tool designed to work with large C++ (and C) codebases.
Apache License 2.0
13 stars 3 forks source link

Introduced lambda could use/return reference to local temporary object #311

Closed JonathanFoo0523 closed 1 month ago

JonathanFoo0523 commented 2 months ago

Applying dredd to

#include <vector>
#include <cassert>

std::vector<unsigned int> bar() {
    std::vector<unsigned int> res(4);
    return res;
}

int main() {
    unsigned foo = bar()[0];
    assert(foo == 0);
}

Possible redundant mutants aside(#310), dredd introduced lambda of form:

unsigned foo = [&]() -> unsigned int& { return static_cast<unsigned int&>(bar()[0]); } ();

Compiling gives the warning

returning reference to local temporary object [-Wreturn-stack-address]

and executing the compiled binary lead to assertion fail.

JonathanFoo0523 commented 2 months ago

The AST for the declaration is

-DeclStmt 0x55d489bcf398 <line:10:5, col:28>
    | `-VarDecl 0x55d489bcf0c0 <col:5, col:27> col:14 used foo 'unsigned int' cinit
    |   `-ExprWithCleanups 0x55d489bcf380 <col:20, col:27> 'value_type':'unsigned int'
    |     `-ImplicitCastExpr 0x55d489bcf368 <col:20, col:27> 'value_type':'unsigned int' <LValueToRValue>
    |       `-CXXOperatorCallExpr 0x55d489bcf330 <col:20, col:27> 'value_type':'unsigned int' lvalue '[]'
    |         |-ImplicitCastExpr 0x55d489bcf318 <col:25, col:27> 'reference (*)(size_type) noexcept' <FunctionToPointerDecay>
    |         | `-DeclRefExpr 0x55d489bcf298 <col:25, col:27> 'reference (size_type) noexcept' lvalue CXXMethod 0x55d489bc03a0 'operator[]' 'reference (size_type) noexcept'
    |         |-MaterializeTemporaryExpr 0x55d489bcf268 <col:20, col:24> 'std::vector<unsigned int>':'std::vector<unsigned int>' lvalue
    |         | `-CXXBindTemporaryExpr 0x55d489bcf228 <col:20, col:24> 'std::vector<unsigned int>':'std::vector<unsigned int>' (CXXTemporary 0x55d489bcf228)
    |         |   `-CallExpr 0x55d489bcf200 <col:20, col:24> 'std::vector<unsigned int>':'std::vector<unsigned int>'
    |         |     `-ImplicitCastExpr 0x55d489bcf1e8 <col:20> 'std::vector<unsigned int> (*)()' <FunctionToPointerDecay>
    |         |       `-DeclRefExpr 0x55d489bcf170 <col:20> 'std::vector<unsigned int> ()' lvalue Function 0x55d489b9f648 'bar' 'std::vector<unsigned int> ()'
    |         `-ImplicitCastExpr 0x55d489bcf280 <col:26> 'size_type':'unsigned long' <IntegralCast>
    |           `-IntegerLiteral 0x55d489bcf248 <col:26> 'int' 0
afd commented 1 month ago

This smaller example yields the warning:

#include <vector>

std::vector<unsigned int> bar();

int main() {
    return bar()[0];
}
afd commented 1 month ago

Very small example that gives the warning:

namespace std {
struct vector {
  unsigned int& operator[](int);
};
}

int main() {
  return std::vector()[0];
}

Here is relevant part of the AST:

`-FunctionDecl 0x58722852c3e0 <line:7:1, line:9:1> line:7:5 main 'int ()'
  `-CompoundStmt 0x58722854b230 <col:12, line:9:1>
    `-ReturnStmt 0x58722852ce40 <line:8:3, col:25>
      `-ExprWithCleanups 0x58722852ce28 <col:10, col:25> 'int'
        `-ImplicitCastExpr 0x58722852ce10 <col:10, col:25> 'int' <IntegralCast>
          `-ImplicitCastExpr 0x58722852cdf8 <col:10, col:25> 'unsigned int' <LValueToRValue>
            `-CXXOperatorCallExpr 0x58722852cdc0 <col:10, col:25> 'unsigned int' lvalue '[]'
              |-ImplicitCastExpr 0x58722852cda8 <col:23, col:25> 'unsigned int &(*)(int)' <FunctionToPointerDecay>
              | `-DeclRefExpr 0x58722852cd58 <col:23, col:25> 'unsigned int &(int)' lvalue CXXMethod 0x58722852c2c0 'operator[]' 'unsigned int &(int)'
              |-MaterializeTemporaryExpr 0x58722852cd40 <col:10, col:22> 'std::vector':'std::vector' lvalue
              | `-CXXTemporaryObjectExpr 0x58722852cc08 <col:10, col:22> 'std::vector':'std::vector' 'void () noexcept' zeroing
              `-IntegerLiteral 0x58722852cd20 <col:24> 'int' 0

This is the mutated code (having gotten rid of a statement removal and printed node types):

int main() {
  return __dredd_replace_expr_int/*ExprWithCleanups*/([&]() -> int {
    return static_cast<int>(__dredd_replace_expr_int/*ImplicitCastExpr*/([&]() -> int {
      return static_cast<int>(__dredd_replace_expr_unsigned_int_lvalue/*CXXOperatorCallExpr*/([&]() -> unsigned int& {
    return static_cast<unsigned int&>(std::vector()[__dredd_replace_expr_int_zero/*IntegerLiteral*/(0, 0)]);
      }, 2));
    }, 4));
  }, 10);
}

The problem is with mutation of the CXXOperatorCallExpr, and the issue seems to be that (a) it yields and l-value, and (b) it receives a materialized temporary expression argument. There is thus scope for that materialized temporary to end up being returned by reference, which is what is leading to the warning and causing the associated bug.

A conservative solution would seem to be to identify calls that return an l-value, and if any of their arguments are materialised temporaries, do not mutate the call node.

afd commented 1 month ago

This is an example of a program that is easier to understand and which is affected by the issue. The mutation changes the point at which the destructor gets called.

#include <iostream>

unsigned int glob = 42;

struct S {

  ~S() {
    glob = 12;
  }

  unsigned int& operator[](int) {
    return glob;
  }
};

int main() {
  std::cout << S()[0] << std::endl;
}