Add ASan and UBSan options #6825

twose · 2021-04-01T08:47:18Z

When we need to enable ASan/UBSan in PHP extension, we also need to enable it in PHP kernel. Providing these options makes it easier for us to enable ASan/UBSan when compiling PHP.
And we use --enable-address-sanitizer and --enable-undefined-sanitizer in azure-pipelines.yml instead of changing CFLAGS.
We also add compile flag check to MSan.

dktapps · 2021-04-07T11:34:13Z

shouldn't it be ARG_WITH() instead of ARG_ENABLE()? ASan requires additional libraries to be linked.

nikic · 2021-04-07T15:07:06Z

enable is preferred here. More generally, we'd like them to be interchangeable (php/php-tasks#2).

configure.ac

dixyes · 2021-04-13T06:02:36Z

Windows build system already have a "--enable-sanitizer" option, but only for MSVC clang toolchain.

Shall we use "--enable-sanitizer" as option name, or use something like "--enable-sanitizer=yes,a,b,c“ ?

The command to configure it may be like:

REM at windows
configure.bat --enable-zts --enable-debug --disable-all --enable-cli --enable-sanitizer
REM or
configure.bat --enable-zts --enable-debug --disable-all --enable-cli --enable-sanitizer=address,undefined
REM or
configure.bat --enable-zts --enable-debug --disable-all --enable-cli --enable-sanitizer=yes,address,undefined
REM note: from VS 2019 16.9, MSVC native CL.exe starts supporting ASan natively

: at unix-like
./configure --enable-zts --enable-debug --disable-all --enable-cli --enable-sanitizer
: or
./configure --enable-zts --enable-debug --disable-all --enable-cli --enable-sanitizer=address,undefined
: or
./configure --enable-zts --enable-debug --disable-all --enable-cli --enable-sanitizer=yes,address,undefined

azure-pipelines.yml

nikic · 2021-04-13T07:56:39Z

Shall we use "--enable-sanitizer" as option name, or use something like "--enable-sanitizer=yes,a,b,c“ ?

Is there any advantage to combining all the options? It seems like same result for more parsing/diagnostic effort.

A blanket --enable-sanitizer option without specifying which sanitizers should be used wouldn't work because some of them are mutually exclusive (memory and address).

dixyes · 2021-04-13T08:03:16Z

Shall we use "--enable-sanitizer" as option name, or use something like "--enable-sanitizer=yes,a,b,c“ ?

Is there any advantage to combining all the options? It seems like same result for more parsing/diagnostic effort.

A blanket --enable-sanitizer option without specifying which sanitizers should be used wouldn't work because some of them are mutually exclusive (memory and address).

We can use same options among Windows/unix-like build system: something like "--enable-zts" vs "--enable-maintainer-zts" seems not good.
Windows build system now will enable "address" and "undefined" sanitizers when enabling blanket --enable-sanitizer option, this behavior should be kept.

configure.ac

When we need to enable ASan/UBSan in PHP extension, we also need to enable it in PHP kernel. Providing these options makes it easier for us to enable ASan when compiling PHP. And we use --enable-address-sanitizer and --enable-undefined-sanitizer in azure-pipelines.yml instead of changing CFLAGS. We also add compile flag check to MSan.

morrisonlevi · 2021-04-24T02:35:23Z

Does enabling sanitizers here automatically add sanitizer settings to extensions built with a sanitizer-enabled PHP? A quick glance at the patch suggests no, but I didn't check it out to find out.

I am asking because if it does then I am quite happy -- this makes it a lot easier to build whole PHP ecosystems with sanitizers. If it does not, I would prefer that we revert these changes. Recommendations for options to use with sanitizers change, and new sanitizer types are added. I don't think special casing these ones is worth any sort of maintenance unless these propagate out into the extension ecosystem.

nikic reviewed Apr 7, 2021

View reviewed changes