[clang][analyzer] Improve modeling of 'popen' and 'pclose' in StdLibraryFunctionsChecker #78895

benshi001 · 2024-01-21T10:30:05Z

No description provided.

llvmbot · 2024-01-21T10:30:35Z

@llvm/pr-subscribers-clang

Author: Ben Shi (benshi001)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/78895.diff

4 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+20-15)
(modified) clang/test/Analysis/Inputs/system-header-simulator.h (+2)
(modified) clang/test/Analysis/std-c-library-functions-POSIX.c (+2-2)
(modified) clang/test/Analysis/stream-errno.c (+25)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index d0eb5091444f6b..4053a829bd7a7d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -2202,6 +2202,16 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
             .ArgConstraint(NotNull(ArgNo(1)))
             .ArgConstraint(NotNull(ArgNo(2))));
 
+    // FILE *popen(const char *command, const char *type);
+    addToFunctionSummaryMap(
+        "popen",
+        Signature(ArgTypes{ConstCharPtrTy, ConstCharPtrTy}, RetType{FilePtrTy}),
+        Summary(NoEvalCall)
+            .Case({NotNull(Ret)}, ErrnoMustNotBeChecked, GenericSuccessMsg)
+            .Case({IsNull(Ret)}, ErrnoNEZeroIrrelevant, GenericFailureMsg)
+            .ArgConstraint(NotNull(ArgNo(0)))
+            .ArgConstraint(NotNull(ArgNo(1))));
+
     // int fclose(FILE *stream);
     addToFunctionSummaryMap(
         "fclose", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
@@ -2211,6 +2221,16 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
                   ErrnoNEZeroIrrelevant, GenericFailureMsg)
             .ArgConstraint(NotNull(ArgNo(0))));
 
+    // int pclose(FILE *stream);
+    addToFunctionSummaryMap(
+        "pclose", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
+        Summary(NoEvalCall)
+            .Case({ReturnValueCondition(WithinRange, {{0, IntMax}})},
+                  ErrnoMustNotBeChecked, GenericSuccessMsg)
+            .Case({ReturnValueCondition(WithinRange, SingleValue(-1))},
+                  ErrnoNEZeroIrrelevant, GenericFailureMsg)
+            .ArgConstraint(NotNull(ArgNo(0))));
+
     // int ungetc(int c, FILE *stream);
     addToFunctionSummaryMap(
         "ungetc", Signature(ArgTypes{IntTy, FilePtrTy}, RetType{IntTy}),
@@ -2827,21 +2847,6 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
             .ArgConstraint(
                 ArgumentCondition(0, WithinRange, Range(0, IntMax))));
 
