Completely unnecessary compiler warnings in function ‘from_zval_write_sockaddr_aux’ #10959

VARGA-Peter · 2023-03-28T10:56:21Z

### 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

The text was updated successfully, but these errors were encountered:

Girgias · 2023-03-28T14:05:41Z

I'm not hitting those bogus compiler warnings on Fedora 37 and GCC 12.2.1 nor Clang 15. What is the full GCC compiler version you are running?

VARGA-Peter · 2023-03-28T19:01:37Z

I'm not hitting those bogus compiler warnings on Fedora 37 and GCC 12.2.1 nor Clang 15. What is the full GCC compiler version you are running?

-v options returns this:

Using built-in specs.
COLLECT_GCC=gcc-12
COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-suse-linux/12/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info --mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 --enable-languages=c,c++,objc,fortran,obj-c++,ada,go,d --enable-offload-targets=nvptx-none, --enable-offload-defaulted --without-cuda-driver --enable-checking=release --disable-werror --with-gxx-include-dir=/usr/include/c++/12 --enable-ssp --disable-libssp --disable-libvtv --enable-cet=auto --disable-libcc1 --disable-plugin --with-bugurl=https://bugs.opensuse.org/ --with-pkgversion='SUSE Linux' --with-slibdir=/lib64 --with-system-zlib --enable-libstdcxx-allocator=new --disable-libstdcxx-pch --enable-libphobos --enable-version-specific-runtime-libs --with-gcc-major-version-only --enable-linker-build-id --enable-linux-futex --enable-gnu-indirect-function --program-suffix=-12 --without-system-libunwind --enable-multilib --with-arch-32=x86-64 --with-tune=generic --build=x86_64-suse-linux --host=x86_64-suse-linux
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 12.2.1 20220830 [revision e927d1cf141f221c5a32574bde0913307e140984] (SUSE Linux)

nielsdos · 2023-03-28T21:10:58Z

I think the warnings are true positives.
from_zval_write_int will not write to the pointer on error or an int that's out of the [INT_MIN, INT_MAX] range. In those cases the value will remain uninitialised.

This was first pointed out in phpGH-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.

nielsdos · 2023-03-28T21:42:25Z

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.

VARGA-Peter · 2023-03-29T15:12:38Z

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.

I understand, but don't you think that just a dummy initialisation - while the declaration - should be done, just to get rid of the warning? It's always good to have no warning even you know the compiler isn't smart enough to analyse the flow correctly?

nielsdos · 2023-03-29T15:21:58Z

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: #10966 (comment).
As far as I understand this is usually only done in master though and not for stable versions.

VARGA-Peter · 2023-03-29T16:38:14Z

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.

OK, I now checked the function from_zval_write_uint32() and I think it has nothing to do, if the compiler is smart or not. Definitely, as written above, the additional check [if (ctx->err.has_error)] must be done and till now it lead to UB. So the compiler warning revealed this bug.

Now, the situation changed and I don't think that the compiler can see that the added code fixed a bug and now indeed the variable cannot be accessed uninitialised - due to the new check. Maybe gcc 16 is that smart but there remains always the problem that the function is in another file, so the compiler doesn't have enough information. A conditional meta data informing the compiler when the variable is written could solve the problem but this is something for the future...

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.

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

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

VARGA-Peter added Bug Status: Needs Triage labels Mar 28, 2023

VARGA-Peter changed the title ~~Completely unnecessary compiler warnings~~ Completely unnecessary compiler warnings in function ‘from_zval_write_sockaddr_aux’ Mar 28, 2023

nielsdos mentioned this issue Mar 28, 2023

Fix uninitialized variable accesses in sockets/conversions #10966

Closed

nielsdos added a commit to nielsdos/php-src that referenced this issue Mar 29, 2023

Silence compiler warnings in ext/sockets/conversions.c

a668920

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

nielsdos mentioned this issue Mar 29, 2023

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

Merged

nielsdos closed this as completed in #10974 Mar 29, 2023

nielsdos added a commit that referenced this issue Mar 29, 2023

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

b606e44

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Completely unnecessary compiler warnings in function ‘from_zval_write_sockaddr_aux’ #10959

Completely unnecessary compiler warnings in function ‘from_zval_write_sockaddr_aux’ #10959

VARGA-Peter commented Mar 28, 2023 •

edited

Loading

Girgias commented Mar 28, 2023

VARGA-Peter commented Mar 28, 2023 •

edited

Loading

nielsdos commented Mar 28, 2023

nielsdos commented Mar 28, 2023

VARGA-Peter commented Mar 29, 2023 •

edited

Loading

nielsdos commented Mar 29, 2023

VARGA-Peter commented Mar 29, 2023

Completely unnecessary compiler warnings in function ‘from_zval_write_sockaddr_aux’ #10959

Completely unnecessary compiler warnings in function ‘from_zval_write_sockaddr_aux’ #10959

Comments

VARGA-Peter commented Mar 28, 2023 • edited Loading

Girgias commented Mar 28, 2023

VARGA-Peter commented Mar 28, 2023 • edited Loading

nielsdos commented Mar 28, 2023

nielsdos commented Mar 28, 2023

VARGA-Peter commented Mar 29, 2023 • edited Loading

nielsdos commented Mar 29, 2023

VARGA-Peter commented Mar 29, 2023

VARGA-Peter commented Mar 28, 2023 •

edited

Loading

VARGA-Peter commented Mar 28, 2023 •

edited

Loading

VARGA-Peter commented Mar 29, 2023 •

edited

Loading