[Clang] Add diagnostic about "%P" specifier with Objective-C pointers #89977
Conversation
…pointers (llvm#89968) A Darwin extension '%P' combined with an Objective-C pointer seems to always be a bug. '%P' will dump bytes at the pointed-to address (in contrast to '%p' which dumps the pointer itself). This extension is only allowed in "OS Log" contexts and is intended to be used like `%{uuid_t}.*16P` or `%{timeval}.*P`. If an ObjC pointer is used, then the internal runtime structure (aka, the is-a pointer and other runtime metadata) will be dumped, which (IMO) is never the expectation. A simple diagnostic can help flag these scenarios.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Jared Grubb (jaredgrubb) ChangesA Darwin extension '%P' combined with an Objective-C pointer seems to always be a bug. '%P' will dump bytes at the pointed-to address (in contrast to '%p' which dumps the pointer itself). This extension is only allowed in "OS Log" contexts and is intended to be used like A simple diagnostic can help flag these scenarios. Full diff: https://github.com/llvm/llvm-project/pull/89977.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 6732a1a98452ad..d7de9cf2c86e0e 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9901,6 +9901,9 @@ def warn_format_invalid_annotation : Warning<
def warn_format_P_no_precision : Warning<
"using '%%P' format specifier without precision">,
InGroup<Format>;
+def warn_format_P_with_objc_pointer : Warning<
+ "using '%%P' format specifier with an Objective-C pointer results in dumping runtime object structure, not object value">,
+ InGroup<Format>;
def warn_printf_ignored_flag: Warning<
"flag '%0' is ignored when flag '%1' is present">,
InGroup<Format>;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 67132701b41cfd..53dbd58b0252a2 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -12544,6 +12544,17 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
return true;
}
+ // Diagnose attempts to use '%P' with ObjC object types, which will result in
+ // dumping raw class data (like is-a pointer), not actual data.
+ if (FS.getConversionSpecifier().getKind() == ConversionSpecifier::PArg &&
+ ExprTy->isObjCObjectPointerType()) {
+ const CharSourceRange &CSR =
+ getSpecifierRange(StartSpecifier, SpecifierLen);
+ EmitFormatDiagnostic(S.PDiag(diag::warn_format_P_with_objc_pointer),
+ E->getExprLoc(), false, CSR);
+ return true;
+ }
+
ArgType::MatchKind ImplicitMatch = ArgType::NoMatch;
ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy);
ArgType::MatchKind OrigMatch = Match;
diff --git a/clang/test/SemaObjC/format-strings-oslog.m b/clang/test/SemaObjC/format-strings-oslog.m
index 20fec93b653bd5..af5aef3d617954 100644
--- a/clang/test/SemaObjC/format-strings-oslog.m
+++ b/clang/test/SemaObjC/format-strings-oslog.m
@@ -44,15 +44,18 @@ void test_os_log_format(const char *pc, int i, void *p, void *buf) {
}
// Test os_log_format primitive with ObjC string literal format argument.
-void test_objc(const char *pc, int i, void *p, void *buf, NSString *nss) {
+void test_objc(const char *pc, int i, void *p, void *buf, NSString *nss, id obj) {
__builtin_os_log_format(buf, @"");
__builtin_os_log_format(buf, @"%d"); // expected-warning {{more '%' conversions than data arguments}}
__builtin_os_log_format(buf, @"%d", i);
+
__builtin_os_log_format(buf, @"%P", p); // expected-warning {{using '%P' format specifier without precision}}
__builtin_os_log_format(buf, @"%.10P", p);
__builtin_os_log_format(buf, @"%.*P", p); // expected-warning {{field precision should have type 'int', but argument has type 'void *'}}
__builtin_os_log_format(buf, @"%.*P", i, p);
__builtin_os_log_format(buf, @"%.*P", i, i); // expected-warning {{format specifies type 'void *' but the argument has type 'int'}}
+ __builtin_os_log_format(buf, @"%.8P", nss); // expected-warning {{using '%P' format specifier with an Objective-C pointer results in dumping runtime object structure, not object value}}
+ __builtin_os_log_format(buf, @"%.*P", i, obj); // expected-warning {{using '%P' format specifier with an Objective-C pointer results in dumping runtime object structure, not object value}}
__builtin_os_log_format(buf, @"%{private}s", pc);
__builtin_os_log_format(buf, @"%@", nss);
|
|
Cc: @ahatanaka @egorzhdan |
| __builtin_os_log_format(buf, @"%P", p); // expected-warning {{using '%P' format specifier without precision}} | ||
| __builtin_os_log_format(buf, @"%.10P", p); | ||
| __builtin_os_log_format(buf, @"%.*P", p); // expected-warning {{field precision should have type 'int', but argument has type 'void *'}} | ||
| __builtin_os_log_format(buf, @"%.*P", i, p); | ||
| __builtin_os_log_format(buf, @"%.*P", i, i); // expected-warning {{format specifies type 'void *' but the argument has type 'int'}} | ||
| __builtin_os_log_format(buf, @"%.8P", nss); // expected-warning {{using '%P' format specifier with an Objective-C pointer results in dumping runtime object structure, not object value}} |
There was a problem hiding this comment.
Should it warn when (void *)nss is passed too?
There was a problem hiding this comment.
I don't think so, but I can be convinced. IMO, using a (void*) cast could be the escape-hatch if someone actually really does want to dump the ObjC runtime structure but also squash this warning.
egorzhdan
changed the title
|
@jaredgrubb Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
A Darwin extension '%P' combined with an Objective-C pointer seems to always be a bug.
'%P' will dump bytes at the pointed-to address (in contrast to '%p' which dumps the pointer itself). This extension is only allowed in "OS Log" contexts and is intended to be used like
%{uuid_t}.*16Por%{timeval}.*P. If an ObjC pointer is used, then the internal runtime structure (aka, the is-a pointer and other runtime metadata) will be dumped, which (IMO) is never the expectation.A simple diagnostic can help flag these scenarios.
Resolves #89968