-    // FILE *popen(const char *command, const char *type);
-    // FIXME: Improve for errno modeling.
-    addToFunctionSummaryMap(
-        "popen",
-        Signature(ArgTypes{ConstCharPtrTy, ConstCharPtrTy}, RetType{FilePtrTy}),
-        Summary(NoEvalCall)
-            .ArgConstraint(NotNull(ArgNo(0)))
-            .ArgConstraint(NotNull(ArgNo(1))));
-
-    // int pclose(FILE *stream);
-    // FIXME: Improve for errno modeling.
-    addToFunctionSummaryMap(
-        "pclose", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
-        Summary(NoEvalCall).ArgConstraint(NotNull(ArgNo(0))));
-
     // int close(int fildes);
     addToFunctionSummaryMap(
         "close", Signature(ArgTypes{IntTy}, RetType{IntTy}),
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index f8e3e546a7aed5..c41f22fef388e1 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -47,7 +47,9 @@ FILE *fopen(const char *restrict path, const char *restrict mode);
 FILE *fdopen(int fd, const char *mode);
 FILE *tmpfile(void);
 FILE *freopen(const char *restrict pathname, const char *restrict mode, FILE *restrict stream);
+FILE *popen(const char *command, const char *mode);
 int fclose(FILE *fp);
+int pclose(FILE *stream);
 size_t fread(void *restrict, size_t, size_t, FILE *restrict);
 size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
 int fgetc(FILE *stream);
diff --git a/clang/test/Analysis/std-c-library-functions-POSIX.c b/clang/test/Analysis/std-c-library-functions-POSIX.c
index 51b136d9ba3567..03aa8e2e00a75d 100644
--- a/clang/test/Analysis/std-c-library-functions-POSIX.c
+++ b/clang/test/Analysis/std-c-library-functions-POSIX.c
@@ -20,7 +20,9 @@
 // CHECK: Loaded summary for: FILE *fdopen(int fd, const char *mode)
 // CHECK: Loaded summary for: FILE *tmpfile(void)
 // CHECK: Loaded summary for: FILE *freopen(const char *restrict pathname, const char *restrict mode, FILE *restrict stream)
+// CHECK: Loaded summary for: FILE *popen(const char *command, const char *type)
 // CHECK: Loaded summary for: int fclose(FILE *stream)
+// CHECK: Loaded summary for: int pclose(FILE *stream)
 // CHECK: Loaded summary for: int fseek(FILE *stream, long offset, int whence)
 // CHECK: Loaded summary for: int fseeko(FILE *stream, off_t offset, int whence)
 // CHECK: Loaded summary for: off_t ftello(FILE *stream)
@@ -74,8 +76,6 @@
 // CHECK: Loaded summary for: DIR *opendir(const char *name)
 // CHECK: Loaded summary for: DIR *fdopendir(int fd)
 // CHECK: Loaded summary for: int isatty(int fildes)
-// CHECK: Loaded summary for: FILE *popen(const char *command, const char *type)
-// CHECK: Loaded summary for: int pclose(FILE *stream)
 // CHECK: Loaded summary for: int close(int fildes)
 // CHECK: Loaded summary for: long fpathconf(int fildes, int name)
 // CHECK: Loaded summary for: long pathconf(const char *path, int name)
diff --git a/clang/test/Analysis/stream-errno.c b/clang/test/Analysis/stream-errno.c
index fab6a58b3275a8..44bf949673082d 100644
--- a/clang/test/Analysis/stream-errno.c
+++ b/clang/test/Analysis/stream-errno.c
@@ -51,6 +51,17 @@ void check_freopen(void) {
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
 }
 
+void check_popen(void) {
+  FILE *F = popen("xxx", "r");
+  if (!F) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+    if (errno) {}                    // no-warning
+  } else {
+    if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [unix.Errno]}}
+    pclose(F);
+  }
+}
+
 void check_fclose(void) {
   FILE *F = tmpfile();
   if (!F)
@@ -64,6 +75,20 @@ void check_fclose(void) {
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
 }
 
+void check_pclose(void) {
+  FILE *F = popen("xx", "w");
+  if (!F)
+    return;
+  int Ret = pclose(F);
+  if (Ret == -1) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+    if (errno) {}                    // no-warning
+  } else {
+    clang_analyzer_eval(Ret >= 0);   // expected-warning{{TRUE}}
+    if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
+  }
+}
+
 void check_fread_size0(void) {
   char Buf[10];
   FILE *F = tmpfile();

llvmbot · 2024-01-21T10:30:35Z

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Ben Shi (benshi001)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/78895.diff

4 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp (+20-15)
(modified) clang/test/Analysis/Inputs/system-header-simulator.h (+2)
(modified) clang/test/Analysis/std-c-library-functions-POSIX.c (+2-2)
(modified) clang/test/Analysis/stream-errno.c (+25)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index d0eb5091444f6b..4053a829bd7a7d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -2202,6 +2202,16 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
             .ArgConstraint(NotNull(ArgNo(1)))
             .ArgConstraint(NotNull(ArgNo(2))));
 
