Fix memory leak in syslog extension

[danog]

The leak can be easily reproduced with ASAN:

php --repeat 2 -f a.php

a.php:

<?php
$priority = LOG_WARNING;
$message = "A simple \0 message";

openlog('PHPT', LOG_PERROR, LOG_USER);
syslog($priority, $message);

?>

Result:

Executing for the first time...
PHPT: A simple \x00 message
Finished execution, repeating...
PHPT: A simple \x00 message

=================================================================
==1318513==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 5 byte(s) in 1 object(s) allocated from:
    #0 0x7ffbe0348819 in __interceptor_malloc (/usr/lib/clang/16/lib/linux/libclang_rt.asan-x86_64.so+0x148819) (BuildId: 6d967465ddfb168365d0ee0296553c15bd2c72de)
    #1 0x563dfcb291f4 in zend_strndup /home/daniil/repos/php-src/Zend/zend_alloc.c:2711:15
    #2 0x563dfc8f61ba in zif_openlog /home/daniil/repos/php-src/ext/standard/syslog.c:90:22
    #3 0x563dfce368c2 in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER /home/daniil/repos/php-src/Zend/zend_vm_execute.h:1250:2
    #4 0x563dfccb0dcc in execute_ex /home/daniil/repos/php-src/Zend/zend_vm_execute.h:55836:7
    #5 0x563dfccb1627 in zend_execute /home/daniil/repos/php-src/Zend/zend_vm_execute.h:60409:2
    #6 0x563dfcc00e78 in zend_execute_scripts /home/daniil/repos/php-src/Zend/zend.c:1833:4
    #7 0x563dfca170d0 in php_execute_script /home/daniil/repos/php-src/main/main.c:2542:13
    #8 0x563dfd17bbe6 in do_cli /home/daniil/repos/php-src/sapi/cli/php_cli.c:964:5
    #9 0x563dfd1796fa in main /home/daniil/repos/php-src/sapi/cli/php_cli.c:1333:18
    #10 0x7ffbdf650ccf  (/usr/lib/libc.so.6+0x27ccf) (BuildId: 8bfe03f6bf9b6a6e2591babd0bbc266837d8f658)

SUMMARY: AddressSanitizer: 5 byte(s) leaked in 1 allocation(s).

[danog]

Removed the rinit function because we already do BG(syslog_device)=NULL in the minit function, plus this is what causes the leak.

[dstogov]

I think the fix is wrong. With the patch openlog() called in first request may have effect on syslog() called in second request.

[danog]

With the patch openlog() called in first request may have effect on syslog() called in second request.

@dstogov But I explicitly call closelog in the rshutsown (not mshutdown) function and clear the prefix buffer, on the contrary shouldn't that completely reset the syslog state after each request finishes?

[dstogov]

@dstogov But I explicitly call closelog in the rshutsown (not mshutdown) function and clear the prefix buffer, on the contrary shouldn't that completely reset the syslog state after each request finishes?

Ah, I messed RSHUTDOWN and MSHUTDOWN...

[dstogov]

Looks good

[bukka]

I think it makes sense but leave it open for a little bit. I want to test few things if set from FPM config or through fcgi. Should be fine but just want to verify.

[dstogov]

@bukka any results?

[bukka]

I plan to test it at the end of this week.

[bukka]

I just had a proper look on this and it is actually fine as the value was reset already - just leaking. Think this makes more sense so merged.

[dktapps]

This change broke something in ZTS during shutdown. I'm seeing a lot of extension faults like this:

==7135== Thread 2:
==7135== Conditional jump or move depends on uninitialised value(s)
==7135==    at 0x5BC2AC: zm_deactivate_syslog (syslog.c:47)
==7135==    by 0x5376E2: zm_deactivate_basic (basic_functions.c:470)
==7135==    by 0x6EF6BF: zend_deactivate_modules (zend_API.c:3240)
==7135==    by 0x612E1C: php_request_shutdown (main.c:1851)
==7135==    by 0x5326067: pmmpthread_prepared_shutdown (in /home/runner/work/ext-pmmpthread/ext-pmmpthread/php/lib/php/extensions/debug-zts-20230901/pmmpthread.so)
==7135==    by 0x532D453: pmmpthread_routine (in /home/runner/work/ext-pmmpthread/ext-pmmpthread/php/lib/php/extensions/debug-zts-20230901/pmmpthread.so)
==7135==    by 0x49E4608: start_thread (pthread_create.c:477)
==7135==    by 0x4B1E132: clone (clone.S:95)
==7135== 

Looks like this is not initialized correctly due to removal of RINIT:

php-src/ext/standard/syslog.c

Line 47 in cc2bf11

if (BG(syslog_device)) {

[danog]

Hmm, we already do BG(syslog_device)=NULL; in MINIT, that should be enough....

[bukka]

Ah, it should be initialized in GINIT, not in MINIT...

[bukka]

Hopefully this will do: #12663