● PHANTOM
🇮🇳 IN
✕
Skip to content

Fix MSan false-positive in zend_max_execution_timer#15408

Merged
zeriyoshi merged 3 commits intophp:PHP-8.2from
zeriyoshi:fix_zts_max_execution_timer
Aug 16, 2024
Merged

Fix MSan false-positive in zend_max_execution_timer#15408
zeriyoshi merged 3 commits intophp:PHP-8.2from
zeriyoshi:fix_zts_max_execution_timer

Conversation

@zeriyoshi
Copy link
Contributor

@zeriyoshi zeriyoshi commented Aug 14, 2024

ZEND_MAX_EXECUTION_TIMER is always enabled in PHP 8.3 and later ZTS builds, but accesses uninitialized memory.
Detected at clang-20 with MSan.

$ CC=clang CXX=clang++ CFLAGS="-DZEND_TRACK_ARENA_ALLOC" ./configure --enable-debug --enable-zts --enable-memory-sanitizer
$ make -j"$(nproc)"
~~~
==284762==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xaaaaafdafc04 in zend_max_execution_timer_settime /usr/src/php-fixed/Zend/zend_max_execution_timer.c:101:6
    #1 0xaaaaaf3f1bfc in zend_set_timeout_ex /usr/src/php-fixed/Zend/zend_execute_API.c:1567:2
    #2 0xaaaaaf3f1f60 in zend_set_timeout /usr/src/php-fixed/Zend/zend_execute_API.c:1636:2
    #3 0xaaaaaeac7c98 in php_request_startup /usr/src/php-fixed/main/main.c:1847:4
    #4 0xaaaaaff87168 in do_cli /usr/src/php-fixed/sapi/cli/php_cli.c:903:7
    #5 0xaaaaaff82988 in main /usr/src/php-fixed/sapi/cli/php_cli.c:1309:18
    #6 0xffffb9e7777c in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #7 0xffffb9e77854 in __libc_start_main csu/../csu/libc-start.c:360:3
    #8 0xaaaaac45ffec in _start (/usr/src/php-fixed/sapi/cli/php+0x14ffec) (BuildId: 27f7688a26e0107ad93e8e90a4bde71027f24c40)

  Uninitialized value was stored to memory at
    #0 0xaaaaafdafc00 in zend_max_execution_timer_settime /usr/src/php-fixed/Zend/zend_max_execution_timer.c:101:20
    #1 0xaaaaaf3f1bfc in zend_set_timeout_ex /usr/src/php-fixed/Zend/zend_execute_API.c:1567:2
    #2 0xaaaaaf3f1f60 in zend_set_timeout /usr/src/php-fixed/Zend/zend_execute_API.c:1636:2
    #3 0xaaaaaeac7c98 in php_request_startup /usr/src/php-fixed/main/main.c:1847:4
    #4 0xaaaaaff87168 in do_cli /usr/src/php-fixed/sapi/cli/php_cli.c:903:7
    #5 0xaaaaaff82988 in main /usr/src/php-fixed/sapi/cli/php_cli.c:1309:18
    #6 0xffffb9e7777c in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #7 0xffffb9e77854 in __libc_start_main csu/../csu/libc-start.c:360:3
    #8 0xaaaaac45ffec in _start (/usr/src/php-fixed/sapi/cli/php+0x14ffec) (BuildId: 27f7688a26e0107ad93e8e90a4bde71027f24c40)

  Uninitialized value was stored to memory at
    #0 0xaaaaafdafa1c in zend_max_execution_timer_settime /usr/src/php-fixed/Zend/zend_max_execution_timer.c:86:10
    #1 0xaaaaaf3f1bfc in zend_set_timeout_ex /usr/src/php-fixed/Zend/zend_execute_API.c:1567:2
    #2 0xaaaaaf3f1f60 in zend_set_timeout /usr/src/php-fixed/Zend/zend_execute_API.c:1636:2
    #3 0xaaaaaeac7c98 in php_request_startup /usr/src/php-fixed/main/main.c:1847:4
    #4 0xaaaaaff87168 in do_cli /usr/src/php-fixed/sapi/cli/php_cli.c:903:7
    #5 0xaaaaaff82988 in main /usr/src/php-fixed/sapi/cli/php_cli.c:1309:18
    #6 0xffffb9e7777c in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #7 0xffffb9e77854 in __libc_start_main csu/../csu/libc-start.c:360:3
    #8 0xaaaaac45ffec in _start (/usr/src/php-fixed/sapi/cli/php+0x14ffec) (BuildId: 27f7688a26e0107ad93e8e90a4bde71027f24c40)

  Uninitialized value was created by a heap allocation
    #0 0xaaaaac49a744 in malloc (/usr/src/php-fixed/sapi/cli/php+0x18a744) (BuildId: 27f7688a26e0107ad93e8e90a4bde71027f24c40)
    #1 0xaaaaaea9e4a4 in allocate_new_resource /usr/src/php-fixed/TSRM/TSRM.c:381:47
    #2 0xaaaaaea9d4d4 in ts_resource_ex /usr/src/php-fixed/TSRM/TSRM.c:451:3
    #3 0xaaaaaeadc8d4 in php_tsrm_startup_ex /usr/src/php-fixed/main/main.c:2759:8
    #4 0xaaaaaeadc938 in php_tsrm_startup /usr/src/php-fixed/main/main.c:2766:9
    #5 0xaaaaaff81a4c in main /usr/src/php-fixed/sapi/cli/php_cli.c:1202:2
    #6 0xffffb9e7777c in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #7 0xffffb9e77854 in __libc_start_main csu/../csu/libc-start.c:360:3
    #8 0xaaaaac45ffec in _start (/usr/src/php-fixed/sapi/cli/php+0x14ffec) (BuildId: 27f7688a26e0107ad93e8e90a4bde71027f24c40)

