main: fix msan

[zeriyoshi]

The code is using estrdup on uninitialized memory allocated by emalloc. While this likely doesn't cause harm, it triggers a warning from MemorySanitizer on clang >= 18.

(I still have a lot of things I don't understand about this field. Please let me know if I'm mistaken.)

 ./sapi/cli/php -v
Uninitialized bytes in __interceptor_strlen at offset 1 inside [0xe29000000000, 33)
==16649==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xaaaad8a67124 in _estrdup /php-src/Zend/zend_alloc.c:2678:11
    #1 0xaaaad87809e4 in php_fopen_with_path /php-src/main/fopen_wrappers.c:688:13
    #2 0xaaaad878c330 in php_init_config /php-src/main/php_ini.c:587:9
    #3 0xaaaad8738098 in php_module_startup /php-src/main/main.c:2229:6
    #4 0xaaaad9c18c98 in php_cli_startup /php-src/sapi/cli/php_cli.c:410:9
    #5 0xaaaad9c10e5c in main /php-src/sapi/cli/php_cli.c:1300:6
    #6 0xffff95d22298  (/lib/aarch64-linux-gnu/libc.so.6+0x22298) (BuildId: 59a6cf3f027666659833681bc2039e9781f3c188)
    #7 0xffff95d2236c in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x2236c) (BuildId: 59a6cf3f027666659833681bc2039e9781f3c188)
    #8 0xaaaad68591ec in _start (/php-src/sapi/cli/php+0x1091ec) (BuildId: 2f336b1641ebcbb92eeab39fdd34febcbdc7a760)

  Uninitialized value was created by a heap allocation
    #0 0xaaaad688d5ac in malloc (/php-src/sapi/cli/php+0x13d5ac) (BuildId: 2f336b1641ebcbb92eeab39fdd34febcbdc7a760)
    #1 0xaaaad8a51f1c in tracked_malloc /php-src/Zend/zend_alloc.c:2832:14
    #2 0xaaaad8a62aa0 in _malloc_custom /php-src/Zend/zend_alloc.c:2477:10
    #3 0xaaaad8a622f8 in _emalloc /php-src/Zend/zend_alloc.c:2596:10
    #4 0xaaaad878af24 in php_init_config /php-src/main/php_ini.c:472:34
    #5 0xaaaad8738098 in php_module_startup /php-src/main/main.c:2229:6
    #6 0xaaaad9c18c98 in php_cli_startup /php-src/sapi/cli/php_cli.c:410:9
    #7 0xaaaad9c10e5c in main /php-src/sapi/cli/php_cli.c:1300:6
    #8 0xffff95d22298  (/lib/aarch64-linux-gnu/libc.so.6+0x22298) (BuildId: 59a6cf3f027666659833681bc2039e9781f3c188)
    #9 0xffff95d2236c in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x2236c) (BuildId: 59a6cf3f027666659833681bc2039e9781f3c188)
    #10 0xaaaad68591ec in _start (/php-src/sapi/cli/php+0x1091ec) (BuildId: 2f336b1641ebcbb92eeab39fdd34febcbdc7a760)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /php-src/Zend/zend_alloc.c:2678:11 in _estrdup
Exiting

[devnexen]

mstm

[iluuu1994]

We already intercept strlcat for MSAN:

php-src/Zend/zend_string.c

Lines 520 to 529 in 6732b88

	static size_t (*libc_strlcat)(char *__restrict, const char *__restrict, size_t);
	size_t strlcat (char *__restrict dest, const char *restrict src, size_t n)
	{
	if (!libc_strlcat) {
	libc_strlcat = dlsym(RTLD_NEXT, "strlcat");
	}
	size_t result = libc_strlcat(dest, src, n);
	__msan_unpoison_string(dest);
	return result;
	}

Not sure why this doesn't work for you...

[zeriyoshi]

I'm currently investigating the optimal testing methods for php-src development. I'll get back to you as soon as I understand the cause of this issue :)

https://github.com/zeriyoshi/php-containers-development/actions

[zeriyoshi]

I'm still struggling with MSan detection on Clang/LLVM 14 on Debian 12 Bookworm...

https://github.com/zeriyoshi/php-containers-development/actions/runs/10738681399/job/29782771727