+    // FILE *popen(const char *command, const char *type);
+    addToFunctionSummaryMap(
+        "popen",
+        Signature(ArgTypes{ConstCharPtrTy, ConstCharPtrTy}, RetType{FilePtrTy}),
+        Summary(NoEvalCall)
+            .Case({NotNull(Ret)}, ErrnoMustNotBeChecked, GenericSuccessMsg)
+            .Case({IsNull(Ret)}, ErrnoNEZeroIrrelevant, GenericFailureMsg)
+            .ArgConstraint(NotNull(ArgNo(0)))
+            .ArgConstraint(NotNull(ArgNo(1))));
+
     // int fclose(FILE *stream);
     addToFunctionSummaryMap(
         "fclose", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
@@ -2211,6 +2221,16 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
                   ErrnoNEZeroIrrelevant, GenericFailureMsg)
             .ArgConstraint(NotNull(ArgNo(0))));
 
+    // int pclose(FILE *stream);
+    addToFunctionSummaryMap(
+        "pclose", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
+        Summary(NoEvalCall)
+            .Case({ReturnValueCondition(WithinRange, {{0, IntMax}})},
+                  ErrnoMustNotBeChecked, GenericSuccessMsg)
+            .Case({ReturnValueCondition(WithinRange, SingleValue(-1))},
+                  ErrnoNEZeroIrrelevant, GenericFailureMsg)
+            .ArgConstraint(NotNull(ArgNo(0))));
+
     // int ungetc(int c, FILE *stream);
     addToFunctionSummaryMap(
         "ungetc", Signature(ArgTypes{IntTy, FilePtrTy}, RetType{IntTy}),
@@ -2827,21 +2847,6 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
             .ArgConstraint(
                 ArgumentCondition(0, WithinRange, Range(0, IntMax))));
 