@zeriyoshi zeriyoshi requested a review from dstogov as a code owner August 14, 2024 15:46
@zeriyoshi zeriyoshi force-pushed the fix_zts_max_execution_timer branch from ab0af58 to 4369885 Compare August 14, 2024 15:49
@iluuu1994 iluuu1994 requested a review from arnaud-lb August 14, 2024 18:22
@arnaud-lb
Copy link
Member

arnaud-lb commented Aug 16, 2024 •
edited
Loading

I believe that it's a false positive. EG(max_execution_timer_timer) is initialized in zend_max_execution_timer_init() (I've checked that timer_create() initializes it fully).

The issue seems to be that MSAN does not intercept the timer_create() call, so it doesn't mark EG(max_execution_timer_timer) as initialized. Apparently an interceptor was implemented, but it was reverted shortly after.

Adding __msan_unpoison(&EG(max_execution_timer_timer), sizeof(EG(max_execution_timer_timer))); fixes the issue:

--- a/Zend/zend_max_execution_timer.c
+++ b/Zend/zend_max_execution_timer.c
@@ -24,6 +24,10 @@
 #include <sys/syscall.h>
 #include <sys/types.h>
 
+#if __has_feature(memory_sanitizer)
+# include <sanitizer/msan_interface.h>
+#endif
+
 #include "zend.h"
 #include "zend_globals.h"
 
@@ -47,6 +51,12 @@ ZEND_API void zend_max_execution_timer_init(void) /* {{{ */
        sev.sigev_signo = SIGRTMIN;
        sev.sigev_notify_thread_id = (pid_t) syscall(SYS_gettid);
 
+#if __has_feature(memory_sanitizer)
+       /* MSan does not intercept timer_create() */
+       __msan_unpoison(&EG(max_execution_timer_timer),
+                       sizeof(EG(max_execution_timer_timer)));
+#endif
+
        // Measure wall time instead of CPU time as originally planned now that it is possible https://github.com/php/php-src/pull/6504#issuecomment-1370303727
        if (timer_create(CLOCK_BOOTTIME, &sev, &EG(max_execution_timer_timer)) != 0) {
                zend_strerror_noreturn(E_ERROR, errno, "Could not create timer");

@zeriyoshi
Copy link
Contributor Author

zeriyoshi commented Aug 16, 2024

@arnaud-lb
Oops, my apologies!

It seems that this issue doesn't occur with the version of clang currently used with PHP (Ubuntu 22.04 / clang14).

However, it does appear to happen on Ubuntu 24.04 / clang18. Should we include a workaround?

@zeriyoshi zeriyoshi changed the title Fix uninitialized memory access in zend_max_execution_timer Fix MSan false-positive in zend_max_execution_timer Aug 16, 2024
@arnaud-lb
Copy link
Member

arnaud-lb commented Aug 16, 2024

No problem :)

Yes, I think we should include a workaround similar to the one above

arnaud-lb
arnaud-lb approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@zeriyoshi zeriyoshi merged commit ec9cdcd into php:PHP-8.2 Aug 16, 2024
@zeriyoshi zeriyoshi deleted the fix_zts_max_execution_timer branch August 16, 2024 17:54
@cmb69
Copy link
Member

cmb69 commented Aug 16, 2024

I see some errors occurring now, see https://app.circleci.com/pipelines/github/php/php-src/8842/workflows/8cdabb98-f0cb-4864-a572-878798126d82/jobs/8745 and https://github.com/php/php-src/actions/runs/10424615881/job/28873779000?pr=15446. Not exactly sure what the problem is.

@cmb69 cmb69 mentioned this pull request Aug 16, 2024
Merged
@zeriyoshi
Copy link
Contributor Author

zeriyoshi commented Aug 16, 2024

@cmb69
Sorry, I will fix the issue as soon as possible.

@zeriyoshi zeriyoshi mentioned this pull request Aug 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arnaud-lb arnaud-lb arnaud-lb approved these changes

@dstogov dstogov Awaiting requested review from dstogov

Assignees

No one assigned

Labels

Category: Engine

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zeriyoshi @arnaud-lb @cmb69