Bug 298450 – [WTF] Use SAFE_FPRINTF / SAFE_PRINTF_TYPE for logging

• Status: REOPENED
• Component: Platform

[Adrian Perez]

In  298228@main (bug #296043) we reverted usage of SAFE_FPRINTF() for stderr logging, in order to make all logging invocations consistent

It should be possible to use SAFE_FPRINTF() as long as we also change the other logging implementations (os_log, Android, journald) to apply the same treatment to format parameters, making them go through SAFE_PRINTF_TYPE/WTF::safePrrintfType to have them all applying the same conversions and type checks.

[Radar WebKit Bug Importer]

<rdar://problem/160470046>

[Alexsander Borges Damaceno]

Pull request: https://github.com/WebKit/WebKit/pull/50908

[EWS]

Committed 300391@main (69d9dbfea896): <https://commits.webkit.org/300391@main>

Reviewed commits have been landed. Closing PR #50908 and removing active labels.

[Adrian Perez]

Reopened Bugzilla.
Broke Clang/Android builds, many logging locations use raw string pointers that would need changing, tracking revert in https://bugs.webkit.org/show_bug.cgi?id=299430.

[Adrian Perez]

I had to revert this because it caused many errors like this when building with Clang:

WTF/Headers/wtf/StdLibExtras.h:1202:19: error: static assertion failed due to requirement '!std::is_same_v<char, char>': char* is not bounds safe; please use a null terminated string type
 1202 |     static_assert(!std::is_same_v<std::remove_cv_t<std::remove_pointer_t<T>>, char>, "char* is not bounds safe; please use a null terminated string type");
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/aperez/devel/WebKit-Android/Source/WebDriver/soup/HTTPServerSoup.cpp:59:9: note: in instantiation of function template specialization 'WTF::safePrintfType<char *>' requested here
   59 |         RELEASE_LOG(WebDriverClassic, "Failed to start HTTP server at port %u: %s", port, error->message);
      |         ^

Also there was a mistake and __VA__OPT__ was used (instead of the correct __VA_OPT__) that broke the Android build.

Given that there are plenty of call sites that expand logging macros using raw pointers (const char* / char*), I am not sure that trying to re-land this is a good idea. At least we would need to plan what to do regarding usage of the logging macros. I took a very quick look before deciding to revert, and at last I could already see a few problematic patterns when passing values for "%s" format specifiers:

  - Passing the result from calling ASCIILiteral::characters(). These need to be changed to pass the ASCIILiteral directly.
  - Same for String::utf8().data(). These need to be changed to remove the .data() call.
  - Some classes (e.g. in the Linux memory pressure handler) keep raw C strings (char*) as logging tags and use them later on for logging. I think these can be changed to use ASCIILiteral.
  - Extracting char* pointers from other string types in intermediate variables.
  - Passing the string from GError::message.

...and probably many other uses that I didn't notice.