[analyzer] Fix crash when modelling 'getline' function in checkers #145229
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Baranov Victor (vbvictor) ChangesFixed crush caused by types mismatch of expected There are 2 ways to fix the issue:
I currently implemented 1st solution, but I'm happy to implement 2nd if it's more appropriate for CSA, WDYT? Addresses #144884. Full diff: https://github.com/llvm/llvm-project/pull/145229.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 12816eed2e8b5..b6f6dce29a87b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1060,6 +1060,9 @@ impact the linker behaviour like the other `-static-*` flags.
Crash and bug fixes
^^^^^^^^^^^^^^^^^^^
+- Fixed a crash in ``UnixAPIMisuseChecker`` and ``MallocChecker`` when analyzing
+ code with non-standard ``getline`` or ``getdelim`` function signatures.
+
Improvements
^^^^^^^^^^^^
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 35e98a5e2719a..b3f0110539ffa 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1482,11 +1482,63 @@ static bool isFromStdNamespace(const CallEvent &Call) {
return FD->isInStdNamespace();
}
+static bool IsGetDelim(const CallEvent &Call, CheckerContext &C) {
+ const FunctionDecl *FD = dyn_cast_if_present<FunctionDecl>(Call.getDecl());
+ if (!FD || FD->getKind() != Decl::Function)
+ return false;
+
+ const StringRef FName = C.getCalleeName(FD);
+ const bool IsDelim = FName == "getdelim";
+ const bool IsLine = FName == "getline";
+
+ unsigned ExpectedNumArgs = 0;
+ if (IsDelim)
+ ExpectedNumArgs = 4;
+ else if (IsLine)
+ ExpectedNumArgs = 3;
+ else
+ return false;
+
+ if (FD->getNumParams() != ExpectedNumArgs)
+ return false;
+
+ // Should be char**
+ const QualType CharPtrType = FD->getParamDecl(0)->getType()->getPointeeType();
+ if (CharPtrType.isNull())
+ return false;
+ const QualType CharType = CharPtrType->getPointeeType();
+ if (CharType.isNull() || !CharType->isCharType())
+ return false;
+
+ // Should be size_t*
+ const QualType SizePtrType = FD->getParamDecl(1)->getType();
+ if (!SizePtrType->isPointerType())
+ return false;
+ const QualType SizeArgType = SizePtrType->getPointeeType();
+ const ASTContext &Ctx = C.getASTContext();
+ if (SizeArgType.isNull() ||
+ !Ctx.hasSameUnqualifiedType(SizeArgType, Ctx.getSizeType()))
+ return false;
+
+ // For getdelim, should be int
+ if (IsDelim)
+ if (!FD->getParamDecl(2)->getType()->isIntegerType())
+ return false;
+
+ // Last parameter should be FILE*
+ const QualType Pointee =
+ FD->getParamDecl(IsDelim ? 3 : 2)->getType()->getPointeeType();
+ if (Pointee.isNull() || Pointee.getAsString() != "FILE")
+ return false;
+
+ return true;
+}
+
void MallocChecker::preGetdelim(ProgramStateRef State, const CallEvent &Call,
CheckerContext &C) const {
// Discard calls to the C++ standard library function std::getline(), which
// is completely unrelated to the POSIX getline() that we're checking.
- if (isFromStdNamespace(Call))
+ if (isFromStdNamespace(Call) || !IsGetDelim(Call, C))
return;
const auto LinePtr = getPointeeVal(Call.getArgSVal(0), State);
@@ -1509,7 +1561,7 @@ void MallocChecker::checkGetdelim(ProgramStateRef State, const CallEvent &Call,
CheckerContext &C) const {
// Discard calls to the C++ standard library function std::getline(), which
// is completely unrelated to the POSIX getline() that we're checking.
- if (isFromStdNamespace(Call))
+ if (isFromStdNamespace(Call) || !IsGetDelim(Call, C))
return;
// Handle the post-conditions of getline and getdelim:
diff --git a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
index 79d10d99e11d0..11d82f6de5e62 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -147,6 +147,55 @@ ProgramStateRef UnixAPIMisuseChecker::EnsurePtrNotNull(
return PtrNotNull;
}
+static bool IsGetDelim(const CallEvent &Call, const FunctionDecl *FD,
+ CheckerContext &C) {
+ const StringRef FName = C.getCalleeName(FD);
+ const bool IsDelim = FName == "getdelim";
+ const bool IsLine = FName == "getline";
+
+ unsigned ExpectedNumArgs = 0;
+ if (IsDelim)
+ ExpectedNumArgs = 4;
+ else if (IsLine)
+ ExpectedNumArgs = 3;
+ else
+ return false;
+
+ if (FD->getNumParams() != ExpectedNumArgs)
+ return false;
+
+ // Should be char**
+ const QualType CharPtrType = FD->getParamDecl(0)->getType()->getPointeeType();
+ if (CharPtrType.isNull())
+ return false;
+ const QualType CharType = CharPtrType->getPointeeType();
+ if (CharType.isNull() || !CharType->isCharType())
+ return false;
+
+ // Should be size_t*
+ const QualType SizePtrType = FD->getParamDecl(1)->getType();
+ if (!SizePtrType->isPointerType())
+ return false;
+ const QualType SizeArgType = SizePtrType->getPointeeType();
+ const ASTContext &Ctx = C.getASTContext();
+ if (SizeArgType.isNull() ||
+ !Ctx.hasSameUnqualifiedType(SizeArgType, Ctx.getSizeType()))
+ return false;
+
+ // For getdelim, should be int
+ if (IsDelim)
+ if (!FD->getParamDecl(2)->getType()->isIntegerType())
+ return false;
+
+ // Last parameter should be FILE*
+ const QualType Pointee =
+ FD->getParamDecl(IsDelim ? 3 : 2)->getType()->getPointeeType();
+ if (Pointee.isNull() || Pointee.getAsString() != "FILE")
+ return false;
+
+ return true;
+}
+
//===----------------------------------------------------------------------===//
// "open" (man 2 open)
//===----------------------------------------------------------------------===/
@@ -176,7 +225,7 @@ void UnixAPIMisuseChecker::checkPreCall(const CallEvent &Call,
else if (FName == "pthread_once")
CheckPthreadOnce(C, Call);
- else if (is_contained({"getdelim", "getline"}, FName))
+ else if (IsGetDelim(Call, FD, C))
CheckGetDelim(C, Call);
}
void UnixAPIMisuseChecker::ReportOpenBug(CheckerContext &C,
diff --git a/clang/test/Analysis/getline-unixapi-invalid-signatures.c b/clang/test/Analysis/getline-unixapi-invalid-signatures.c
new file mode 100644
index 0000000000000..ad218937f1184
--- /dev/null
+++ b/clang/test/Analysis/getline-unixapi-invalid-signatures.c
@@ -0,0 +1,130 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_CORRECT
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_1
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_2
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_3
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_4
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_5
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_GH144884
+
+// emulator of "system-header-simulator.h" because of redefinition of 'getline' function
+typedef struct _FILE FILE;
+typedef __typeof(sizeof(int)) size_t;
+typedef long ssize_t;
+#define NULL 0
+
+int fclose(FILE *fp);
+FILE *tmpfile(void);
+
+#ifdef TEST_CORRECT
+ssize_t getline(char **lineptr, size_t *n, FILE *stream);
+ssize_t getdelim(char **lineptr, size_t *n, int delimiter, FILE *stream);
+
+void test_correct() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, NULL, F1); // expected-warning {{Size pointer might be NULL}}
+ fclose(F1);
+}
+
+void test_delim_correct() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getdelim(&buffer, NULL, ',', F1); // expected-warning {{Size pointer might be NULL}}
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_1
+// expected-no-diagnostics
+ssize_t getline(int lineptr);
+
+void test() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ int buffer = 0;
+ getline(buffer);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_2
+// expected-no-diagnostics
+ssize_t getline(char **lineptr, size_t *n);
+
+void test() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, NULL);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_3
+// expected-no-diagnostics
+ssize_t getline(char **lineptr, size_t n, FILE *stream);
+
+void test() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, 0, F1);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_4
+// expected-no-diagnostics
+ssize_t getline(char **lineptr, size_t *n, int stream);
+ssize_t getdelim(char **lineptr, size_t *n, int delimiter, int stream);
+
+void test() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, NULL, 1);
+ fclose(F1);
+}
+
+void test_delim() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getdelim(&buffer, NULL, ',', 1);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_5
+// expected-no-diagnostics
+ssize_t getdelim(char **lineptr, size_t *n, const char* delimiter, FILE *stream);
+
+void test_delim() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getdelim(&buffer, NULL, ",", F1);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_GH144884
+// expected-no-diagnostics
+struct AW_string {};
+void getline(int *, struct AW_string);
+void top() {
+ struct AW_string line;
+ int getline_file_info;
+ getline(&getline_file_info, line);
+}
+#endif
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Baranov Victor (vbvictor) ChangesFixed crush caused by types mismatch of expected There are 2 ways to fix the issue:
I currently implemented 1st solution, but I'm happy to implement 2nd if it's more appropriate for CSA, WDYT? Addresses #144884. Full diff: https://github.com/llvm/llvm-project/pull/145229.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 12816eed2e8b5..b6f6dce29a87b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1060,6 +1060,9 @@ impact the linker behaviour like the other `-static-*` flags.
Crash and bug fixes
^^^^^^^^^^^^^^^^^^^
+- Fixed a crash in ``UnixAPIMisuseChecker`` and ``MallocChecker`` when analyzing
+ code with non-standard ``getline`` or ``getdelim`` function signatures.
+
Improvements
^^^^^^^^^^^^
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 35e98a5e2719a..b3f0110539ffa 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1482,11 +1482,63 @@ static bool isFromStdNamespace(const CallEvent &Call) {
return FD->isInStdNamespace();
}
+static bool IsGetDelim(const CallEvent &Call, CheckerContext &C) {
+ const FunctionDecl *FD = dyn_cast_if_present<FunctionDecl>(Call.getDecl());
+ if (!FD || FD->getKind() != Decl::Function)
+ return false;
+
+ const StringRef FName = C.getCalleeName(FD);
+ const bool IsDelim = FName == "getdelim";
+ const bool IsLine = FName == "getline";
+
+ unsigned ExpectedNumArgs = 0;
+ if (IsDelim)
+ ExpectedNumArgs = 4;
+ else if (IsLine)
+ ExpectedNumArgs = 3;
+ else
+ return false;
+
+ if (FD->getNumParams() != ExpectedNumArgs)
+ return false;
+
+ // Should be char**
+ const QualType CharPtrType = FD->getParamDecl(0)->getType()->getPointeeType();
+ if (CharPtrType.isNull())
+ return false;
+ const QualType CharType = CharPtrType->getPointeeType();
+ if (CharType.isNull() || !CharType->isCharType())
+ return false;
+
+ // Should be size_t*
+ const QualType SizePtrType = FD->getParamDecl(1)->getType();
+ if (!SizePtrType->isPointerType())
+ return false;
+ const QualType SizeArgType = SizePtrType->getPointeeType();
+ const ASTContext &Ctx = C.getASTContext();
+ if (SizeArgType.isNull() ||
+ !Ctx.hasSameUnqualifiedType(SizeArgType, Ctx.getSizeType()))
+ return false;
+
+ // For getdelim, should be int
+ if (IsDelim)
+ if (!FD->getParamDecl(2)->getType()->isIntegerType())
+ return false;
+
+ // Last parameter should be FILE*
+ const QualType Pointee =
+ FD->getParamDecl(IsDelim ? 3 : 2)->getType()->getPointeeType();
+ if (Pointee.isNull() || Pointee.getAsString() != "FILE")
+ return false;
+
+ return true;
+}
+
void MallocChecker::preGetdelim(ProgramStateRef State, const CallEvent &Call,
CheckerContext &C) const {
// Discard calls to the C++ standard library function std::getline(), which
// is completely unrelated to the POSIX getline() that we're checking.
- if (isFromStdNamespace(Call))
+ if (isFromStdNamespace(Call) || !IsGetDelim(Call, C))
return;
const auto LinePtr = getPointeeVal(Call.getArgSVal(0), State);
@@ -1509,7 +1561,7 @@ void MallocChecker::checkGetdelim(ProgramStateRef State, const CallEvent &Call,
CheckerContext &C) const {
// Discard calls to the C++ standard library function std::getline(), which
// is completely unrelated to the POSIX getline() that we're checking.
- if (isFromStdNamespace(Call))
+ if (isFromStdNamespace(Call) || !IsGetDelim(Call, C))
return;
// Handle the post-conditions of getline and getdelim:
diff --git a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
index 79d10d99e11d0..11d82f6de5e62 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -147,6 +147,55 @@ ProgramStateRef UnixAPIMisuseChecker::EnsurePtrNotNull(
return PtrNotNull;
}
+static bool IsGetDelim(const CallEvent &Call, const FunctionDecl *FD,
+ CheckerContext &C) {
+ const StringRef FName = C.getCalleeName(FD);
+ const bool IsDelim = FName == "getdelim";
+ const bool IsLine = FName == "getline";
+
+ unsigned ExpectedNumArgs = 0;
+ if (IsDelim)
+ ExpectedNumArgs = 4;
+ else if (IsLine)
+ ExpectedNumArgs = 3;
+ else
+ return false;
+
+ if (FD->getNumParams() != ExpectedNumArgs)
+ return false;
+
+ // Should be char**
+ const QualType CharPtrType = FD->getParamDecl(0)->getType()->getPointeeType();
+ if (CharPtrType.isNull())
+ return false;
+ const QualType CharType = CharPtrType->getPointeeType();
+ if (CharType.isNull() || !CharType->isCharType())
+ return false;
+
+ // Should be size_t*
+ const QualType SizePtrType = FD->getParamDecl(1)->getType();
+ if (!SizePtrType->isPointerType())
+ return false;
+ const QualType SizeArgType = SizePtrType->getPointeeType();
+ const ASTContext &Ctx = C.getASTContext();
+ if (SizeArgType.isNull() ||
+ !Ctx.hasSameUnqualifiedType(SizeArgType, Ctx.getSizeType()))
+ return false;
+
+ // For getdelim, should be int
+ if (IsDelim)
+ if (!FD->getParamDecl(2)->getType()->isIntegerType())
+ return false;
+
+ // Last parameter should be FILE*
+ const QualType Pointee =
+ FD->getParamDecl(IsDelim ? 3 : 2)->getType()->getPointeeType();
+ if (Pointee.isNull() || Pointee.getAsString() != "FILE")
+ return false;
+
+ return true;
+}
+
//===----------------------------------------------------------------------===//
// "open" (man 2 open)
//===----------------------------------------------------------------------===/
@@ -176,7 +225,7 @@ void UnixAPIMisuseChecker::checkPreCall(const CallEvent &Call,
else if (FName == "pthread_once")
CheckPthreadOnce(C, Call);
- else if (is_contained({"getdelim", "getline"}, FName))
+ else if (IsGetDelim(Call, FD, C))
CheckGetDelim(C, Call);
}
void UnixAPIMisuseChecker::ReportOpenBug(CheckerContext &C,
diff --git a/clang/test/Analysis/getline-unixapi-invalid-signatures.c b/clang/test/Analysis/getline-unixapi-invalid-signatures.c
new file mode 100644
index 0000000000000..ad218937f1184
--- /dev/null
+++ b/clang/test/Analysis/getline-unixapi-invalid-signatures.c
@@ -0,0 +1,130 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_CORRECT
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_1
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_2
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_3
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_4
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_5
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -verify %s -DTEST_GETLINE_GH144884
+
+// emulator of "system-header-simulator.h" because of redefinition of 'getline' function
+typedef struct _FILE FILE;
+typedef __typeof(sizeof(int)) size_t;
+typedef long ssize_t;
+#define NULL 0
+
+int fclose(FILE *fp);
+FILE *tmpfile(void);
+
+#ifdef TEST_CORRECT
+ssize_t getline(char **lineptr, size_t *n, FILE *stream);
+ssize_t getdelim(char **lineptr, size_t *n, int delimiter, FILE *stream);
+
+void test_correct() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, NULL, F1); // expected-warning {{Size pointer might be NULL}}
+ fclose(F1);
+}
+
+void test_delim_correct() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getdelim(&buffer, NULL, ',', F1); // expected-warning {{Size pointer might be NULL}}
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_1
+// expected-no-diagnostics
+ssize_t getline(int lineptr);
+
+void test() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ int buffer = 0;
+ getline(buffer);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_2
+// expected-no-diagnostics
+ssize_t getline(char **lineptr, size_t *n);
+
+void test() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, NULL);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_3
+// expected-no-diagnostics
+ssize_t getline(char **lineptr, size_t n, FILE *stream);
+
+void test() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, 0, F1);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_4
+// expected-no-diagnostics
+ssize_t getline(char **lineptr, size_t *n, int stream);
+ssize_t getdelim(char **lineptr, size_t *n, int delimiter, int stream);
+
+void test() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getline(&buffer, NULL, 1);
+ fclose(F1);
+}
+
+void test_delim() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getdelim(&buffer, NULL, ',', 1);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_5
+// expected-no-diagnostics
+ssize_t getdelim(char **lineptr, size_t *n, const char* delimiter, FILE *stream);
+
+void test_delim() {
+ FILE *F1 = tmpfile();
+ if (!F1)
+ return;
+ char *buffer = NULL;
+ getdelim(&buffer, NULL, ",", F1);
+ fclose(F1);
+}
+#endif
+
+#ifdef TEST_GETLINE_GH144884
+// expected-no-diagnostics
+struct AW_string {};
+void getline(int *, struct AW_string);
+void top() {
+ struct AW_string line;
+ int getline_file_info;
+ getline(&getline_file_info, line);
+}
+#endif
|
|
Ideally, we could reuse the signature matching of the |
|
Personally I feel that option 2 ("add more error handling to avoid crashes in Your concern that "However, we will still try to model "incorrect" |
Indeed, without measurements, this argument is not viable. I've investigated the code of For now, I can implement more error-handling as in option 2 and refactor |
|
Modeling a function should only happen if that's the intended function. Sometimes this matching is deliberately fuzzy, but most often it's incidentally so and they should have really check the parameter and return types for exact match. Or at least the parameters that the model cares about. Such incidental matches are usually not a big deal, unless they lead to false assumptions thus crashes like in this case.
I don't want to push you work, but I'm curious. If it was easy to hoist the |
vbvictor
force-pushed
the
fix-geline-crash
branch
from
d2793c2 to
82f9579
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vbvictor
force-pushed
the
fix-geline-crash
branch
from
82f9579 to
ba9f1ed
Compare
It's "easier" to some extent, I think it's more readable and maintainable with full dedicated signature matching. I created a small PR in my LLVM fork to demonstrate how it can look like vbvictor#1. I updated this PR and implemented option 2. It has more gentle error handling, just avoid crushes. But it still accept all functions that "look like" I think it's still helpful to have strict function signature matching. Even in option 2 I needed to explicitly check types to avoid crush, so having a dedicated instrument would make it better. |
There was a problem hiding this comment.
Thanks for the update 🙂 !
I think the current revision of this PR is a good patch for fixing the crash in a clean and self-contained way. I also looked at the Signature interface (on the private branch that you linked), and it seems to be promising architectural improvement, but I would prefer keeping this bugfix and that internal architectural improvement in two separate commits if that isn't too much trouble.
I marked a few opportunities for minor simplification in inline comments, but once those are handled, I'd happily support merging this commit.
Yes, making |
steakhal
changed the title
|
Also updated names of function in |
Fixed crash caused by types mismatch of expected
getlineandgetdelimsignatures and actual user-defined signatures.There are 2 ways to fix the issue:
Make sure that all parameters of
CallEventunderlyingFunctionDeclhas the exact types of unixgetlinefunction. This way bloats the code by rigorous checking but may eliminate unnecessary transitions. I didn't find a way to eliminate code duplication, is there aUtilsdirectory for CSA to place such utility functions?Add more error-handling to
CheckGetDelimmethods to avoid crushes (as for now, it crushed trying to dereference an invalidoptional). However, we will still try to model "incorrect"getlinemethods, wasting cpu.I currently implemented 1st solution, but I'm happy to implement 2nd if it's more appropriate for CSA, WDYT?
I will create more PRs for other potential crushes of
unix-function if we settle on the way of fixing it.Addresses #144884.