[analyzer] Should be able to compare pointers of UnknownSpaceRegion and StackSpaceRegion

[mzyKi]

I ran clang-analyzer-unix.Stream on the following testcase

#include <stdio.h>
char *get_str(char *Input);

void check_f_leak() {
  FILE *fp = fopen("test", "rb");
  if (NULL == fp) {
    return;
  }
  char str[64];
  if (get_str(str) != str) {
    fclose(fp);
  }
}

It show no warning and if I change the testcase like this:

#include <stdio.h>
char *get_str(char *Input);

void check_f_leak_2() {
  FILE *fp = fopen("test", "rb");
  if (NULL == fp) {
    return;
  }
  char str[64];
  if (get_str(str) != NULL) {
    fclose(fp);
  }
}

It will show:

/Workspace/test.c:24:1: warning: Opened stream never closed. Potential resource leak [clang-analyzer-unix.Stream]
   24 | }
      | ^
/Workspace/test.c:16:14: note: Stream opened here
   16 |   FILE *fp = fopen("test", "rb");
      |              ^~~~~~~~~~~~~~~~~~~
/Workspace/test.c:17:15: note: 'fp' is not equal to NULL
   17 |   if (NULL == fp) {
      |               ^~
/Workspace/test.c:17:3: note: Taking false branch
   17 |   if (NULL == fp) {
      |   ^
/Workspace/test.c:21:7: note: Assuming the condition is false
   21 |   if (get_str(str) != NULL) {
      |       ^~~~~~~~~~~~~~~~~~~~
/Workspace/test.c:21:3: note: Taking false branch
   21 |   if (get_str(str) != NULL) {
      |   ^
/Workspace/test.c:24:1: note: Opened stream never closed. Potential resource leak
   24 | }
      | ^

In my expectation,the first case should be the same as the second case and branch in the if statement.If we don't know the definition of get_str,we should not know the result of get_str(str) != NULL and it shall be an unknownval.
After,I used lldb to track the cause of this phenomenon, I found the result value of comparison is bound in SimpleSValBuilder.cpp:958

    if (LeftMS != RightMS &&
        ((LeftMS != UnknownMS && RightMS != UnknownMS) ||
         (isa<StackSpaceRegion>(LeftMS) || isa<StackSpaceRegion>(RightMS)))) {
      switch (op) {
      default:
        return UnknownVal();
      case BO_EQ:
        return makeTruthVal(false, resultTy);
      case BO_NE:
        return makeTruthVal(true, resultTy);
      }
    }

I think LeftMS != UnknownMS && RightMS != UnknownMS seems to have some logic error.About the expressionget_str(str) != str, I dump LeftMS is UnknownSpaceRegion and RightMS is StackLocalSpaceRegion. But LeftMS != UnknownMS is false.I am not sure whether this s as expected.

In my later PR, I fix false negative in first case. But I still have question about LeftMS != UnknownMS && RightMS != UnknownMS, I‘d really appreciate any suggestion from anyone familiar with this.

[llvmbot]

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

Author: Exile (mzyKi)

I ran clang-analyzer-unix.Stream on the following testcase

#include <stdio.h>
char *get_str(char *Input);

void check_f_leak() {
  FILE *fp = fopen("test", "rb");
  if (NULL == fp) {
    return;
  }
  char str[64];
  if (get_str(str) != str) {
    fclose(fp);
  }
}

It show no warning and if I change the testcase like this:

#include <stdio.h>
char *get_str(char *Input);

void check_f_leak_2() {
  FILE *fp = fopen("test", "rb");
  if (NULL == fp) {
    return;
  }
  char str[64];
  if (get_str(str) != NULL) {
    fclose(fp);
  }
}

It will show:

/Workspace/test.c:24:1: warning: Opened stream never closed. Potential resource leak [clang-analyzer-unix.Stream]
   24 | }
      | ^
/Workspace/test.c:16:14: note: Stream opened here
   16 |   FILE *fp = fopen("test", "rb");
      |              ^~~~~~~~~~~~~~~~~~~
/Workspace/test.c:17:15: note: 'fp' is not equal to NULL
   17 |   if (NULL == fp) {
      |               ^~
/Workspace/test.c:17:3: note: Taking false branch
   17 |   if (NULL == fp) {
      |   ^
/Workspace/test.c:21:7: note: Assuming the condition is false
   21 |   if (get_str(str) != NULL) {
      |       ^~~~~~~~~~~~~~~~~~~~
/Workspace/test.c:21:3: note: Taking false branch
   21 |   if (get_str(str) != NULL) {
      |   ^
/Workspace/test.c:24:1: note: Opened stream never closed. Potential resource leak
   24 | }
      | ^

In my expectation,the first case should be the same as the second case and branch in the if statement.If we don't know the definition of get_str,we should not know the result of get_str(str) != NULL and it shall be an unknownval.
After,I used lldb to track the cause of this phenomenon, I found the result value of comparison is bound in SimpleSValBuilder.cpp:958

    if (LeftMS != RightMS &&
        ((LeftMS != UnknownMS && RightMS != UnknownMS) ||
         (isa<StackSpaceRegion>(LeftMS) || isa<StackSpaceRegion>(RightMS)))) {
      switch (op) {
      default:
        return UnknownVal();
      case BO_EQ:
        return makeTruthVal(false, resultTy);
      case BO_NE:
        return makeTruthVal(true, resultTy);
      }
    }

I think LeftMS != UnknownMS && RightMS != UnknownMS seems to have some logic error.About the expressionget_str(str) != str, I dump LeftMS is UnknownSpaceRegion and RightMS is StackLocalSpaceRegion. But LeftMS != UnknownMS is false.I am not sure whether this s as expected.

In my later PR, I fix false negative in first case. But I still have question about LeftMS != UnknownMS && RightMS != UnknownMS, I‘d really appreciate any suggestion from anyone familiar with this.

[steakhal]

This is actually the same issue as described in #115410.
This issue is more to the point though, as the other one required some investigation to get to this conclusion.

[steakhal]

I think LeftMS != UnknownMS && RightMS != UnknownMS seems to have some logic error.About the expressionget_str(str) != str, I dump LeftMS is UnknownSpaceRegion and RightMS is StackLocalSpaceRegion. But LeftMS != UnknownMS is false.I am not sure whether this s as expected.

In my later PR, I fix false negative in first case. But I still have question about LeftMS != UnknownMS && RightMS != UnknownMS, I‘d really appreciate any suggestion from anyone familiar with this.

Read this: #115410 (comment)

You may wanna collaborate with @Flandini who showed interest in solving the issue.