-    // FILE *popen(const char *command, const char *type);
-    // FIXME: Improve for errno modeling.
-    addToFunctionSummaryMap(
-        "popen",
-        Signature(ArgTypes{ConstCharPtrTy, ConstCharPtrTy}, RetType{FilePtrTy}),
-        Summary(NoEvalCall)
-            .ArgConstraint(NotNull(ArgNo(0)))
-            .ArgConstraint(NotNull(ArgNo(1))));
-
-    // int pclose(FILE *stream);
-    // FIXME: Improve for errno modeling.
-    addToFunctionSummaryMap(
-        "pclose", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}),
-        Summary(NoEvalCall).ArgConstraint(NotNull(ArgNo(0))));
-
     // int close(int fildes);
     addToFunctionSummaryMap(
         "close", Signature(ArgTypes{IntTy}, RetType{IntTy}),
diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
index f8e3e546a7aed5..c41f22fef388e1 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -47,7 +47,9 @@ FILE *fopen(const char *restrict path, const char *restrict mode);
 FILE *fdopen(int fd, const char *mode);
 FILE *tmpfile(void);
 FILE *freopen(const char *restrict pathname, const char *restrict mode, FILE *restrict stream);
+FILE *popen(const char *command, const char *mode);
 int fclose(FILE *fp);
+int pclose(FILE *stream);
 size_t fread(void *restrict, size_t, size_t, FILE *restrict);
 size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
 int fgetc(FILE *stream);
diff --git a/clang/test/Analysis/std-c-library-functions-POSIX.c b/clang/test/Analysis/std-c-library-functions-POSIX.c
index 51b136d9ba3567..03aa8e2e00a75d 100644
--- a/clang/test/Analysis/std-c-library-functions-POSIX.c
+++ b/clang/test/Analysis/std-c-library-functions-POSIX.c
@@ -20,7 +20,9 @@
 // CHECK: Loaded summary for: FILE *fdopen(int fd, const char *mode)
 // CHECK: Loaded summary for: FILE *tmpfile(void)
 // CHECK: Loaded summary for: FILE *freopen(const char *restrict pathname, const char *restrict mode, FILE *restrict stream)
+// CHECK: Loaded summary for: FILE *popen(const char *command, const char *type)
 // CHECK: Loaded summary for: int fclose(FILE *stream)
+// CHECK: Loaded summary for: int pclose(FILE *stream)
 // CHECK: Loaded summary for: int fseek(FILE *stream, long offset, int whence)
 // CHECK: Loaded summary for: int fseeko(FILE *stream, off_t offset, int whence)
 // CHECK: Loaded summary for: off_t ftello(FILE *stream)
@@ -74,8 +76,6 @@
 // CHECK: Loaded summary for: DIR *opendir(const char *name)
 // CHECK: Loaded summary for: DIR *fdopendir(int fd)
 // CHECK: Loaded summary for: int isatty(int fildes)
-// CHECK: Loaded summary for: FILE *popen(const char *command, const char *type)
-// CHECK: Loaded summary for: int pclose(FILE *stream)
 // CHECK: Loaded summary for: int close(int fildes)
 // CHECK: Loaded summary for: long fpathconf(int fildes, int name)
 // CHECK: Loaded summary for: long pathconf(const char *path, int name)
diff --git a/clang/test/Analysis/stream-errno.c b/clang/test/Analysis/stream-errno.c
index fab6a58b3275a8..44bf949673082d 100644
--- a/clang/test/Analysis/stream-errno.c
+++ b/clang/test/Analysis/stream-errno.c
@@ -51,6 +51,17 @@ void check_freopen(void) {
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
 }
 
+void check_popen(void) {
+  FILE *F = popen("xxx", "r");
+  if (!F) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+    if (errno) {}                    // no-warning
+  } else {
+    if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [unix.Errno]}}
+    pclose(F);
+  }
+}
+
 void check_fclose(void) {
   FILE *F = tmpfile();
   if (!F)
@@ -64,6 +75,20 @@ void check_fclose(void) {
   if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
 }
 
+void check_pclose(void) {
+  FILE *F = popen("xx", "w");
+  if (!F)
+    return;
+  int Ret = pclose(F);
+  if (Ret == -1) {
+    clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}}
+    if (errno) {}                    // no-warning
+  } else {
+    clang_analyzer_eval(Ret >= 0);   // expected-warning{{TRUE}}
+    if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}}
+  }
+}
+
 void check_fread_size0(void) {
   char Buf[10];
   FILE *F = tmpfile();

clang/test/Analysis/stream-errno.c

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

…aryFunctionsChecker

benshi001 requested review from steakhal, balazske and NagyDonat January 21, 2024 10:30

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Jan 21, 2024

balazske reviewed Jan 23, 2024

View reviewed changes

clang/test/Analysis/stream-errno.c Outdated Show resolved Hide resolved

benshi001 requested a review from balazske January 24, 2024 01:40

steakhal requested changes Jan 24, 2024

View reviewed changes

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp Show resolved Hide resolved

benshi001 added 2 commits January 25, 2024 12:54

[clang][analyzer] Improve modeling of 'popen' and 'pclose' in StdLibr…

382ae9d

…aryFunctionsChecker

[clang][analyzer] Improve modeling of 'popen' and 'pclose' in StdLibr…

8a57c1a

…aryFunctionsChecker

benshi001 requested a review from steakhal January 25, 2024 07:55

steakhal approved these changes Jan 25, 2024

View reviewed changes

balazske approved these changes Jan 25, 2024

View reviewed changes

benshi001 merged commit ff05c30 into llvm:main Jan 27, 2024
3 of 4 checks passed

benshi001 deleted the csa-pipe branch January 27, 2024 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][analyzer] Improve modeling of 'popen' and 'pclose' in StdLibraryFunctionsChecker #78895

[clang][analyzer] Improve modeling of 'popen' and 'pclose' in StdLibraryFunctionsChecker #78895

benshi001 commented Jan 21, 2024

llvmbot commented Jan 21, 2024

llvmbot commented Jan 21, 2024

[clang][analyzer] Improve modeling of 'popen' and 'pclose' in StdLibraryFunctionsChecker #78895

[clang][analyzer] Improve modeling of 'popen' and 'pclose' in StdLibraryFunctionsChecker #78895

Conversation

benshi001 commented Jan 21, 2024

llvmbot commented Jan 21, 2024

llvmbot commented Jan 21, 2024