Thanks!

### Description

/bin/sh /builds/php-8.2.4/libtool --silent --preserve-dup-deps --tag CC --mode=compile gcc-12 -Iext/standard/ -I/builds/php-8.2.4/ext/standard/ -I/builds/php-8.2.4/include -I/builds/php-8.2.4/main -I/builds/php-8.2.4 -I/builds/php-8.2.4/ext/date/lib -I/usr/include/libxml2 -I/usr/include/libpng16 -I/builds/php-8.2.4/ext/mbstring/libmbfl -I/builds/php-8.2.4/ext/mbstring/libmbfl/mbfl -I/builds/php-8.2.4/TSRM -I/builds/php-8.2.4/Zend  -D_GNU_SOURCE -D_REENTRANT -pthread  -fno-common -Wformat-truncation -Wlogical-op -Wduplicated-cond -Wno-clobbered -Wall -Wextra -Wno-strict-aliasing -Wno-unused-parameter -Wno-sign-compare -g -O2 -fvisibility=hidden -pthread -Wimplicit-fallthrough=1 -DZTS -DZEND_SIGNALS   -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 -c /builds/php-8.2.4/ext/standard/link.c -o ext/standard/link.lo  -MMD -MF ext/standard/link.dep -MT ext/standard/link.lo
In function ‘from_zval_write_sockaddr_aux’,
    inlined from ‘from_zval_write_name’ at /builds/php-8.2.4/ext/sockets/conversions.c:1041:2:
