PoC: php_printf: introduce %pS to replace custom specifier %S

[arnaud-lb]

The string formater supports custom format specifiers such as S (zend_string*), but format strings using these specifiers do not pass the compiler's type checks that are performed on functions tagged with ZEND_ATTRIBUTE_FORMAT:

Zend/zend_compile.c: In function 'zend_compile_closure_binding':
Zend/zend_compile.c:8586:62: error: format '%S' expects argument of type 'wchar_t *', but argument 3 has type 'zend_string *' {aka 'struct _zend_string *'} [-Werror=format=]
 8586 |                         zend_error_noreturn(E_COMPILE_ERROR, "Cannot use variable $%S twice", var_name);
      |                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~
      |                                                                                               |
      |                                                                                               zend_string * {aka struct _zend_string *}

As a result we can not use these specifiers without resorting to workarounds:

• Use variants of formatting functions that do not have ZEND_ATTRIBUTE_FORMAT (example)
• Or declare the format string separately (example)

Here I propose to re-introduce %S as %pS. The compiler will only see a %p specifier followed by the ordinary literal character S, so it will be happy about an argument of type zend_string*.

This trick is used in the Linux kernel, and can be applied to more custom specifiers.

[arnaud-lb]

cc @TimWolla @DanielEScherzer since you've suggested to use %S in reviews

[Girgias]

Ohhhh I like this idea a lot, will allows us to remove all of those unchecked variants too. Pinging @NattyNarwhal as we encountered a similar problem looking at php_error related error messages recently.

[TimWolla]

I recently learned that passing a non-void* pointer to %p is technically UB, but all compilers just seem to accept it without warnings. Since this is not a regular printf() anyways, I very much like this hack.

[NattyNarwhal]

The only other "easy" hack is some kind of macro to just cast zend_string* to wchar_t*, and this is less intrusive. With this, you can get rid of php_error_docref_unchecked too, which only has one user.

I was wondering about proposing a way to specify custom format specifiers to gcc/clang. That I think would be very useful (i.e. don't mix up char* and zend_string* by accident), but it'd be a while before we could use such a thing.

[DanielEScherzer]

• this should probably be highlighted as a breaking change in UPGRADING.INTERNALS - we have existing in-tree code that uses %pd that wouldn't break, but is the kind of thing that would break,

  php-src/ext/sqlite3/sqlite3.c

  Line 1663 in 61a190c

  php_sqlite3_error(stmt_obj->db_obj, 0, "Unknown parameter type: %pd for parameter %pd", param->type, param->param_number);

• what if I wanted the old behavior of %pS or some other character afterwards? Can we add support for %pP to mean "I want %p", so that %pPS allows the old behavior, and (e.g. if %pd was added to format something custom) %pPd would allow the old behavior?

[arnaud-lb]

• this should probably be highlighted as a breaking change in UPGRADING.INTERNALS - we have existing in-tree code that uses %pd that wouldn't break, but is the kind of thing that would break,

  php-src/ext/sqlite3/sqlite3.c

  Line 1663 in 61a190c

  php_sqlite3_error(stmt_obj->db_obj, 0, "Unknown parameter type: %pd for parameter %pd", param->type, param->param_number);

I was planning to not change the behavior of %pSomething unless Something has special meaning. So the behavior of %pd wouldn't change.

But maybe it's better to break this once and for all, so that new specifiers can be introduced without breaking changes in the future.

Edit: It looks like that this sqlite3 code is unreachable? Apparently the p in %pd is a length modifier, but this modifier triggers an E_CORE_ERROR since 8.1.

• what if I wanted the old behavior of %pS or some other character afterwards? Can we add support for %pP to mean "I want %p", so that %pPS allows the old behavior, and (e.g. if %pd was added to format something custom) %pPd would allow the old behavior?

Very good point. Tried to find how the kernel addresses this, but apparently they don't. A workaround is to print the adjacent char with %c:

- php_printf("%pS", ptr);
+ php_printf("%p%c", ptr, 'S');

But I like the idea of doubling p:

- php_printf("%pS", ptr);
+ php_printf("%ppS", ptr);