(If you're interested, you can try it with the following Docker image: ghcr.io/zeriyoshi/php-containers-development:amd64-8.3-msan-nts-bookworm)

[iluuu1994]

@zeriyoshi I think you may need the llvm package to provide an instrumented libc.

[zeriyoshi]

@iluuu1994
Thank you, that's probably the cause!

I was able to make progress by installing the stable (18) old stable (17) package with libc for Debian from the LLVM Project (https://apt.llvm.org/), and by disabling non-instrumentable bundled extensions as @Girgias taught me in room11.

https://github.com/zeriyoshi/php-containers-development/actions/runs/10752149760

However, 8.3-zts, master-nts, and master-zts are still detecting errors with MSan. I looked at the content, but this might not be a false positive.
Could you check it when you have some free time? (The Zend Engine was too complex for me...)

You can use the container images (security options are necessary for MSan):

$ docker run --rm --privileged --security-opt seccomp:unconfined --cap-add SYS_ADMIN -it <IMAGE>
IMAGE:
    - master-nts: ghcr.io/zeriyoshi/php-containers-development:amd64-master-msan-nts-bookworm
    - master-zts: ghcr.io/zeriyoshi/php-containers-development:amd64-master-msan-zts-bookworm
    - 8.3-zts: ghcr.io/zeriyoshi/php-containers-development:amd64-8.3-msan-zts-bookworm

The issue in master might be related to Enum.
I was able to fix it with the following patch (I couldn't determine if this is a false positive):

https://github.com/zeriyoshi/php-containers-development/actions/runs/10752149760/job/29820131890#step:3:351

diff --git a/Zend/zend_enum.c b/Zend/zend_enum.c
index e6731aed40..123d1d317d 100644
--- a/Zend/zend_enum.c
+++ b/Zend/zend_enum.c
@@ -533,7 +533,7 @@ static zend_ast_ref *create_enum_case_ast(
 	// TODO: Use custom node type for enum cases?
 	size_t size = sizeof(zend_ast_ref) + zend_ast_size(3)
 		+ (value ? 3 : 2) * sizeof(zend_ast_zval);
-	char *p = pemalloc(size, 1);
+	char *p = pecalloc(1, size, 1);
 	zend_ast_ref *ref = (zend_ast_ref *) p; p += sizeof(zend_ast_ref);
 	GC_SET_REFCOUNT(ref, 1);
 	GC_TYPE_INFO(ref) = GC_CONSTANT_AST | GC_PERSISTENT | GC_IMMUTABLE;

[zeriyoshi]

This was due to my test environment not being properly configured. sorry!

hint: For Debian, always use the packages from apt.llvm.org.
https://apt.llvm.org/

[zeriyoshi]

@iluuu1994 @Girgias
Sorry, I closed wrong PR...

[bukka]

But it sets php_ini_search_path[0] = 0 after that so I don't see how this could be problematic. Why do you think that this might not be a false positive?

[iluuu1994]

Right, this is a false positive. Can you please check whether the strlcat() override in zend_string.c is actually triggered?

[iluuu1994]

Ahh, you're testing on PHP-8.2. This issue is only fixed on master. If you want to use it on PHP-8.2, you'll need to backport #12674.

[zeriyoshi]

@bukka @iluuu1994
I apologize, the issue initially reported in this Pull Request was a false positive due to my oversight.

Currently, I'm uncertain whether the following MSan warnings are false positives or not:

• zend_stack_int_top https://github.com/zeriyoshi/php-containers-development/actions/runs/10752149760/job/29820131380#step:3:125
• zend_ast_get_lineno https://github.com/zeriyoshi/php-containers-development/actions/runs/10752149760/job/29820131890#step:3:380 Fixed Fix uninitialized lineno in enum init ast #15806

Regarding zend_ast_get_lineno, I believe the changes made to the Enum are the cause. In fact, changing pemalloc to pecalloc eliminated the warning.

However, since I'm unsure if this warning is a false positive, I don't know whether we should use calloc or keep it as malloc...

[iluuu1994]

Regarding zend_ast_get_lineno, I believe the changes made to the Enum are the cause. In fact, changing pemalloc to pecalloc eliminated the warning.

This is already fixed. See #15806.

I'll need to take a closer look at zend_stack_int_top to determine the cause. This test didn't fail for me yesterday.

[zeriyoshi]

@iluuu1994
I overlooked that! Thank you!

Regarding the issue with zend_stack_int_top, I'll look into it a bit more.

[zeriyoshi]

After retrying the tests on the latest branch, the NTS issue has been completely resolved!
However, the ZTS build still shows MSan warnings around error_reporting. I'll take a closer look when I have more time.

https://github.com/zeriyoshi/php-containers-development/actions/runs/10772695102

[iluuu1994]

@zeriyoshi I backported existing fixes, and created a few new ones.

#15812
#15813
#15814

This fixes all issues for me locally.

[zeriyoshi]

@iluuu1994
Thank you!
I've tested these changes on a branch and confirmed that all tests passed successfully :)
https://github.com/zeriyoshi/php-containers-development/actions/runs/10777301170

[zeriyoshi]

By the way, are tests like these helpful for php-src development?

https://github.com/zeriyoshi/php-containers-development/actions/runs/10752149760

I think they might be useful because by conducting tests on Linux using container images, we can provide developers with snapshot images, which could be beneficial.

[iluuu1994]

@zeriyoshi Note that I haven't looked at your repository. Generally, I wouldn't object to re-working GitHub actions, our configuration has lots of repetition. However, there are many special variations, especially in nightly, and I'm not sure it makes sense to just replace a fraction of them.

[zeriyoshi]

@iluuu1994 Thank you. I plan to continue refining it until it can fully replace the current php-src nightly, while also seeking opinions on the Internals mailing list!