[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8c4a65b9b2ca: [ubsan] Check implicit casts in ObjC for-in
statements (authored by vsk).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGObjC.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/CodeGenObjC/for-in.m
compiler-rt/lib/ubsan/ubsan_checks.inc
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_value.cpp
compiler-rt/lib/ubsan/ubsan_value.h
compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
Index: compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
@@ -0,0 +1,27 @@
+// REQUIRES: darwin
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast %s -O1 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast -fno-sanitize-recover=objc-cast %s -O1 -o %t.trap
+// RUN: not %run %t.trap 2>&1 | FileCheck %s
+
+#include
+
+int main() {
+ NSArray *arrayOfInt = [NSArray arrayWithObjects:@1, @2, @3, (void *)0];
+ // CHECK: objc-cast.m:[[@LINE+1]]:{{.*}}: runtime error: invalid ObjC cast, object is a '__NSCFNumber', but expected a 'NSString'
+ for (NSString *str in arrayOfInt) {
+NSLog(@"%@", str);
+ }
+
+ NSArray *arrayOfStr = [NSArray arrayWithObjects:@"a", @"b", @"c", (void *)0];
+ for (NSString *str in arrayOfStr) {
+NSLog(@"%@", str);
+ }
+
+ // The diagnostic should only be printed once.
+ // CHECK-NOT: runtime error
+
+ return 0;
+}
Index: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
===
--- compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
+++ compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
@@ -109,6 +109,7 @@
HANDLER(float_cast_overflow, "float-cast-overflow")
HANDLER(load_invalid_value, "load-invalid-value")
HANDLER(invalid_builtin, "invalid-builtin")
+HANDLER(invalid_objc_cast, "invalid-objc-cast")
HANDLER(function_type_mismatch, "function-type-mismatch")
HANDLER(implicit_conversion, "implicit-conversion")
HANDLER(nonnull_arg, "nonnull-arg")
Index: compiler-rt/lib/ubsan/ubsan_value.h
===
--- compiler-rt/lib/ubsan/ubsan_value.h
+++ compiler-rt/lib/ubsan/ubsan_value.h
@@ -135,6 +135,9 @@
/// \brief An opaque handle to a value.
typedef uptr ValueHandle;
+/// Returns the class name of the given ObjC object, or null if the name
+/// cannot be found.
+const char *getObjCClassName(ValueHandle Pointer);
/// \brief Representation of an operand value provided by the instrumented code.
///
Index: compiler-rt/lib/ubsan/ubsan_value.cpp
===
--- compiler-rt/lib/ubsan/ubsan_value.cpp
+++ compiler-rt/lib/ubsan/ubsan_value.cpp
@@ -16,9 +16,57 @@
#include "ubsan_value.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_mutex.h"
+
+// TODO(dliew): Prefer '__APPLE__' here over 'SANITIZER_MAC', as the latter is
+// unclear. rdar://58124919 tracks using a more obviously portable guard.
+#if defined(__APPLE__)
+#include
+#endif
using namespace __ubsan;
+typedef const char *(*ObjCGetClassNameTy)(void *);
+
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
+ // to introduce a static dependency from the ubsan runtime onto ObjC. Try to
+ // grab a handle to the ObjC runtime used by the process.
+ static bool AttemptedDlopen = false;
+ static void *ObjCHandle = nullptr;
+ static void *ObjCObjectGetClassName = nullptr;
+
+ // Prevent threads from racing to dlopen().
+ static __sanitizer::StaticSpinMutex Lock;
+ {
+__sanitizer::SpinMutexLock Guard();
+
+if (!AttemptedDlopen) {
+ ObjCHandle = dlopen(
+ "/usr/lib/libobjc.A.dylib",
+ RTLD_LAZY // Only bind symbols when used.
+ | RTLD_LOCAL // Only make symbols available via the handle.
+ | RTLD_NOLOAD // Do not load the dylib, just grab a handle if the
+// image is already loaded.
+ | RTLD_FIRST // Only search the image pointed-to by the handle.
+ );
+ AttemptedDlopen = true;
+ if (!ObjCHandle)
+return nullptr;
+
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
ahatanak added a comment. Thank you, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk updated this revision to Diff 273815.
vsk added a comment.
Use the `IsKindOfClass` CallArgList when emitting the check, and add a runtime
test to ensure that an objc-cast diagnostic is not emitted on correct code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGObjC.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/CodeGenObjC/for-in.m
compiler-rt/lib/ubsan/ubsan_checks.inc
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_value.cpp
compiler-rt/lib/ubsan/ubsan_value.h
compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
Index: compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
@@ -0,0 +1,27 @@
+// REQUIRES: darwin
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast %s -O1 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast -fno-sanitize-recover=objc-cast %s -O1 -o %t.trap
+// RUN: not %run %t.trap 2>&1 | FileCheck %s
+
+#include
+
+int main() {
+ NSArray *arrayOfInt = [NSArray arrayWithObjects:@1, @2, @3, (void *)0];
+ // CHECK: objc-cast.m:[[@LINE+1]]:{{.*}}: runtime error: invalid ObjC cast, object is a '__NSCFNumber', but expected a 'NSString'
+ for (NSString *str in arrayOfInt) {
+NSLog(@"%@", str);
+ }
+
+ NSArray *arrayOfStr = [NSArray arrayWithObjects:@"a", @"b", @"c", (void *)0];
+ for (NSString *str in arrayOfStr) {
+NSLog(@"%@", str);
+ }
+
+ // The diagnostic should only be printed once.
+ // CHECK-NOT: runtime error
+
+ return 0;
+}
Index: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
===
--- compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
+++ compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
@@ -109,6 +109,7 @@
HANDLER(float_cast_overflow, "float-cast-overflow")
HANDLER(load_invalid_value, "load-invalid-value")
HANDLER(invalid_builtin, "invalid-builtin")
+HANDLER(invalid_objc_cast, "invalid-objc-cast")
HANDLER(function_type_mismatch, "function-type-mismatch")
HANDLER(implicit_conversion, "implicit-conversion")
HANDLER(nonnull_arg, "nonnull-arg")
Index: compiler-rt/lib/ubsan/ubsan_value.h
===
--- compiler-rt/lib/ubsan/ubsan_value.h
+++ compiler-rt/lib/ubsan/ubsan_value.h
@@ -135,6 +135,9 @@
/// \brief An opaque handle to a value.
typedef uptr ValueHandle;
+/// Returns the class name of the given ObjC object, or null if the name
+/// cannot be found.
+const char *getObjCClassName(ValueHandle Pointer);
/// \brief Representation of an operand value provided by the instrumented code.
///
Index: compiler-rt/lib/ubsan/ubsan_value.cpp
===
--- compiler-rt/lib/ubsan/ubsan_value.cpp
+++ compiler-rt/lib/ubsan/ubsan_value.cpp
@@ -16,9 +16,57 @@
#include "ubsan_value.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_mutex.h"
+
+// TODO(dliew): Prefer '__APPLE__' here over 'SANITIZER_MAC', as the latter is
+// unclear. rdar://58124919 tracks using a more obviously portable guard.
+#if defined(__APPLE__)
+#include
+#endif
using namespace __ubsan;
+typedef const char *(*ObjCGetClassNameTy)(void *);
+
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
+ // to introduce a static dependency from the ubsan runtime onto ObjC. Try to
+ // grab a handle to the ObjC runtime used by the process.
+ static bool AttemptedDlopen = false;
+ static void *ObjCHandle = nullptr;
+ static void *ObjCObjectGetClassName = nullptr;
+
+ // Prevent threads from racing to dlopen().
+ static __sanitizer::StaticSpinMutex Lock;
+ {
+__sanitizer::SpinMutexLock Guard();
+
+if (!AttemptedDlopen) {
+ ObjCHandle = dlopen(
+ "/usr/lib/libobjc.A.dylib",
+ RTLD_LAZY // Only bind symbols when used.
+ | RTLD_LOCAL // Only make symbols available via the handle.
+ | RTLD_NOLOAD // Do not load the dylib, just grab a handle if the
+// image is already loaded.
+ | RTLD_FIRST // Only search the image pointed-to by the handle.
+ );
+ AttemptedDlopen = true;
+ if (!ObjCHandle)
+return nullptr;
+ ObjCObjectGetClassName = dlsym(ObjCHandle,
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk marked 2 inline comments as done. vsk added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1860 + CGM.getObjCRuntime().GetClass(*this, InterfaceTy->getDecl()); + Args.add(RValue::get(Cls), C.getObjCClassType()); + llvm::Value *IsClass = ahatanak wrote: > It looks like `Args` should be cleared before adding the argument. Or should > the argument be added to `IsKindOfClassArg`? Thanks for catching this. The argument should be added to `IsKindOfClassArg`. This wasn't working correctly before: a leftover `%struct.__objcFastEnumerationState*` was included in the message-send. I've added a runtime test to ensure that the diagnostic does not fire when the implicit cast is correct. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
ahatanak added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1856 + Selector IsKindOfClassSel = C.Selectors.getSelector( + llvm::array_lengthof(IsKindOfClassII), [0]); + CallArgList IsKindOfClassArgs; Can you use `GetUnarySelector` here? Comment at: clang/lib/CodeGen/CGObjC.cpp:1860 + CGM.getObjCRuntime().GetClass(*this, InterfaceTy->getDecl()); + Args.add(RValue::get(Cls), C.getObjCClassType()); + llvm::Value *IsClass = It looks like `Args` should be cleared before adding the argument. Or should the argument be added to `IsKindOfClassArg`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk updated this revision to Diff 271508.
vsk added a comment.
Herald added projects: clang, Sanitizers.
Herald added a subscriber: Sanitizers.
Rebase.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGObjC.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/CodeGenObjC/for-in.m
compiler-rt/lib/ubsan/ubsan_checks.inc
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_value.cpp
compiler-rt/lib/ubsan/ubsan_value.h
compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
Index: compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
@@ -0,0 +1,22 @@
+// REQUIRES: darwin
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast %s -O1 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast -fno-sanitize-recover=objc-cast %s -O1 -o %t.trap
+// RUN: not %run %t.trap 2>&1 | FileCheck %s
+
+#include
+
+int main() {
+ NSArray *array = [NSArray arrayWithObjects: @1, @2, @3, (void *)0];
+ // CHECK: objc-cast.m:[[@LINE+1]]:{{.*}}: runtime error: invalid ObjC cast, object is a '__NSCFNumber', but expected a 'NSString'
+ for (NSString *str in array) {
+NSLog(@"%@", str);
+ }
+
+ // The diagnostic should only be printed once.
+ // CHECK-NOT: runtime error
+
+ return 0;
+}
Index: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
===
--- compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
+++ compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
@@ -109,6 +109,7 @@
HANDLER(float_cast_overflow, "float-cast-overflow")
HANDLER(load_invalid_value, "load-invalid-value")
HANDLER(invalid_builtin, "invalid-builtin")
+HANDLER(invalid_objc_cast, "invalid-objc-cast")
HANDLER(function_type_mismatch, "function-type-mismatch")
HANDLER(implicit_conversion, "implicit-conversion")
HANDLER(nonnull_arg, "nonnull-arg")
Index: compiler-rt/lib/ubsan/ubsan_value.h
===
--- compiler-rt/lib/ubsan/ubsan_value.h
+++ compiler-rt/lib/ubsan/ubsan_value.h
@@ -135,6 +135,9 @@
/// \brief An opaque handle to a value.
typedef uptr ValueHandle;
+/// Returns the class name of the given ObjC object, or null if the name
+/// cannot be found.
+const char *getObjCClassName(ValueHandle Pointer);
/// \brief Representation of an operand value provided by the instrumented code.
///
Index: compiler-rt/lib/ubsan/ubsan_value.cpp
===
--- compiler-rt/lib/ubsan/ubsan_value.cpp
+++ compiler-rt/lib/ubsan/ubsan_value.cpp
@@ -16,9 +16,57 @@
#include "ubsan_value.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_mutex.h"
+
+// TODO(dliew): Prefer '__APPLE__' here over 'SANITIZER_MAC', as the latter is
+// unclear. rdar://58124919 tracks using a more obviously portable guard.
+#if defined(__APPLE__)
+#include
+#endif
using namespace __ubsan;
+typedef const char *(*ObjCGetClassNameTy)(void *);
+
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
+ // to introduce a static dependency from the ubsan runtime onto ObjC. Try to
+ // grab a handle to the ObjC runtime used by the process.
+ static bool AttemptedDlopen = false;
+ static void *ObjCHandle = nullptr;
+ static void *ObjCObjectGetClassName = nullptr;
+
+ // Prevent threads from racing to dlopen().
+ static __sanitizer::StaticSpinMutex Lock;
+ {
+__sanitizer::SpinMutexLock Guard();
+
+if (!AttemptedDlopen) {
+ ObjCHandle = dlopen(
+ "/usr/lib/libobjc.A.dylib",
+ RTLD_LAZY // Only bind symbols when used.
+ | RTLD_LOCAL // Only make symbols available via the handle.
+ | RTLD_NOLOAD // Do not load the dylib, just grab a handle if the
+// image is already loaded.
+ | RTLD_FIRST // Only search the image pointed-to by the handle.
+ );
+ AttemptedDlopen = true;
+ if (!ObjCHandle)
+return nullptr;
+ ObjCObjectGetClassName = dlsym(ObjCHandle, "object_getClassName");
+}
+ }
+
+ if (!ObjCObjectGetClassName)
+return nullptr;
+
+ return ObjCGetClassNameTy(ObjCObjectGetClassName)((void *)Pointer);
+#else
+ return nullptr;
+#endif
+}
+
SIntMax Value::getSIntValue() const {
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk added subscribers: erik.pilkington, ahatanak. vsk added a comment. + Erik and Akira for IRgen expertise. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
delcypher added a comment. @vsk The compiler-rt side seems fine to me but I'm not very familiar with the Clang side of things. @arphaman @jfb @rjmccall any thoughts? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk added a comment. Friendly ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk added inline comments.
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
delcypher wrote:
> vsk wrote:
> > delcypher wrote:
> > > vsk wrote:
> > > > delcypher wrote:
> > > > > The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined
> > > > > to be 1 if Apple otherwise it's 0) rather than `__APPLE__`.
> > > > I see. That seems problematic, as it makes it tricky to add
> > > > macOS-specific (or iOS-specific) functionality down the road. I don't
> > > > think SANITIZER_MAC should be defined unless TARGET_OS_MACOS is true.
> > > Sorry I should clarify. `SANITIZER_MAC` is poorly named but it is defined
> > > to be `1` for Apple platforms and `0`. I'm just pointing out the
> > > convention that exists today. You're absolutely right that we might want
> > > to do different things for different Apple platforms but I don't think we
> > > want to start doing a mass re-name until arm64e and arm64_32 support are
> > > completed landed in llvm's master.
> > I think 'SANITIZER_MAC' is confusing, and my preference would be to not use
> > it. `__APPLE__` seems clearer to me, and (IIUC) the plan is to replace
> > usage of 'SANITIZER_MAC' with it down the line anyway?
> There aren't any plans to do it right now but cleaning this up seems like a
> reasonable thing to do. If you take a look at
> `compiler-rt/lib/sanitizer_common/sanitizer_platform.h` you'll see that we
> actually have `SANITIZER_` for the other platforms that seems to be
> set as you'd expect. It's just `SANITIZER_MAC` that's badly named.
>
> I've filed a radar for this issue (rdar://problem/58124919). So if you prefer
> to use `__APPLE__` could you leave a comment with something like
>
> ```
> // TODO(dliew): Don't use unclear `SANITIZER_MAC` here. Instead wait for its
> replacement rdar://problem/58124919.
> ```
>
> Note the `TODO():` style is enforced by the sanitizer specific linter
> so you have to put a name there.
Thanks, done!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk updated this revision to Diff 238393.
vsk marked an inline comment as done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGObjC.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/CodeGenObjC/for-in.m
compiler-rt/lib/ubsan/ubsan_checks.inc
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_value.cpp
compiler-rt/lib/ubsan/ubsan_value.h
compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
Index: compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
@@ -0,0 +1,22 @@
+// REQUIRES: darwin
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast %s -O1 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast -fno-sanitize-recover=objc-cast %s -O1 -o %t.trap
+// RUN: not %run %t.trap 2>&1 | FileCheck %s
+
+#include
+
+int main() {
+ NSArray *array = [NSArray arrayWithObjects: @1, @2, @3, (void *)0];
+ // CHECK: objc-cast.m:[[@LINE+1]]:{{.*}}: runtime error: invalid ObjC cast, object is a '__NSCFNumber', but expected a 'NSString'
+ for (NSString *str in array) {
+NSLog(@"%@", str);
+ }
+
+ // The diagnostic should only be printed once.
+ // CHECK-NOT: runtime error
+
+ return 0;
+}
Index: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
===
--- compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
+++ compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
@@ -109,6 +109,7 @@
HANDLER(float_cast_overflow, "float-cast-overflow")
HANDLER(load_invalid_value, "load-invalid-value")
HANDLER(invalid_builtin, "invalid-builtin")
+HANDLER(invalid_objc_cast, "invalid-objc-cast")
HANDLER(function_type_mismatch, "function-type-mismatch")
HANDLER(implicit_conversion, "implicit-conversion")
HANDLER(nonnull_arg, "nonnull-arg")
Index: compiler-rt/lib/ubsan/ubsan_value.h
===
--- compiler-rt/lib/ubsan/ubsan_value.h
+++ compiler-rt/lib/ubsan/ubsan_value.h
@@ -135,6 +135,9 @@
/// \brief An opaque handle to a value.
typedef uptr ValueHandle;
+/// Returns the class name of the given ObjC object, or null if the name
+/// cannot be found.
+const char *getObjCClassName(ValueHandle Pointer);
/// \brief Representation of an operand value provided by the instrumented code.
///
Index: compiler-rt/lib/ubsan/ubsan_value.cpp
===
--- compiler-rt/lib/ubsan/ubsan_value.cpp
+++ compiler-rt/lib/ubsan/ubsan_value.cpp
@@ -16,9 +16,57 @@
#include "ubsan_value.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_mutex.h"
+
+// TODO(dliew): Prefer '__APPLE__' here over 'SANITIZER_MAC', as the latter is
+// unclear. rdar://58124919 tracks using a more obviously portable guard.
+#if defined(__APPLE__)
+#include
+#endif
using namespace __ubsan;
+typedef const char *(*ObjCGetClassNameTy)(void *);
+
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
+ // to introduce a static dependency from the ubsan runtime onto ObjC. Try to
+ // grab a handle to the ObjC runtime used by the process.
+ static bool AttemptedDlopen = false;
+ static void *ObjCHandle = nullptr;
+ static void *ObjCObjectGetClassName = nullptr;
+
+ // Prevent threads from racing to dlopen().
+ static __sanitizer::StaticSpinMutex Lock;
+ {
+__sanitizer::SpinMutexLock Guard();
+
+if (!AttemptedDlopen) {
+ ObjCHandle = dlopen(
+ "/usr/lib/libobjc.A.dylib",
+ RTLD_LAZY // Only bind symbols when used.
+ | RTLD_LOCAL // Only make symbols available via the handle.
+ | RTLD_NOLOAD // Do not load the dylib, just grab a handle if the
+// image is already loaded.
+ | RTLD_FIRST // Only search the image pointed-to by the handle.
+ );
+ AttemptedDlopen = true;
+ if (!ObjCHandle)
+return nullptr;
+ ObjCObjectGetClassName = dlsym(ObjCHandle, "object_getClassName");
+}
+ }
+
+ if (!ObjCObjectGetClassName)
+return nullptr;
+
+ return ObjCGetClassNameTy(ObjCObjectGetClassName)((void *)Pointer);
+#else
+ return nullptr;
+#endif
+}
+
SIntMax Value::getSIntValue() const {
CHECK(getType().isSignedIntegerTy());
if (isInlineInt()) {
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
delcypher added inline comments.
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
vsk wrote:
> delcypher wrote:
> > vsk wrote:
> > > delcypher wrote:
> > > > The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to
> > > > be 1 if Apple otherwise it's 0) rather than `__APPLE__`.
> > > I see. That seems problematic, as it makes it tricky to add
> > > macOS-specific (or iOS-specific) functionality down the road. I don't
> > > think SANITIZER_MAC should be defined unless TARGET_OS_MACOS is true.
> > Sorry I should clarify. `SANITIZER_MAC` is poorly named but it is defined
> > to be `1` for Apple platforms and `0`. I'm just pointing out the convention
> > that exists today. You're absolutely right that we might want to do
> > different things for different Apple platforms but I don't think we want to
> > start doing a mass re-name until arm64e and arm64_32 support are completed
> > landed in llvm's master.
> I think 'SANITIZER_MAC' is confusing, and my preference would be to not use
> it. `__APPLE__` seems clearer to me, and (IIUC) the plan is to replace usage
> of 'SANITIZER_MAC' with it down the line anyway?
There aren't any plans to do it right now but cleaning this up seems like a
reasonable thing to do. If you take a look at
`compiler-rt/lib/sanitizer_common/sanitizer_platform.h` you'll see that we
actually have `SANITIZER_` for the other platforms that seems to be
set as you'd expect. It's just `SANITIZER_MAC` that's badly named.
I've filed a radar for this issue (rdar://problem/58124919). So if you prefer
to use `__APPLE__` could you leave a comment with something like
```
// TODO(dliew): Don't use unclear `SANITIZER_MAC` here. Instead wait for its
replacement rdar://problem/58124919.
```
Note the `TODO():` style is enforced by the sanitizer specific linter so
you have to put a name there.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk added inline comments.
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
delcypher wrote:
> vsk wrote:
> > delcypher wrote:
> > > The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to
> > > be 1 if Apple otherwise it's 0) rather than `__APPLE__`.
> > I see. That seems problematic, as it makes it tricky to add macOS-specific
> > (or iOS-specific) functionality down the road. I don't think SANITIZER_MAC
> > should be defined unless TARGET_OS_MACOS is true.
> Sorry I should clarify. `SANITIZER_MAC` is poorly named but it is defined to
> be `1` for Apple platforms and `0`. I'm just pointing out the convention that
> exists today. You're absolutely right that we might want to do different
> things for different Apple platforms but I don't think we want to start doing
> a mass re-name until arm64e and arm64_32 support are completed landed in
> llvm's master.
I think 'SANITIZER_MAC' is confusing, and my preference would be to not use it.
`__APPLE__` seems clearer to me, and (IIUC) the plan is to replace usage of
'SANITIZER_MAC' with it down the line anyway?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
delcypher added inline comments.
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
vsk wrote:
> delcypher wrote:
> > The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to be
> > 1 if Apple otherwise it's 0) rather than `__APPLE__`.
> I see. That seems problematic, as it makes it tricky to add macOS-specific
> (or iOS-specific) functionality down the road. I don't think SANITIZER_MAC
> should be defined unless TARGET_OS_MACOS is true.
Sorry I should clarify. `SANITIZER_MAC` is poorly named but it is defined to be
`1` for Apple platforms and `0`. I'm just pointing out the convention that
exists today. You're absolutely right that we might want to do different things
for different Apple platforms but I don't think we want to start doing a mass
re-name until arm64e and arm64_32 support are completed landed in llvm's master.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk updated this revision to Diff 234142.
vsk added a comment.
Ignore an objc-cast report at a given SourceLocation after it's been reported
once.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGObjC.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/CodeGenObjC/for-in.m
compiler-rt/lib/ubsan/ubsan_checks.inc
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_value.cpp
compiler-rt/lib/ubsan/ubsan_value.h
compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
Index: compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
@@ -0,0 +1,22 @@
+// REQUIRES: darwin
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast %s -O1 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast -fno-sanitize-recover=objc-cast %s -O1 -o %t.trap
+// RUN: not %run %t.trap 2>&1 | FileCheck %s
+
+#include
+
+int main() {
+ NSArray *array = [NSArray arrayWithObjects: @1, @2, @3, (void *)0];
+ // CHECK: objc-cast.m:[[@LINE+1]]:{{.*}}: runtime error: invalid ObjC cast, object is a '__NSCFNumber', but expected a 'NSString'
+ for (NSString *str in array) {
+NSLog(@"%@", str);
+ }
+
+ // The diagnostic should only be printed once.
+ // CHECK-NOT: runtime error
+
+ return 0;
+}
Index: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
===
--- compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
+++ compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
@@ -109,6 +109,7 @@
HANDLER(float_cast_overflow, "float-cast-overflow")
HANDLER(load_invalid_value, "load-invalid-value")
HANDLER(invalid_builtin, "invalid-builtin")
+HANDLER(invalid_objc_cast, "invalid-objc-cast")
HANDLER(function_type_mismatch, "function-type-mismatch")
HANDLER(implicit_conversion, "implicit-conversion")
HANDLER(nonnull_arg, "nonnull-arg")
Index: compiler-rt/lib/ubsan/ubsan_value.h
===
--- compiler-rt/lib/ubsan/ubsan_value.h
+++ compiler-rt/lib/ubsan/ubsan_value.h
@@ -135,6 +135,9 @@
/// \brief An opaque handle to a value.
typedef uptr ValueHandle;
+/// Returns the class name of the given ObjC object, or null if the name
+/// cannot be found.
+const char *getObjCClassName(ValueHandle Pointer);
/// \brief Representation of an operand value provided by the instrumented code.
///
Index: compiler-rt/lib/ubsan/ubsan_value.cpp
===
--- compiler-rt/lib/ubsan/ubsan_value.cpp
+++ compiler-rt/lib/ubsan/ubsan_value.cpp
@@ -16,9 +16,55 @@
#include "ubsan_value.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_mutex.h"
+
+#if defined(__APPLE__)
+#include
+#endif
using namespace __ubsan;
+typedef const char *(*ObjCGetClassNameTy)(void *);
+
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
+ // to introduce a static dependency from the ubsan runtime onto ObjC. Try to
+ // grab a handle to the ObjC runtime used by the process.
+ static bool AttemptedDlopen = false;
+ static void *ObjCHandle = nullptr;
+ static void *ObjCObjectGetClassName = nullptr;
+
+ // Prevent threads from racing to dlopen().
+ static __sanitizer::StaticSpinMutex Lock;
+ {
+__sanitizer::SpinMutexLock Guard();
+
+if (!AttemptedDlopen) {
+ ObjCHandle = dlopen(
+ "/usr/lib/libobjc.A.dylib",
+ RTLD_LAZY // Only bind symbols when used.
+ | RTLD_LOCAL // Only make symbols available via the handle.
+ | RTLD_NOLOAD // Do not load the dylib, just grab a handle if the
+// image is already loaded.
+ | RTLD_FIRST // Only search the image pointed-to by the handle.
+ );
+ AttemptedDlopen = true;
+ if (!ObjCHandle)
+return nullptr;
+ ObjCObjectGetClassName = dlsym(ObjCHandle, "object_getClassName");
+}
+ }
+
+ if (!ObjCObjectGetClassName)
+return nullptr;
+
+ return ObjCGetClassNameTy(ObjCObjectGetClassName)((void *)Pointer);
+#else
+ return nullptr;
+#endif
+}
+
SIntMax Value::getSIntValue() const {
CHECK(getType().isSignedIntegerTy());
if (isInlineInt()) {
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
===
---
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk updated this revision to Diff 234139.
vsk added a comment.
Avoid a static initializer.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGObjC.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/CodeGenObjC/for-in.m
compiler-rt/lib/ubsan/ubsan_checks.inc
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_value.cpp
compiler-rt/lib/ubsan/ubsan_value.h
compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
Index: compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
@@ -0,0 +1,18 @@
+// REQUIRES: darwin
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast %s -O1 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast -fno-sanitize-recover=objc-cast %s -O1 -o %t.trap
+// RUN: not %run %t.trap 2>&1 | FileCheck %s
+
+#include
+
+int main() {
+ NSArray *array = [NSArray arrayWithObjects: @1, @2, @3, (void *)0];
+ // CHECK: objc-cast.m:[[@LINE+1]]:{{.*}}: runtime error: invalid ObjC cast, object is a '__NSCFNumber', but expected a 'NSString'
+ for (NSString *str in array) {
+NSLog(@"%@", str);
+ }
+ return 0;
+}
Index: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
===
--- compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
+++ compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
@@ -109,6 +109,7 @@
HANDLER(float_cast_overflow, "float-cast-overflow")
HANDLER(load_invalid_value, "load-invalid-value")
HANDLER(invalid_builtin, "invalid-builtin")
+HANDLER(invalid_objc_cast, "invalid-objc-cast")
HANDLER(function_type_mismatch, "function-type-mismatch")
HANDLER(implicit_conversion, "implicit-conversion")
HANDLER(nonnull_arg, "nonnull-arg")
Index: compiler-rt/lib/ubsan/ubsan_value.h
===
--- compiler-rt/lib/ubsan/ubsan_value.h
+++ compiler-rt/lib/ubsan/ubsan_value.h
@@ -135,6 +135,9 @@
/// \brief An opaque handle to a value.
typedef uptr ValueHandle;
+/// Returns the class name of the given ObjC object, or null if the name
+/// cannot be found.
+const char *getObjCClassName(ValueHandle Pointer);
/// \brief Representation of an operand value provided by the instrumented code.
///
Index: compiler-rt/lib/ubsan/ubsan_value.cpp
===
--- compiler-rt/lib/ubsan/ubsan_value.cpp
+++ compiler-rt/lib/ubsan/ubsan_value.cpp
@@ -16,9 +16,55 @@
#include "ubsan_value.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_mutex.h"
+
+#if defined(__APPLE__)
+#include
+#endif
using namespace __ubsan;
+typedef const char *(*ObjCGetClassNameTy)(void *);
+
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
+ // to introduce a static dependency from the ubsan runtime onto ObjC. Try to
+ // grab a handle to the ObjC runtime used by the process.
+ static bool AttemptedDlopen = false;
+ static void *ObjCHandle = nullptr;
+ static void *ObjCObjectGetClassName = nullptr;
+
+ // Prevent threads from racing to dlopen().
+ static __sanitizer::StaticSpinMutex Lock;
+ {
+__sanitizer::SpinMutexLock Guard();
+
+if (!AttemptedDlopen) {
+ ObjCHandle = dlopen(
+ "/usr/lib/libobjc.A.dylib",
+ RTLD_LAZY // Only bind symbols when used.
+ | RTLD_LOCAL // Only make symbols available via the handle.
+ | RTLD_NOLOAD // Do not load the dylib, just grab a handle if the
+// image is already loaded.
+ | RTLD_FIRST // Only search the image pointed-to by the handle.
+ );
+ AttemptedDlopen = true;
+ if (!ObjCHandle)
+return nullptr;
+ ObjCObjectGetClassName = dlsym(ObjCHandle, "object_getClassName");
+}
+ }
+
+ if (!ObjCObjectGetClassName)
+return nullptr;
+
+ return ObjCGetClassNameTy(ObjCObjectGetClassName)((void *)Pointer);
+#else
+ return nullptr;
+#endif
+}
+
SIntMax Value::getSIntValue() const {
CHECK(getType().isSignedIntegerTy());
if (isInlineInt()) {
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
===
--- compiler-rt/lib/ubsan/ubsan_handlers.h
+++ compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -168,6 +168,14 @@
/// Handle a builtin called in an invalid way.
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk added inline comments.
Comment at: clang/lib/Driver/ToolChain.cpp:953
+ SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+ SanitizerKind::LocalBounds | SanitizerKind::ObjCCast;
if (getTriple().getArch() == llvm::Triple::x86 ||
delcypher wrote:
> `SanitizerKind::ObjCCast` doesn't seem to fit the comment above. It is
> platform dependent (only really works for Apple platforms) and it **does**
> require runtime support (i.e. the Objective-C runtime).
The runtime dependency is optional, and there's nothing inherently
Apple-specific about this check. However, perhaps it's best not to
inadvertently advertise support for the GNU objc runtime before it's in tree.
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
delcypher wrote:
> The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to be 1
> if Apple otherwise it's 0) rather than `__APPLE__`.
I see. That seems problematic, as it makes it tricky to add macOS-specific (or
iOS-specific) functionality down the road. I don't think SANITIZER_MAC should
be defined unless TARGET_OS_MACOS is true.
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:37
+
+ if (!AttemptedDlopen) {
+ObjCHandle = dlopen(
delcypher wrote:
> You might want some sort of lock here (or atomic variable) to ensure we don't
> race and try to `dlopen()` multiple times.
Yes, thanks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk updated this revision to Diff 234136.
vsk marked 3 inline comments as done.
vsk added a comment.
Address review feedback.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGObjC.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/CodeGenObjC/for-in.m
compiler-rt/lib/ubsan/ubsan_checks.inc
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_value.cpp
compiler-rt/lib/ubsan/ubsan_value.h
compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
Index: compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
@@ -0,0 +1,18 @@
+// REQUIRES: darwin
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast %s -O1 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast -fno-sanitize-recover=objc-cast %s -O1 -o %t.trap
+// RUN: not %run %t.trap 2>&1 | FileCheck %s
+
+#include
+
+int main() {
+ NSArray *array = [NSArray arrayWithObjects: @1, @2, @3, (void *)0];
+ // CHECK: objc-cast.m:[[@LINE+1]]:{{.*}}: runtime error: invalid ObjC cast, object is a '__NSCFNumber', but expected a 'NSString'
+ for (NSString *str in array) {
+NSLog(@"%@", str);
+ }
+ return 0;
+}
Index: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
===
--- compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
+++ compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
@@ -109,6 +109,7 @@
HANDLER(float_cast_overflow, "float-cast-overflow")
HANDLER(load_invalid_value, "load-invalid-value")
HANDLER(invalid_builtin, "invalid-builtin")
+HANDLER(invalid_objc_cast, "invalid-objc-cast")
HANDLER(function_type_mismatch, "function-type-mismatch")
HANDLER(implicit_conversion, "implicit-conversion")
HANDLER(nonnull_arg, "nonnull-arg")
Index: compiler-rt/lib/ubsan/ubsan_value.h
===
--- compiler-rt/lib/ubsan/ubsan_value.h
+++ compiler-rt/lib/ubsan/ubsan_value.h
@@ -135,6 +135,9 @@
/// \brief An opaque handle to a value.
typedef uptr ValueHandle;
+/// Returns the class name of the given ObjC object, or null if the name
+/// cannot be found.
+const char *getObjCClassName(ValueHandle Pointer);
/// \brief Representation of an operand value provided by the instrumented code.
///
Index: compiler-rt/lib/ubsan/ubsan_value.cpp
===
--- compiler-rt/lib/ubsan/ubsan_value.cpp
+++ compiler-rt/lib/ubsan/ubsan_value.cpp
@@ -16,9 +16,55 @@
#include "ubsan_value.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
+#include "sanitizer_common/sanitizer_mutex.h"
+
+#if defined(__APPLE__)
+#include
+#endif
using namespace __ubsan;
+typedef const char *(*ObjCGetClassNameTy)(void *);
+
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
+ // to introduce a static dependency from the ubsan runtime onto ObjC. Try to
+ // grab a handle to the ObjC runtime used by the process.
+ static bool AttemptedDlopen = false;
+ static void *ObjCHandle = nullptr;
+ static void *ObjCObjectGetClassName = nullptr;
+
+ // Prevent threads from racing to dlopen().
+ static __sanitizer::SpinMutex Lock;
+ {
+__sanitizer::GenericScopedLock<__sanitizer::SpinMutex> Guard();
+
+if (!AttemptedDlopen) {
+ ObjCHandle = dlopen(
+ "/usr/lib/libobjc.A.dylib",
+ RTLD_LAZY // Only bind symbols when used.
+ | RTLD_LOCAL // Only make symbols available via the handle.
+ | RTLD_NOLOAD // Do not load the dylib, just grab a handle if the
+// image is already loaded.
+ | RTLD_FIRST // Only search the image pointed-to by the handle.
+ );
+ AttemptedDlopen = true;
+ if (!ObjCHandle)
+return nullptr;
+ ObjCObjectGetClassName = dlsym(ObjCHandle, "object_getClassName");
+}
+ }
+
+ if (!ObjCObjectGetClassName)
+return nullptr;
+
+ return ObjCGetClassNameTy(ObjCObjectGetClassName)((void *)Pointer);
+#else
+ return nullptr;
+#endif
+}
+
SIntMax Value::getSIntValue() const {
CHECK(getType().isSignedIntegerTy());
if (isInlineInt()) {
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
===
--- compiler-rt/lib/ubsan/ubsan_handlers.h
+++ compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -168,6 +168,14
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
delcypher added inline comments. Comment at: clang/lib/Driver/ToolChain.cpp:953 + SanitizerKind::ImplicitConversion | SanitizerKind::Nullability | + SanitizerKind::LocalBounds | SanitizerKind::ObjCCast; if (getTriple().getArch() == llvm::Triple::x86 || `SanitizerKind::ObjCCast` doesn't seem to fit the comment above. It is platform dependent (only really works for Apple platforms) and it **does** require runtime support (i.e. the Objective-C runtime). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71491/new/ https://reviews.llvm.org/D71491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
delcypher added inline comments.
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to be 1 if
Apple otherwise it's 0) rather than `__APPLE__`.
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:37
+
+ if (!AttemptedDlopen) {
+ObjCHandle = dlopen(
You might want some sort of lock here (or atomic variable) to ensure we don't
race and try to `dlopen()` multiple times.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements
vsk created this revision.
vsk added reviewers: rjmccall, jfb, arphaman, delcypher.
Herald added subscribers: llvm-commits, dexonsmith.
Herald added a project: LLVM.
Check that the implicit cast from `id` used to construct the element
variable in an ObjC for-in statement is valid.
This check is included as part of a new `objc-cast` sanitizer, outside
of the main 'undefined' group, as (IIUC) the behavior it's checking for
is not technically UB.
The check can be extended to cover other kinds of invalid casts in ObjC.
Partially addresses: rdar://12903059, rdar://9542496
https://reviews.llvm.org/D71491
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGObjC.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Driver/SanitizerArgs.cpp
clang/lib/Driver/ToolChain.cpp
clang/test/CodeGenObjC/for-in.m
compiler-rt/lib/ubsan/ubsan_checks.inc
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/lib/ubsan/ubsan_value.cpp
compiler-rt/lib/ubsan/ubsan_value.h
compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
Index: compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/objc-cast.m
@@ -0,0 +1,18 @@
+// REQUIRES: darwin
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast %s -O1 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+//
+// RUN: %clang -framework Foundation -fsanitize=objc-cast -fno-sanitize-recover=objc-cast %s -O1 -o %t.trap
+// RUN: not %run %t.trap 2>&1 | FileCheck %s
+
+#include
+
+int main() {
+ NSArray *array = [NSArray arrayWithObjects: @1, @2, @3, (void *)0];
+ // CHECK: objc-cast.m:[[@LINE+1]]:{{.*}}: runtime error: invalid ObjC cast, object is a '__NSCFNumber', but expected a 'NSString'
+ for (NSString *str in array) {
+NSLog(@"%@", str);
+ }
+ return 0;
+}
Index: compiler-rt/lib/ubsan/ubsan_value.h
===
--- compiler-rt/lib/ubsan/ubsan_value.h
+++ compiler-rt/lib/ubsan/ubsan_value.h
@@ -135,6 +135,9 @@
/// \brief An opaque handle to a value.
typedef uptr ValueHandle;
+/// Returns the class name of the given ObjC object, or null if the name
+/// cannot be found.
+const char *getObjCClassName(ValueHandle Pointer);
/// \brief Representation of an operand value provided by the instrumented code.
///
Index: compiler-rt/lib/ubsan/ubsan_value.cpp
===
--- compiler-rt/lib/ubsan/ubsan_value.cpp
+++ compiler-rt/lib/ubsan/ubsan_value.cpp
@@ -17,8 +17,46 @@
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
+#if defined(__APPLE__)
+#include
+#endif
+
using namespace __ubsan;
+typedef const char *(*ObjCGetClassNameTy)(void *);
+
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
+ // to introduce a static dependency from the ubsan runtime onto ObjC. Try to
+ // grab a handle to the ObjC runtime used by the process.
+ static bool AttemptedDlopen = false;
+ static void *ObjCHandle = nullptr;
+ static void *ObjCObjectGetClassName = nullptr;
+
+ if (!AttemptedDlopen) {
+ObjCHandle = dlopen(
+"/usr/lib/libobjc.A.dylib",
+RTLD_LAZY /* Only bind symbols when used. */
+| RTLD_LOCAL /* Only make symbols available via the handle. */
+| RTLD_NOLOAD /* Do not load the dylib, just grab a handle if the
+ image is already loaded. */
+| RTLD_FIRST /* Only search the image pointed-to by the handle. */);
+AttemptedDlopen = true;
+if (!ObjCHandle)
+ return nullptr;
+ObjCObjectGetClassName = dlsym(ObjCHandle, "object_getClassName");
+ }
+
+ if (!ObjCObjectGetClassName)
+return nullptr;
+
+ return ObjCGetClassNameTy(ObjCObjectGetClassName)((void *)Pointer);
+#else
+ return nullptr;
+#endif
+}
+
SIntMax Value::getSIntValue() const {
CHECK(getType().isSignedIntegerTy());
if (isInlineInt()) {
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
===
--- compiler-rt/lib/ubsan/ubsan_handlers.h
+++ compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -168,6 +168,14 @@
/// Handle a builtin called in an invalid way.
RECOVERABLE(invalid_builtin, InvalidBuiltinData *Data)
+struct InvalidObjCCast {
+ SourceLocation Loc;
+ const TypeDescriptor
+};
+
+/// Handle an invalid ObjC cast.
+RECOVERABLE(invalid_objc_cast, InvalidObjCCast *Data, ValueHandle Pointer)
+
struct NonNullReturnData {
SourceLocation AttrLoc;
};
Index: compiler-rt/lib/ubsan/ubsan_handlers.cpp
===
--- compiler-rt/lib/ubsan/ubsan_handlers.cpp
+++ compiler-rt/lib/ubsan/ubsan_handlers.cpp
