[clang][Sema] Add support for OpenBSD's syslog format attribute

[brad0]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Brad Smith (brad0)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/97366.diff

4 Files Affected:

• (modified) clang/include/clang/Sema/Sema.h (+1)
• (modified) clang/lib/Sema/SemaChecking.cpp (+3-2)
• (modified) clang/lib/Sema/SemaDeclAttr.cpp (+1-1)
• (modified) clang/test/Sema/attr-format.c (+7)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index ef4fc47567a7c..89d8a7009e4d9 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2108,6 +2108,7 @@ class Sema final : public SemaBase {
     FST_FreeBSDKPrintf,
     FST_OSTrace,
     FST_OSLog,
+    FST_Syslog,
     FST_Unknown
   };
   static FormatStringType GetFormatStringType(const FormatAttr *Format);
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index cc3615da7f940..7ea23cdbf0274 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -6114,7 +6114,7 @@ static const Expr *maybeConstEvalStringLiteral(ASTContext &Context,
 Sema::FormatStringType Sema::GetFormatStringType(const FormatAttr *Format) {
   return llvm::StringSwitch<FormatStringType>(Format->getType()->getName())
       .Case("scanf", FST_Scanf)
-      .Cases("printf", "printf0", FST_Printf)
+      .Cases("printf", "printf0", "syslog", FST_Printf)
       .Cases("NSString", "CFString", FST_NSString)
       .Case("strftime", FST_Strftime)
       .Case("strfmon", FST_Strfmon)
@@ -6211,6 +6211,7 @@ bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args,
     case FST_Kprintf:
     case FST_FreeBSDKPrintf:
     case FST_Printf:
+    case FST_Syslog:
       Diag(FormatLoc, diag::note_format_security_fixit)
         << FixItHint::CreateInsertion(FormatLoc, "\"%s\", ");
       break;
@@ -7947,7 +7948,7 @@ static void CheckFormatString(
 
   if (Type == Sema::FST_Printf || Type == Sema::FST_NSString ||
       Type == Sema::FST_FreeBSDKPrintf || Type == Sema::FST_OSLog ||
-      Type == Sema::FST_OSTrace) {
+      Type == Sema::FST_OSTrace || Type == Sema::FST_Syslog) {
     CheckPrintfHandler H(
         S, FExpr, OrigFormatExpr, Type, firstDataArg, numDataArgs,
         (Type == Sema::FST_NSString || Type == Sema::FST_OSTrace), Str, APK,
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 41489789919d0..2a9b43a26287a 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3436,7 +3436,7 @@ static FormatAttrKind getFormatAttrKind(StringRef Format) {
       // Otherwise, check for supported formats.
       .Cases("scanf", "printf", "printf0", "strfmon", SupportedFormat)
       .Cases("cmn_err", "vcmn_err", "zcmn_err", SupportedFormat)
-      .Case("kprintf", SupportedFormat)         // OpenBSD.
+      .Cases("kprintf", "syslog", SupportedFormat) // OpenBSD.
       .Case("freebsd_kprintf", SupportedFormat) // FreeBSD.
       .Case("os_trace", SupportedFormat)
       .Case("os_log", SupportedFormat)
diff --git a/clang/test/Sema/attr-format.c b/clang/test/Sema/attr-format.c
index 1f4c864d4f78b..5a8b1ac9eca5c 100644
--- a/clang/test/Sema/attr-format.c
+++ b/clang/test/Sema/attr-format.c
@@ -99,3 +99,10 @@ void forward_fixed(const char *fmt, _Bool b, char i, short j, int k, float l, do
   a(fmt, b, i, j, k, l, m);
 }
 
+// OpenBSD
+// same as format(printf(...))...
+void a2(const char *a, ...) __attribute__((format(syslog, 1, 2)));    // no-error
+void b2(const char *a, ...) __attribute__((format(syslog, 1, 1)));    // expected-error {{'format' attribute parameter 3 is out of bounds}}
+void c2(const char *a, ...) __attribute__((format(syslog, 0, 2)));    // expected-error {{'format' attribute parameter 2 is out of bounds}}
+void d2(const char *a, int c) __attribute__((format(syslog, 1, 2)));  // expected-warning {{GCC requires a function with the 'format' attribute to be variadic}}
+void e2(char *str, int c, ...) __attribute__((format(syslog, 2, 3))); // expected-error {{format argument not a string type}}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Endilll]

Sema.h changes look good.

[Sirraide]

Hmm, this function also seems to exist on Linux, though. Additionally, from my man syslog docs:

The remaining arguments are a format, as in printf(3), and any arguments required by the format, except that the two-character sequence %m will be replaced by the error message string strerror(errno).

I also know next to nothing about BSD and how its syslog() works, but we should support that extra %m specifier (if we don’t already do that because it’s apparently also a glibc extension for printf()...). If the two of them differ, we might need two different format styles for BSD syslog() vs Linux syslog().

[brad0]

Hmm, this function also seems to exist on Linux, though. Additionally, from my man syslog docs:

I'm not sure what you mean by that in relation to this addition of a format attribute.

The remaining arguments are a format, as in printf(3), and any arguments required by the format, except that the two-character sequence %m will be replaced by the error message string strerror(errno).

I also know next to nothing about BSD and how its syslog() works, but we should support that extra %m specifier (if we don’t already do that because it’s apparently also a glibc extension for printf()...). If the two of them differ, we might need two different format styles for BSD syslog() vs Linux syslog().

Should be more or less the same as Linux and the X/Open specified functions.

The %m specifier is not a glibc extension from what I can find.

[Sirraide]

Hmm, this function also seems to exist on Linux, though. Additionally, from my man syslog docs:

I'm not sure what you mean by that in relation to this addition of a format attribute.

I think my question was mainly as to why syslog is treated as an OpenBSD feature (at least in getFormatAttrKind) if it’s not exclusive to OpenBSD, because Linux also seems to have it.

The %m specifier is not a glibc extension from what I can find.

According to man 3 printf on my system:

Additionally:

llvm-project/clang/lib/AST/PrintfFormatString.cpp

Lines 346 to 347 in 106621b

	// Glibc specific.
	case 'm': k = ConversionSpecifier::PrintErrno; break;

So it does seem like we already support this. In that case, amending the comment to include that it’s also used by syslog() (on both BSD and Linux) would be nice.

[Sirraide]

So yeah, this still needs a release note, and I don’t think we should be treating it / referring to it as an openbsd-exclusive feature, but apart from that the changes LGTM.