NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
Through C cast magic it's possible to put a raw void pointer into a variable of
non-void pointer type. It is fine - generally, it's possible to put anything
into anywhere by temporarily re-interpreting the anywhere. We cast it back to
the correct type during load - with the help of
`StoreManager::CastRetrievedVal()`.
The attached example demonstrates that we're not casting the retrieved value
pedantically enough when it comes to retrieving pointer values. In fact, we
don't cast pointers-to-pointers at all because `SValBuilder` doesn't know how
to do that or even that it needs to do that at all.
In this patch i perform the pointer cast manually for the situation that causes
the crash. Performing the cast more carefully in other cases is possible, but i
didn't manage to produce an observable effect (the second test passes just fine
without it, and works correctly).
I cannot put the `castRegion()` call into `SValBuilder` because `SValBuilder`
doesn't have access to `StoreManager` when doing casts. We really need to
decouple this stuff and make a single good function for handling casts, because
understanding how current code for pointer casts works is a nightmare.
Repository:
rC Clang
https://reviews.llvm.org/D46415
Files:
lib/StaticAnalyzer/Core/Store.cpp
test/Analysis/casts.c
Index: test/Analysis/casts.c
===================================================================
--- test/Analysis/casts.c
+++ test/Analysis/casts.c
@@ -149,3 +149,19 @@
clang_analyzer_eval(*((char *)y1) == *((char *) y3)); //
expected-warning{{TRUE}}
}
+
+void *getVoidPtr();
+
+void testCastVoidPtrToIntPtrThroughIntTypedAssignment() {
+ int *x;
+ (*((int *)(&x))) = (int)getVoidPtr();
+ *x = 1; // no-crash
+}
+
+void testCastCharPtrToIntPtrThroughIntTypedAssignment() {
+ unsigned c;
+ int *x;
+ (*((int *)(&x))) = (int)&c;
+ *x = 1;
+ clang_analyzer_eval(c == 1); // expected-warning{{TRUE}}
+}
Index: lib/StaticAnalyzer/Core/Store.cpp
===================================================================
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -378,6 +378,20 @@
if (castTy.isNull() || V.isUnknownOrUndef())
return V;
+ // When retrieving symbolic pointer and expecting a non-void pointer,
+ // wrap them into element regions of the expected type if necessary.
+ // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
+ // make sure that the retrieved value makes sense, because there's no other
+ // cast in the AST that would tell us to cast it to the correct pointer type.
+ // We might need to do that for non-void pointers as well.
+ // FIXME: We really need a single good function to perform casts for us
+ // correctly every time we need it.
+ if (castTy->isPointerType() && !castTy->isVoidPointerType())
+ if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(V.getAsRegion()))
+ if (SR->getSymbol()->getType().getCanonicalType() !=
+ castTy.getCanonicalType())
+ return loc::MemRegionVal(castRegion(SR, castTy));
+
return svalBuilder.dispatchCast(V, castTy);
}
Index: test/Analysis/casts.c
===================================================================
--- test/Analysis/casts.c
+++ test/Analysis/casts.c
@@ -149,3 +149,19 @@
clang_analyzer_eval(*((char *)y1) == *((char *) y3)); // expected-warning{{TRUE}}
}
+
+void *getVoidPtr();
+
+void testCastVoidPtrToIntPtrThroughIntTypedAssignment() {
+ int *x;
+ (*((int *)(&x))) = (int)getVoidPtr();
+ *x = 1; // no-crash
+}
+
+void testCastCharPtrToIntPtrThroughIntTypedAssignment() {
+ unsigned c;
+ int *x;
+ (*((int *)(&x))) = (int)&c;
+ *x = 1;
+ clang_analyzer_eval(c == 1); // expected-warning{{TRUE}}
+}
Index: lib/StaticAnalyzer/Core/Store.cpp
===================================================================
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -378,6 +378,20 @@
if (castTy.isNull() || V.isUnknownOrUndef())
return V;
+ // When retrieving symbolic pointer and expecting a non-void pointer,
+ // wrap them into element regions of the expected type if necessary.
+ // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
+ // make sure that the retrieved value makes sense, because there's no other
+ // cast in the AST that would tell us to cast it to the correct pointer type.
+ // We might need to do that for non-void pointers as well.
+ // FIXME: We really need a single good function to perform casts for us
+ // correctly every time we need it.
+ if (castTy->isPointerType() && !castTy->isVoidPointerType())
+ if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(V.getAsRegion()))
+ if (SR->getSymbol()->getType().getCanonicalType() !=
+ castTy.getCanonicalType())
+ return loc::MemRegionVal(castRegion(SR, castTy));
+
return svalBuilder.dispatchCast(V, castTy);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
