From bb9cb77cab7b073d45c0b998c926a0b60a75a35e Mon Sep 17 00:00:00 2001 From: Gabor Spaits Date: Tue, 10 Oct 2023 13:40:05 +0200 Subject: [PATCH 1/6] [analyzer] Fix note for member reference --- .../Core/BugReporterVisitors.cpp | 57 ++++++++++++++----- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 42d03f67510cf88..914011f367d8f2e 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -132,6 +132,18 @@ const Expr *bugreporter::getDerefExpr(const Stmt *S) { } // Pattern match for a few useful cases: a[0], p->f, *p etc. else if (const auto *ME = dyn_cast(E)) { + // This handles the case when the dereferencing of a member reference + // happens. This is needed, because the ast for dereferencing of a + // member reference looks like the following: + // |-MemberExpr + // `-DeclRefExpr + // This branch without the special case just takes out the DeclRefExpr + // of the struct, class or union. + // This is wrong, because this DeclRefExpr will be passed + // to the bug reporting and the notes will refer to wrong variable + // (the struct instead of the member). + if (ME->getMemberDecl()->getType()->isReferenceType()) + break; E = ME->getBase(); } else if (const auto *IvarRef = dyn_cast(E)) { E = IvarRef->getBase(); @@ -157,26 +169,43 @@ const Expr *bugreporter::getDerefExpr(const Stmt *S) { return E; } +static const VarDecl *getVarDeclForExpression(const Expr *E) { + if (const auto *DR = dyn_cast(E)) + return dyn_cast(DR->getDecl()); + return nullptr; +} + static const MemRegion * getLocationRegionIfReference(const Expr *E, const ExplodedNode *N, bool LookingForReference = true) { - if (const auto *DR = dyn_cast(E)) { - if (const auto *VD = dyn_cast(DR->getDecl())) { - if (LookingForReference && !VD->getType()->isReferenceType()) - return nullptr; - return N->getState() - ->getLValue(VD, N->getLocationContext()) - .getAsRegion(); + if (const auto *ME = dyn_cast(E)) { + // This handles other kinds of null references, + // for example, references from FieldRegions: + // struct Wrapper { int &ref; }; + // Wrapper w = { *(int *)0 }; + // w.ref = 1; + const Expr *Base = ME->getBase(); + const VarDecl *VD = getVarDeclForExpression(Base); + if (!VD) + return nullptr; + + const auto *FD = dyn_cast(ME->getMemberDecl()); + if (!FD) + return nullptr; + + if (FD->getType()->isReferenceType()) { + SVal StructSVal = N->getState()->getLValue(VD, N->getLocationContext()); + return N->getState()->getLValue(FD, StructSVal).getAsRegion(); } + return nullptr; } - // FIXME: This does not handle other kinds of null references, - // for example, references from FieldRegions: - // struct Wrapper { int &ref; }; - // Wrapper w = { *(int *)0 }; - // w.ref = 1; - - return nullptr; + const VarDecl *VD = getVarDeclForExpression(E); + if (!VD) + return nullptr; + if (LookingForReference && !VD->getType()->isReferenceType()) + return nullptr; + return N->getState()->getLValue(VD, N->getLocationContext()).getAsRegion(); } /// Comparing internal representations of symbolic values (via From 5bf3ba5f3b66d972e3c26c8f782ae077700e459a Mon Sep 17 00:00:00 2001 From: Gabor Spaits Date: Tue, 10 Oct 2023 15:26:28 +0200 Subject: [PATCH 2/6] Add test cases for null reference reports --- .../deref-track-symbolic-region.cpp | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp b/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp index e258a60aa966a53..2442851da989b16 100644 --- a/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp +++ b/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp @@ -41,3 +41,34 @@ int testRefToNullPtr2() { return *p2; //expected-warning {{Dereference of null pointer}} // expected-note@-1{{Dereference of null pointer}} } + +void testMemberNullPointerDeref() { + struct Wrapper {char c; int *ptr; }; + Wrapper w = {'a', nullptr}; // expected-note {{'w.ptr' initialized to a null pointer value}} + *w.ptr = 1; //expected-warning {{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} +} + +void testMemberNullReferenceDeref() { + struct Wrapper {char c; int &ref; }; + Wrapper w = {.c = 'a', .ref = *(int *)0 }; // expected-note {{'w.ref' initialized to a null pointer value}} + // expected-warning@-1 {{binding dereferenced null pointer to reference has undefined behavior}} + w.ref = 1; //expected-warning {{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} +} + +void testReferenceToPointerWithNullptr() { + int *i = nullptr; // expected-note {{'i' initialized to a null pointer value}} + struct Wrapper {char c; int *&a;}; + Wrapper w {'c', i}; // expected-note{{'w.a' initialized here}} + *(w.a) = 25; // expected-warning {{Dereference of null pointer}} + // expected-note@-1 {{Dereference of null pointer}} +} + +void aaa() { + struct Wrapper {char c; int *&a;}; + Wrapper w {'c', *(int **)0 }; // expected-note{{'w.a' initialized to a null pointer value}} + // expected-warning@-1 {{binding dereferenced null pointer to reference has undefined behavior}} + w.a = nullptr; // expected-warning {{Dereference of null pointer}} + // expected-note@-1 {{Dereference of null pointer}} +} \ No newline at end of file From 6f9ddb9d6f598a65ed3fca602329aa26bb5e3458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Spaits?= <48805437+spaits@users.noreply.github.com> Date: Thu, 12 Oct 2023 13:59:19 +0200 Subject: [PATCH 3/6] Update test function name Co-authored-by: DonatNagyE --- clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp b/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp index 2442851da989b16..e9f62c2407e88b4 100644 --- a/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp +++ b/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp @@ -65,7 +65,7 @@ void testReferenceToPointerWithNullptr() { // expected-note@-1 {{Dereference of null pointer}} } -void aaa() { +void testNullReferenceToPointer() { struct Wrapper {char c; int *&a;}; Wrapper w {'c', *(int **)0 }; // expected-note{{'w.a' initialized to a null pointer value}} // expected-warning@-1 {{binding dereferenced null pointer to reference has undefined behavior}} From 43e544ad5b45bb2586dc5ca7825a0e6227d50220 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Spaits?= <48805437+spaits@users.noreply.github.com> Date: Thu, 12 Oct 2023 14:00:53 +0200 Subject: [PATCH 4/6] Clarify explanation Co-authored-by: DonatNagyE --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 914011f367d8f2e..025214106a63b38 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -137,11 +137,9 @@ const Expr *bugreporter::getDerefExpr(const Stmt *S) { // member reference looks like the following: // |-MemberExpr // `-DeclRefExpr - // This branch without the special case just takes out the DeclRefExpr - // of the struct, class or union. - // This is wrong, because this DeclRefExpr will be passed - // to the bug reporting and the notes will refer to wrong variable - // (the struct instead of the member). + // Without this special case the notes would refer to the whole object + // (struct, class or union variable) instead of just the relevant member. + if (ME->getMemberDecl()->getType()->isReferenceType()) break; E = ME->getBase(); From 13fd05a709e93b93c4fd6c8135cf258deec7ebe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Spaits?= <48805437+spaits@users.noreply.github.com> Date: Thu, 12 Oct 2023 14:02:14 +0200 Subject: [PATCH 5/6] Simplify descriotion Co-authored-by: DonatNagyE --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 025214106a63b38..b2df8a976bcf04b 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -177,8 +177,7 @@ static const MemRegion * getLocationRegionIfReference(const Expr *E, const ExplodedNode *N, bool LookingForReference = true) { if (const auto *ME = dyn_cast(E)) { - // This handles other kinds of null references, - // for example, references from FieldRegions: + // This handles null references from FieldRegions, for example: // struct Wrapper { int &ref; }; // Wrapper w = { *(int *)0 }; // w.ref = 1; From 995e85eed2e2d48e4209e48437db04ce4f431259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Spaits?= <48805437+spaits@users.noreply.github.com> Date: Thu, 12 Oct 2023 14:03:35 +0200 Subject: [PATCH 6/6] Fix acronym with lowercase letters Co-authored-by: DonatNagyE --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index b2df8a976bcf04b..2d184d529513253 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -133,7 +133,7 @@ const Expr *bugreporter::getDerefExpr(const Stmt *S) { // Pattern match for a few useful cases: a[0], p->f, *p etc. else if (const auto *ME = dyn_cast(E)) { // This handles the case when the dereferencing of a member reference - // happens. This is needed, because the ast for dereferencing of a + // happens. This is needed, because the AST for dereferencing a // member reference looks like the following: // |-MemberExpr // `-DeclRefExpr