/builds/php-8.2.4/ext/sockets/conversions.c:727:9: warning: ‘family’ may be used uninitialized [-Wmaybe-uninitialized]
  727 |         switch (family) {
      |         ^~~~~~
/builds/php-8.2.4/ext/sockets/conversions.c: In function ‘from_zval_write_name’:
/builds/php-8.2.4/ext/sockets/conversions.c:703:25: note: ‘family’ was declared here
  703 |         int             family;
      |                         ^~~~~~ 

/bin/sh /builds/php-8.2.4/libtool --silent --preserve-dup-deps --tag CC --mode=compile gcc-12 -Iext/standard/ -I/builds/php-8.2.4/ext/standard/ -I/builds/php-8.2.4/include -I/builds/php-8.2.4/main -I/builds/php-8.2.4 -I/builds/php-8.2.4/ext/date/lib -I/usr/include/libxml2 -I/usr/include/libpng16 -I/builds/php-8.2.4/ext/mbstring/libmbfl -I/builds/php-8.2.4/ext/mbstring/libmbfl/mbfl -I/builds/php-8.2.4/TSRM -I/builds/php-8.2.4/Zend  -D_GNU_SOURCE -D_REENTRANT -pthread  -fno-common -Wformat-truncation -Wlogical-op -Wduplicated-cond -Wno-clobbered -Wall -Wextra -Wno-strict-aliasing -Wno-unused-parameter -Wno-sign-compare -g -O2 -fvisibility=hidden -pthread -Wimplicit-fallthrough=1 -DZTS -DZEND_SIGNALS   -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 -c /builds/php-8.2.4/ext/standard/math.c -o ext/standard/math.lo  -MMD -MF ext/standard/math.dep -MT ext/standard/math.lo
/builds/php-8.2.4/ext/sockets/conversions.c: In function ‘from_zval_write_controllen’:
/builds/php-8.2.4/ext/sockets/conversions.c:1122:31: warning: ‘len’ may be used uninitialized [-Wmaybe-uninitialized]
 1122 |         msghdr->msg_control = accounted_emalloc(len, ctx);
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/php-8.2.4/ext/sockets/conversions.c:1112:18: note: ‘len’ was declared here
 1112 |         uint32_t len;
      |                  ^~~ 

### PHP Version

PHP 8.2.4

### Operating System

SLES 15.4 with gcc 12

Silence compiler warnings in ext/sockets/conversions.c (#10974)

These values will be initialised, but the compiler can't see it. Write a dummy value to silence this.

Closes GH-10959.

These values will be initialised, but the compiler can't see it. Write a dummy value to silence this.

Closes GH-10959.

Fix an additional problem

Remove unnecessary memory clearing in virtual_file_ex()

I checked a simple Laravel CRUD application's home page under Callgrind and found that the line: char resolved_path[MAXPATHLEN] = {0}; took up about 0.95% of the spent instruction count. This is because when opcache revalidates the timestamps, it has to go through the function virtual_file_ex() which contains that line. That line will memset 4096 bytes on my system to all zeroes. This is bad for the data cache and for the runtime.

I found that this memsetting is unnecessary in most cases, and that we can fix the one remaining case:

• Lines 1020-1027 don't do anything with resolved_path, so that's okay.
• Lines 1033-1098:
  • The !IS_ABSOLUTE_PATH branch will always result in a memcpy from path to resolved_path (+ sometimes an offset) with the total copied amount equal to path_length+1, so that includes a NUL byte.
  • The else branch either takes the WIN32 path or the non-WIN32 path. ° WIN32: There's a copy from path+2 with length path_length-1. Note that we chop off the first 2 bytes, so this also includes the NUL byte. ° Non-WIN32: Copies path_length+1 bytes, so that includes a NUL byte.

At this point we know that resolved_path ends in a NUL byte. Going further in the code:

• Lines 1100-1106 don't write to resolved_path, so no NUL byte is removed.
• Lines 1108-1136:
  • The IS_UNC_PATH branch: ° Lines 1111-1112 don't overwrite the NUL byte, because we know the path length is at least 2 due to the IS_UNC_PATH check. ° Both while loops uppercase the path until a slash is found. If a NUL byte was found then it jumps to verify. Therefore, no NUL byte can be overwritten. Furthermore, Lines 1121 and 1129 cannot overwrite a NUL byte because the check at lines 1115 and 1123 would've jumped to verify when a NUL byte would be encountered. Therefore, the IS_UNC_PATH branch cannot overwrite a NUL byte, so the NUL byte we know we already got stays in place.
  • The else branch: ° We know the path length is at least 2 due to IS_ABSOLUTE_PATH. That means the earliest NUL byte can be at index 2, which can be overwritten on line 1133. We fix this by adding one byte write if the length is 2.

All uses of resolved_path in lines 1139-1141 have a NUL byte at the end now. Lines 1154-1164 do a bunch of post-processing but line 1164 will make sure resolved_path still ends in a NUL byte.

So therefore I propose to remove the huge memset, and add a single byte write in that one else branch I mentioned earlier.

Looking at Callgrind, the instruction count before this patch for 200 requests is 14,264,569,942; and after the patch it's 14,129,358,195 (averaged over a handful of runs).

Remove unnecessary memory clearing in virtual_file_ex()

I checked a simple Laravel CRUD application's home page under Callgrind and found that the line: char resolved_path[MAXPATHLEN] = {0}; took up about 0.95% of the spent instruction count. This is because when opcache revalidates the timestamps, it has to go through the function virtual_file_ex() which contains that line. That line will memset 4096 bytes on my system to all zeroes. This is bad for the data cache and for the runtime.

I found that this memsetting is unnecessary in most cases, and that we can fix the one remaining case:

• Lines 1020-1027 don't do anything with resolved_path, so that's okay.
• Lines 1033-1098:
  • The !IS_ABSOLUTE_PATH branch will always result in a memcpy from path to resolved_path (+ sometimes an offset) with the total copied amount equal to path_length+1, so that includes a NUL byte.
  • The else branch either takes the WIN32 path or the non-WIN32 path. ° WIN32: There's a copy from path+2 with length path_length-1. Note that we chop off the first 2 bytes, so this also includes the NUL byte. ° Non-WIN32: Copies path_length+1 bytes, so that includes a NUL byte.

At this point we know that resolved_path ends in a NUL byte. Going further in the code:

• Lines 1100-1106 don't write to resolved_path, so no NUL byte is removed.
• Lines 1108-1136:
  • The IS_UNC_PATH branch: ° Lines 1111-1112 don't overwrite the NUL byte, because we know the path length is at least 2 due to the IS_UNC_PATH check. ° Both while loops uppercase the path until a slash is found. If a NUL byte was found then it jumps to verify. Therefore, no NUL byte can be overwritten. Furthermore, Lines 1121 and 1129 cannot overwrite a NUL byte because the check at lines 1115 and 1123 would've jumped to verify when a NUL byte would be encountered. Therefore, the IS_UNC_PATH branch cannot overwrite a NUL byte, so the NUL byte we know we already got stays in place.
  • The else branch: ° We know the path length is at least 2 due to IS_ABSOLUTE_PATH. That means the earliest NUL byte can be at index 2, which can be overwritten on line 1133. We fix this by adding one byte write if the length is 2.

All uses of resolved_path in lines 1139-1141 have a NUL byte at the end now. Lines 1154-1164 do a bunch of post-processing but line 1164 will make sure resolved_path still ends in a NUL byte.

So therefore I propose to remove the huge memset, and add a single byte write in that one else branch I mentioned earlier.

Looking at Callgrind, the instruction count before this patch for 200 requests is 14,264,569,942; and after the patch it's 14,129,358,195 (averaged over a handful of runs).

Alright I pushed the requested change. Thanks for reviewing! :)

Remove unnecessary memory clearing in virtual_file_ex()

I checked a simple Laravel CRUD application's home page under Callgrind and found that the line: char resolved_path[MAXPATHLEN] = {0}; took up about 0.95% of the spent instruction count. This is because when opcache revalidates the timestamps, it has to go through the function virtual_file_ex() which contains that line. That line will memset 4096 bytes on my system to all zeroes. This is bad for the data cache and for the runtime.

I found that this memsetting is unnecessary in most cases, and that we can fix the one remaining case:

• Lines 1020-1027 don't do anything with resolved_path, so that's okay.
• Lines 1033-1098:
  • The !IS_ABSOLUTE_PATH branch will always result in a memcpy from path to resolved_path (+ sometimes an offset) with the total copied amount equal to path_length+1, so that includes a NUL byte.
  • The else branch either takes the WIN32 path or the non-WIN32 path. ° WIN32: There's a copy from path+2 with length path_length-1. Note that we chop off the first 2 bytes, so this also includes the NUL byte. ° Non-WIN32: Copies path_length+1 bytes, so that includes a NUL byte.

At this point we know that resolved_path ends in a NUL byte. Going further in the code:

• Lines 1100-1106 don't write to resolved_path, so no NUL byte is removed.
• Lines 1108-1136:
  • The IS_UNC_PATH branch: ° Lines 1111-1112 don't overwrite the NUL byte, because we know the path length is at least 2 due to the IS_UNC_PATH check. ° Both while loops uppercase the path until a slash is found. If a NUL byte was found then it jumps to verify. Therefore, no NUL byte can be overwritten. Furthermore, Lines 1121 and 1129 cannot overwrite a NUL byte because the check at lines 1115 and 1123 would've jumped to verify when a NUL byte would be encountered. Therefore, the IS_UNC_PATH branch cannot overwrite a NUL byte, so the NUL byte we know we already got stays in place.
  • The else branch: ° We know the path length is at least 2 due to IS_ABSOLUTE_PATH. That means the earliest NUL byte can be at index 2, which can be overwritten on line 1133. We fix this by adding one byte write if the length is 2.

All uses of resolved_path in lines 1139-1141 have a NUL byte at the end now. Lines 1154-1164 do a bunch of post-processing but line 1164 will make sure resolved_path still ends in a NUL byte.

So therefore I propose to remove the huge memset, and add a single byte write in that one else branch I mentioned earlier.

Looking at Callgrind, the instruction count before this patch for 200 requests is 14,264,569,942; and after the patch it's 14,129,358,195 (averaged over a handful of runs).

Hmmm Upon adding @Girgias as reviewer GitHub auto-removed @devnexen (code-owner) review request, and now I can't add him anymore for some reason... Strange.

These values will be initialised, but the compiler can't see it. Write a dummy value to silence this.

Closes GH-10959.

This was first pointed out in GH-10959. The from_zval_... functions don't always write to the pointer, in particular it is necessary to check for an error before using the value. Otherwise we can access an uninitialized value and that's UB (and dangerous).

Note: this does NOT get rid of the compiler warning. Even though there is error checking now, the compiler isn't smart enough to figure out that the values can not be used uninitialized.

Fix uninitialized variable accesses in sockets/conversions

This was first pointed out in GH-10959. The from_zval_... functions don't always write to the pointer, in particular it is necessary to check for an error before using the value. Otherwise we can access an uninitialized value and that's UB (and dangerous).

Note: this does NOT get rid of the compiler warning. Even though there is error checking now, the compiler isn't smart enough to figure out that the values can not be used uninitialized.

Closes GH-10966.

Merge branch 'PHP-8.1' into PHP-8.2

• PHP-8.1: Fix uninitialized variable accesses in sockets/conversions

Fix uninitialized variable accesses in sockets/conversions

This was first pointed out in GH-10959. The from_zval_... functions don't always write to the pointer, in particular it is necessary to check for an error before using the value. Otherwise we can access an uninitialized value and that's UB (and dangerous).

Note: this does NOT get rid of the compiler warning. Even though there is error checking now, the compiler isn't smart enough to figure out that the values can not be used uninitialized.

Closes GH-10966.

Fix uninitialized variable accesses in sockets/conversions

This was first pointed out in GH-10959. The from_zval_... functions don't always write to the pointer, in particular it is necessary to check for an error before using the value. Otherwise we can access an uninitialized value and that's UB (and dangerous).

Note: this does NOT get rid of the compiler warning. Even though there is error checking now, the compiler isn't smart enough to figure out that the values can not be used uninitialized.

Closes GH-10966.

Merge branch 'PHP-8.1' into PHP-8.2

• PHP-8.1: Fix uninitialized variable accesses in sockets/conversions

Merge branch 'PHP-8.2'

• PHP-8.2: Fix uninitialized variable accesses in sockets/conversions

@javiereguiluz Thanks for pointing that out. This patch also likely improves that, because in general this patch will give an improvement if a lot of files are loaded (e.g. web framework files). I'll take that repo into account for future measurements.

IMO yes we should get rid of the warning, I was already considering that. I also read the review comment on my PR now and proposed a solution for the warning: https://github.com/php/php-src/pull/10966#discussion_r1152117683. As far as I understand this is usually only done in master though and not for stable versions.

Description

Why do expressions match with && or || operator behave like the example? What is the reason for returning false?

The following code: https://3v4l.org/KktKB#v8.2.4

<?php

$result = match('test') {
    'test' && true => true,
    'test2' && true => true,
    default => false,
};

var_dump($result);

Resulted in this output:

bool(false)

But I expected this output instead:

bool(true)

PHP Version

8.2

Operating System

No response

I think you're misunderstanding the match arms. The match arms are evaluated as expression on their own. So when you write 'test' && true this gets evaluated to true, similar reasoning for 'test2' && true this also becomes true. Note that the conditional expressions are only evaluated if the previous matches failed. So you example code, after evaluating the arm expressions, is actually equivalent to:

<?php

$result = match('test') {
    true => true,
    true => true,
    default => false,
};

var_dump($result);

There is no arm that matches with an identity check (===) to 'test'. Therefore the default match arm is taken, which results in the value false as you saw in your code example. The fact that expressions are allowed in match arms allows for some dynamic computation of the arms.

I submitted a PR to fix the uninitialized accesses. However, this does not get rid of the compile warnings because now the compiler is just not smart enough to see the variables can no longer be used uninitialized.

This was first pointed out in GH-10959. The from_zval_... functions don't always write to the pointer, in particular it is necessary to check for an error before using the value. Otherwise we can access an uninitialized value and that's UB (and dangerous).

Note: this does NOT get rid of the compiler warning. Even though there is error checking now, the compiler isn't smart enough to figure out that the values can not be used uninitialized.