From 2100dca9ad30277bbdb53384358245fd1566a9cb Mon Sep 17 00:00:00 2001 From: Akira Hatanaka Date: Fri, 5 Sep 2025 07:01:00 -0700 Subject: [PATCH 1/2] [os_log] Fix a CodeGen crash that occurs when arguments of struct, class, or complex types are passed to _builtin_os_log_format This change fixes a crash in clang's CodeGen by ensuring that those arguments are ignored. rdar://139824423 --- clang/lib/AST/OSLog.cpp | 48 ++++++++++++++++++++++----------- clang/test/CodeGenObjC/os_log.m | 23 ++++++++++++++++ 2 files changed, 55 insertions(+), 16 deletions(-) diff --git a/clang/lib/AST/OSLog.cpp b/clang/lib/AST/OSLog.cpp index 91f8410e89e86..d228d297dd2ca 100644 --- a/clang/lib/AST/OSLog.cpp +++ b/clang/lib/AST/OSLog.cpp @@ -70,22 +70,31 @@ class OSLogFormatStringHandler bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS, const char *StartSpecifier, unsigned SpecifierLen, const TargetInfo &) override { + // Set the argument expression. Arguments of struct/class/complex types are + // ignored. + auto CheckAndSetArgExpr = [&](unsigned Idx, auto &ArgE) { + const Expr *E = Args[Idx]; + if (E && (E->getType()->isRecordType() || E->getType()->isComplexType())) + return false; + ArgE = E; + return true; + }; + if (!FS.consumesDataArgument() && FS.getConversionSpecifier().getKind() != clang::analyze_format_string::ConversionSpecifier::PrintErrno) return true; - ArgsData.emplace_back(); + ArgData ArgD; unsigned ArgIndex = FS.getArgIndex(); if (ArgIndex < Args.size()) - ArgsData.back().E = Args[ArgIndex]; + if (!CheckAndSetArgExpr(ArgIndex, ArgD.E)) + return true; // First get the Kind - ArgsData.back().Kind = getKind(FS.getConversionSpecifier().getKind()); - if (ArgsData.back().Kind != OSLogBufferItem::ErrnoKind && - !ArgsData.back().E) { + ArgD.Kind = getKind(FS.getConversionSpecifier().getKind()); + if (ArgD.Kind != OSLogBufferItem::ErrnoKind && !ArgD.E) { // missing argument - ArgsData.pop_back(); return false; } @@ -97,10 +106,11 @@ class OSLogFormatStringHandler case clang::analyze_format_string::OptionalAmount::NotSpecified: // "%s" break; case clang::analyze_format_string::OptionalAmount::Constant: // "%.16s" - ArgsData.back().Size = precision.getConstantAmount(); + ArgD.Size = precision.getConstantAmount(); break; case clang::analyze_format_string::OptionalAmount::Arg: // "%.*s" - ArgsData.back().Count = Args[precision.getArgIndex()]; + if (!CheckAndSetArgExpr(precision.getArgIndex(), ArgD.Count)) + return true; break; case clang::analyze_format_string::OptionalAmount::Invalid: return false; @@ -113,10 +123,11 @@ class OSLogFormatStringHandler case clang::analyze_format_string::OptionalAmount::NotSpecified: // "%P" return false; // length must be supplied with pointer format specifier case clang::analyze_format_string::OptionalAmount::Constant: // "%.16P" - ArgsData.back().Size = precision.getConstantAmount(); + ArgD.Size = precision.getConstantAmount(); break; case clang::analyze_format_string::OptionalAmount::Arg: // "%.*P" - ArgsData.back().Count = Args[precision.getArgIndex()]; + if (!CheckAndSetArgExpr(precision.getArgIndex(), ArgD.Count)) + return true; break; case clang::analyze_format_string::OptionalAmount::Invalid: return false; @@ -125,22 +136,27 @@ class OSLogFormatStringHandler } default: if (FS.getPrecision().hasDataArgument()) { - ArgsData.back().Precision = Args[FS.getPrecision().getArgIndex()]; + if (!CheckAndSetArgExpr(FS.getPrecision().getArgIndex(), + ArgD.Precision)) + return true; } break; } if (FS.getFieldWidth().hasDataArgument()) { - ArgsData.back().FieldWidth = Args[FS.getFieldWidth().getArgIndex()]; + if (!CheckAndSetArgExpr(FS.getFieldWidth().getArgIndex(), + ArgD.FieldWidth)) + return true; } if (FS.isSensitive()) - ArgsData.back().Flags |= OSLogBufferItem::IsSensitive; + ArgD.Flags |= OSLogBufferItem::IsSensitive; else if (FS.isPrivate()) - ArgsData.back().Flags |= OSLogBufferItem::IsPrivate; + ArgD.Flags |= OSLogBufferItem::IsPrivate; else if (FS.isPublic()) - ArgsData.back().Flags |= OSLogBufferItem::IsPublic; + ArgD.Flags |= OSLogBufferItem::IsPublic; - ArgsData.back().MaskType = FS.getMaskType(); + ArgD.MaskType = FS.getMaskType(); + ArgsData.push_back(ArgD); return true; } diff --git a/clang/test/CodeGenObjC/os_log.m b/clang/test/CodeGenObjC/os_log.m index 837883ec4bb75..00229a746304c 100644 --- a/clang/test/CodeGenObjC/os_log.m +++ b/clang/test/CodeGenObjC/os_log.m @@ -13,6 +13,13 @@ + (id)m1; C *c; +struct S { + int a[4]; +}; + +struct S s; +_Complex float cf; + @class NSString; extern __attribute__((visibility("default"))) NSString *GenString(void); void os_log_pack_send(void *); @@ -123,3 +130,19 @@ void test_builtin_os_log5(void *buf) { __builtin_os_log_format(buf, "capabilities: %@", (0, GenString())); os_log_pack_send(buf); } + +// CHECK-LABEL: define void @test_builtin_os_log6( +// CHECK: call void @__os_log_helper_1_0_0( + +void test_builtin_os_log6(void *buf) { + __builtin_os_log_format(buf, "%.*s %.*P %*.*f", s, s, s, s, s, s, s); +} + +// CHECK-LABEL: define linkonce_odr hidden void @__os_log_helper_1_0_0( + +// CHECK-LABEL: define void @test_builtin_os_log7( +// CHECK: call void @__os_log_helper_1_0_0( + +void test_builtin_os_log7(void *buf) { + __builtin_os_log_format(buf, "%.*s %.*P %*.*f", cf, cf, cf, cf, cf, cf, cf); +} From d82fed8f2f00d14214d14e430f2ea6add994b8b2 Mon Sep 17 00:00:00 2001 From: Akira Hatanaka Date: Mon, 22 Sep 2025 19:16:29 -0700 Subject: [PATCH 2/2] Error out if struct or complex type arguments are passed --- .../clang/Basic/DiagnosticSemaKinds.td | 5 ++ clang/lib/AST/OSLog.cpp | 48 +++++++------------ clang/lib/Sema/SemaChecking.cpp | 23 +++++++-- clang/test/CodeGenObjC/os_log.m | 23 --------- clang/test/SemaObjC/os_log.m | 22 +++++++++ 5 files changed, 61 insertions(+), 60 deletions(-) create mode 100644 clang/test/SemaObjC/os_log.m diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index bd896524321d1..506adad5b2c47 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -10448,6 +10448,9 @@ def warn_format_conversion_argument_type_mismatch : Warning< "format specifies type %0 but the argument has " "%select{type|underlying type}2 %1">, InGroup; +def err_format_conversion_argument_type_mismatch : Error< + "format specifies type %0 but the argument has " + "%select{type|underlying type}2 %1">; def warn_format_conversion_argument_type_mismatch_pedantic : Extension< warn_format_conversion_argument_type_mismatch.Summary>, InGroup; @@ -10497,6 +10500,8 @@ def warn_printf_asterisk_missing_arg : Warning< def warn_printf_asterisk_wrong_type : Warning< "field %select{width|precision}0 should have type %1, but argument has type %2">, InGroup; +def err_printf_asterisk_wrong_type : Error< + "field %select{width|precision}0 should have type %1, but argument has type %2">; def warn_printf_nonsensical_optional_amount: Warning< "%select{field width|precision}0 used with '%1' conversion specifier, resulting in undefined behavior">, InGroup; diff --git a/clang/lib/AST/OSLog.cpp b/clang/lib/AST/OSLog.cpp index d228d297dd2ca..91f8410e89e86 100644 --- a/clang/lib/AST/OSLog.cpp +++ b/clang/lib/AST/OSLog.cpp @@ -70,31 +70,22 @@ class OSLogFormatStringHandler bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS, const char *StartSpecifier, unsigned SpecifierLen, const TargetInfo &) override { - // Set the argument expression. Arguments of struct/class/complex types are - // ignored. - auto CheckAndSetArgExpr = [&](unsigned Idx, auto &ArgE) { - const Expr *E = Args[Idx]; - if (E && (E->getType()->isRecordType() || E->getType()->isComplexType())) - return false; - ArgE = E; - return true; - }; - if (!FS.consumesDataArgument() && FS.getConversionSpecifier().getKind() != clang::analyze_format_string::ConversionSpecifier::PrintErrno) return true; - ArgData ArgD; + ArgsData.emplace_back(); unsigned ArgIndex = FS.getArgIndex(); if (ArgIndex < Args.size()) - if (!CheckAndSetArgExpr(ArgIndex, ArgD.E)) - return true; + ArgsData.back().E = Args[ArgIndex]; // First get the Kind - ArgD.Kind = getKind(FS.getConversionSpecifier().getKind()); - if (ArgD.Kind != OSLogBufferItem::ErrnoKind && !ArgD.E) { + ArgsData.back().Kind = getKind(FS.getConversionSpecifier().getKind()); + if (ArgsData.back().Kind != OSLogBufferItem::ErrnoKind && + !ArgsData.back().E) { // missing argument + ArgsData.pop_back(); return false; } @@ -106,11 +97,10 @@ class OSLogFormatStringHandler case clang::analyze_format_string::OptionalAmount::NotSpecified: // "%s" break; case clang::analyze_format_string::OptionalAmount::Constant: // "%.16s" - ArgD.Size = precision.getConstantAmount(); + ArgsData.back().Size = precision.getConstantAmount(); break; case clang::analyze_format_string::OptionalAmount::Arg: // "%.*s" - if (!CheckAndSetArgExpr(precision.getArgIndex(), ArgD.Count)) - return true; + ArgsData.back().Count = Args[precision.getArgIndex()]; break; case clang::analyze_format_string::OptionalAmount::Invalid: return false; @@ -123,11 +113,10 @@ class OSLogFormatStringHandler case clang::analyze_format_string::OptionalAmount::NotSpecified: // "%P" return false; // length must be supplied with pointer format specifier case clang::analyze_format_string::OptionalAmount::Constant: // "%.16P" - ArgD.Size = precision.getConstantAmount(); + ArgsData.back().Size = precision.getConstantAmount(); break; case clang::analyze_format_string::OptionalAmount::Arg: // "%.*P" - if (!CheckAndSetArgExpr(precision.getArgIndex(), ArgD.Count)) - return true; + ArgsData.back().Count = Args[precision.getArgIndex()]; break; case clang::analyze_format_string::OptionalAmount::Invalid: return false; @@ -136,27 +125,22 @@ class OSLogFormatStringHandler } default: if (FS.getPrecision().hasDataArgument()) { - if (!CheckAndSetArgExpr(FS.getPrecision().getArgIndex(), - ArgD.Precision)) - return true; + ArgsData.back().Precision = Args[FS.getPrecision().getArgIndex()]; } break; } if (FS.getFieldWidth().hasDataArgument()) { - if (!CheckAndSetArgExpr(FS.getFieldWidth().getArgIndex(), - ArgD.FieldWidth)) - return true; + ArgsData.back().FieldWidth = Args[FS.getFieldWidth().getArgIndex()]; } if (FS.isSensitive()) - ArgD.Flags |= OSLogBufferItem::IsSensitive; + ArgsData.back().Flags |= OSLogBufferItem::IsSensitive; else if (FS.isPrivate()) - ArgD.Flags |= OSLogBufferItem::IsPrivate; + ArgsData.back().Flags |= OSLogBufferItem::IsPrivate; else if (FS.isPublic()) - ArgD.Flags |= OSLogBufferItem::IsPublic; + ArgsData.back().Flags |= OSLogBufferItem::IsPublic; - ArgD.MaskType = FS.getMaskType(); - ArgsData.push_back(ArgD); + ArgsData.back().MaskType = FS.getMaskType(); return true; } diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 740b472b0eb16..f4607c0134f2c 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -7653,6 +7653,14 @@ void CheckPrintfHandler::handleInvalidMaskType(StringRef MaskType) { S.Diag(getLocationOfByte(MaskType.data()), diag::err_invalid_mask_type_size); } +// Error out if struct or complex type argments are passed to os_log. +static bool isInvalidOSLogArgTypeForCodeGen(FormatStringType FSType, + QualType T) { + if (FSType != FormatStringType::OSLog) + return false; + return T->isRecordType() || T->isComplexType(); +} + bool CheckPrintfHandler::HandleAmount( const analyze_format_string::OptionalAmount &Amt, unsigned k, const char *startSpecifier, unsigned specifierLen) { @@ -7685,11 +7693,14 @@ bool CheckPrintfHandler::HandleAmount( assert(AT.isValid()); if (!AT.matchesType(S.Context, T)) { - EmitFormatDiagnostic(S.PDiag(diag::warn_printf_asterisk_wrong_type) - << k << AT.getRepresentativeTypeName(S.Context) - << T << Arg->getSourceRange(), + unsigned DiagID = isInvalidOSLogArgTypeForCodeGen(FSType, T) + ? diag::err_printf_asterisk_wrong_type + : diag::warn_printf_asterisk_wrong_type; + EmitFormatDiagnostic(S.PDiag(DiagID) + << k << AT.getRepresentativeTypeName(S.Context) + << T << Arg->getSourceRange(), getLocationOfByte(Amt.getStart()), - /*IsStringLocation*/true, + /*IsStringLocation*/ true, getSpecifierRange(startSpecifier, specifierLen)); // Don't do any more checking. We will just emit // spurious errors. @@ -8744,7 +8755,9 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS, Diag = diag::warn_format_conversion_argument_type_mismatch_confusion; break; case ArgType::NoMatch: - Diag = diag::warn_format_conversion_argument_type_mismatch; + Diag = isInvalidOSLogArgTypeForCodeGen(FSType, ExprTy) + ? diag::err_format_conversion_argument_type_mismatch + : diag::warn_format_conversion_argument_type_mismatch; break; } diff --git a/clang/test/CodeGenObjC/os_log.m b/clang/test/CodeGenObjC/os_log.m index 00229a746304c..837883ec4bb75 100644 --- a/clang/test/CodeGenObjC/os_log.m +++ b/clang/test/CodeGenObjC/os_log.m @@ -13,13 +13,6 @@ + (id)m1; C *c; -struct S { - int a[4]; -}; - -struct S s; -_Complex float cf; - @class NSString; extern __attribute__((visibility("default"))) NSString *GenString(void); void os_log_pack_send(void *); @@ -130,19 +123,3 @@ void test_builtin_os_log5(void *buf) { __builtin_os_log_format(buf, "capabilities: %@", (0, GenString())); os_log_pack_send(buf); } - -// CHECK-LABEL: define void @test_builtin_os_log6( -// CHECK: call void @__os_log_helper_1_0_0( - -void test_builtin_os_log6(void *buf) { - __builtin_os_log_format(buf, "%.*s %.*P %*.*f", s, s, s, s, s, s, s); -} - -// CHECK-LABEL: define linkonce_odr hidden void @__os_log_helper_1_0_0( - -// CHECK-LABEL: define void @test_builtin_os_log7( -// CHECK: call void @__os_log_helper_1_0_0( - -void test_builtin_os_log7(void *buf) { - __builtin_os_log_format(buf, "%.*s %.*P %*.*f", cf, cf, cf, cf, cf, cf, cf); -} diff --git a/clang/test/SemaObjC/os_log.m b/clang/test/SemaObjC/os_log.m new file mode 100644 index 0000000000000..3a8a550eb2009 --- /dev/null +++ b/clang/test/SemaObjC/os_log.m @@ -0,0 +1,22 @@ +// RUN: %clang_cc1 -verify %s + +struct S { + int a[4]; +}; + +struct S s; +_Complex float cf; + +void test_builtin_os_log_invalid_arg(void *buf) { + __builtin_os_log_format(buf, "%*.*f", s, 5, 1.3); // expected-error {{field width should have type 'int', but argument has type 'struct S'}} + __builtin_os_log_format(buf, "%*.*f", 1, s, 1.3); // expected-error {{field precision should have type 'int', but argument has type 'struct S'}} + __builtin_os_log_format(buf, "%*.*f", 1, 5, s); // expected-error {{format specifies type 'double' but the argument has type 'struct S'}} + + __builtin_os_log_format(buf, "%*.*f", cf, 5, 1.3); // expected-error {{field width should have type 'int', but argument has type '_Complex float'}} + __builtin_os_log_format(buf, "%*.*f", 1, cf, 1.3); // expected-error {{field precision should have type 'int', but argument has type '_Complex float'}} + __builtin_os_log_format(buf, "%*.*f", 1, 5, cf); // expected-error {{format specifies type 'double' but the argument has type '_Complex float'}} + + __builtin_os_log_format(buf, "%*.*f", (void *)0, 5, 1.3); // expected-warning {{field width should have type 'int', but argument has type 'void *'}} + __builtin_os_log_format(buf, "%*.*f", 1, (void *)0, 1.3); // expected-warning {{field precision should have type 'int', but argument has type 'void *'}} + __builtin_os_log_format(buf, "%*.*f", 1, 5, (void *)0); // expected-warning {{format specifies type 'double' but the argument has type 'void *'}} +}