[analyzer] False Positives due to conflicting assumptions in callee and caller about returned value's field

[marco-antognini-sonarsource]

#115917 introduced a regression with the following code:

typedef struct { int error; } Status;
Status getError();
int global;

Status get(int **outptr) {
  Status e = getError();
  if (e.error != 0) return e; // field error is assumed to be non 0
  *outptr = &global; // therefore this is not executed
  return e;
}

int func() {
  int *ptr = 0;
  if (get(&ptr).error == 0) // field error is assumed to be 0
    return *ptr; // therefore this is a null deref
  return 0;
}

https://godbolt.org/z/seaaeMrTd

Another example with another Checker:

typedef struct {
  int v;
} STATUS;

STATUS STATUS_OK = {0};

void use(const char* x);

STATUS get_two(const char** ret);

static STATUS get_one(const char** ret) {
  STATUS status;
  const char* s;

  status = get_two(&s);
  if (status.v != 0) {
    return status;
  }

  *ret = s;

  return STATUS_OK;
}

int main() {
  STATUS status;
  const char* s;

  status = get_one(&s);
  if (status.v == 0) {
    use(s);  // FP
  }
}

https://godbolt.org/z/8x5Ghh95b

FYI The godbolt links show the difference between trunk and v19. I've done the bisection locally down to 4610e5c.

This issue impact many projects that have struct to represent error codes. I've often seen them wrapped with macros, such as in the Samba project. https://github.com/samba-team/samba/blob/7b5cc7d37f11347a606a4a9fe7f44ae43193318d/libcli/util/werror.h#L66

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Marco Borgeaud (marco-antognini-sonarsource)

https://github.com//pull/115917 introduced a regression with the following code:

typedef struct { int error; } Status;
Status getError();
int global;

Status get(int **outptr) {
  Status e = getError();
  if (e.error != 0) return e; // field error is assumed to be non 0
  *outptr = &global; // therefore this is not executed
  return e;
}

int func() {
  int *ptr = 0;
  if (get(&ptr).error == 0) // field error is assumed to be 0
    return *ptr; // therefore this is a null deref
  return 0;
}

https://godbolt.org/z/seaaeMrTd

Another example with another Checker:

typedef struct {
  int v;
} STATUS;

STATUS STATUS_OK = {0};

void use(const char* x);

STATUS get_two(const char** ret);

static STATUS get_one(const char** ret) {
  STATUS status;
  const char* s;

  status = get_two(&s);
  if (status.v != 0) {
    return status;
  }

  *ret = s;

  return STATUS_OK;
}

int main() {
  STATUS status;
  const char* s;

  status = get_one(&s);
  if (status.v == 0) {
    use(s);  // FP
  }
}

https://godbolt.org/z/8x5Ghh95b

FYI The godbolt links show the difference between trunk and v19. I've done the bisection locally down to 4610e5c.

This issue impact many projects that have struct to represent error codes. I've often seen them wrapped with macros, such as in the Samba project. https://github.com/samba-team/samba/blob/7b5cc7d37f11347a606a4a9fe7f44ae43193318d/libcli/util/werror.h#L66

[steakhal]

Shall we just revert #115917?

[marco-antognini-sonarsource]

Shall we just revert #115917?

It may be the best course of action. I've found the result of the analysis also differs on this code example. In other words, #115917 not only causes FPs, but it causes FNs too.

struct StorageWrapper {
  // Imagine those two call a reset() function (among other things)
  ~StorageWrapper() { delete parts; }
  void operator=(StorageWrapper&&) { delete parts; }

  // Not provided
  StorageWrapper();
  StorageWrapper(StorageWrapper&);

  long* parts;
};

// Toggle which test should run. They are mutually exclusive...
#if 1

// Behaviour changes between v19 and v20 for this one:
// v19: Attempt to free released memory (double free)
// v20: no issues.
// Note: v19 is correct.
struct Test {
  StorageWrapper* Object;
  Test() : Object(new StorageWrapper[]{StorageWrapper()}) {
    Object[0] = StorageWrapper();
  }
};

#else

// Behaviour changes between v19 and v20 for this one:
// v19: Attempt to free released memory (double free)
// v20: Potential leak of memory pointed to by object.
// Note: BOTH are TPs.
void test() {
  StorageWrapper* object = new StorageWrapper[]{StorageWrapper()};
  object[0] = StorageWrapper();
  // delete object; // Adding this deletes results in same behaviour in v19 and v20 (double free)
}

#endif

https://godbolt.org/z/GoMzn6M1s

[marco-antognini-sonarsource]

For posterity, there's a great summary of the problem here: #163461 (comment)