releases property attributes of internal classes #11980
Conversation
|
The same fix is in a PR which I opened a short while ago :) e1b618e |
Looks like our interests converge! 😉 Anyway, a search for |
|
@ju1ius Thank you for the change and the test! I can't reproduce the ASAN failure locally on Clang 16 or GCC. CI runs with Clang 14, it's probable that this is a bug in Clang 14. We may try to update Ubuntu 22.10 which has Clang 15. I'm not sure if this issue is fixed there. |
I'm using Clang 14.0.6 locally (from debian unstable repos) and cannot reproduce either (it fails for another unrelated test though... 🤷🏻). I didn't dig much into the CI configs so mine might slightly differ. |
|
@ju1ius Did you configure with |
ju1ius
force-pushed
the
ju1ius/free-prop-attrs2
branch
from
8c597ae to
b24f7df
Compare
Ah, no. It seemed to me that
Nope, Debian Sid. Long story short, I'm writing a library to write PHP modules in Rust and building with clang+lldb allows for cross-language step-debugging sessions (it might also be possible with gcc but I couldn't make it work...). |
|
Hmm, weird. I thought -fsanitize=address would work. Clang required the same for LDFLAGS, but omitting that would result in a linker error instead. The benefit of using the configure flag is just that it doesn't use that CFLAG for the various autoconf tests, which can significantly slow them down.
Cool cool 😎 |
|
So I had another go: This is my PHP build script#!/bin/bash
make distclean
git clean -fx
./buildconf
declare -a CC_FLAGS=(
-gdwarf-4
-ggdb3
-glldb
-DZEND_RC_DEBUG=1
-DTSRM_DEBUG=1
-DZEND_TRACK_ARENA_ALLOC
-fsanitize=undefined,address
)
declare -a CONFIG_FLAGS=(
--enable-debug
--enable-zts
--enable-address-sanitizer
--enable-zend_test=shared
--enable-opcache
--disable-opcache-jit
--enable-tokenizer
--enable-mbstring
--enable-dom
--with-libxml
--enable-xml
--enable-xmlwriter
--enable-phar
--disable-cgi
--disable-fpm
)
./configure \
CC=clang \
CFLAGS="${CC_FLAGS[*]}" \
"${CONFIG_FLAGS[@]}"
make -j "$(nproc)"The command to run the test case failing in CI:./sapi/cli/php -P -q \
-d "extension_dir=$(pwd)/modules" \
-d opcache.enable_cli=1 \
--asan \
-x -j8 \
./sapi/cli/tests/ext_loading.phptProduces the following outputTL;DR The test passes, ASAN reports an UAF when trying to free _ZendTestClass on shutdown, and an But nothing that looks like the CI failure... clang 14.0.6 btw |
|
Also I just rebased this PR, and now the ASAN build fails but for another set of tests (under |
|
I'm afraid there's some random component. Can you try switching to Ubuntu 22.10 or 23.04? Just to see if that avoids the issue. |
I'm afraid not since I'm not on Ubuntu in the first place and don't intend to switch to it. 😆 I can try with another clang version though. |
|
Sorry, I meant in .github/workflows/push.yml of course 😄 |
Yeah I was afraid you'd say that... |
It seems that 22.04 is the highest available version on GH actions.... I can try with ubuntu-latest maybe? |
🙃
In that case, |
Maybe trying to switch to gcc? Or install a custom LLVM toolchain... |
|
@ju1ius Sure, for testing that makes sense. I used clang because it produces a significantly faster asan debug build. Tests take much longer to finish on gcc + asan. |
There's KyleMayes/install-llvm-action |
Nope, same results w/ GCC, and w/ the same set of test cases... At least it's not so random.... |
|
So, after reading through those seemingly related issues:
I tried setting The build failed due to detected leaks: But this time LSAN did not crash... I have absolutely no idea of what this option really does, nor if it is really OK to use it (especially since the ASAN build runs under ZTS which does use TLS), but it seems to be a commonly adopted workaround for this issue, so if somebody would like to take over... |
|
Great find! Reading https://maskray.me/blog/2023-02-12-all-about-leak-sanitizer, it sounds like setting Edit: I missed that this shows leaks for us. I'll look at those more closely in a couple of hours. |
diff --git a/.github/lsan-suppressions.txt b/.github/lsan-suppressions.txt
index b8f863ce60..9278992166 100644
--- a/.github/lsan-suppressions.txt
+++ b/.github/lsan-suppressions.txt
@@ -1,3 +1,4 @@
leak:acommon::DictInfoList::elements
leak:timer_create
leak:netsnmp_init_mib_internals
+leak:dlopen
diff --git a/run-tests.php b/run-tests.php
index 6f10e4090b..01cae7e44a 100755
--- a/run-tests.php
+++ b/run-tests.php
@@ -586,7 +586,7 @@ function main(): void
$lsanSuppressions = __DIR__ . '/.github/lsan-suppressions.txt';
if (file_exists($lsanSuppressions)) {
$environment['LSAN_OPTIONS'] = 'suppressions=' . $lsanSuppressions
- . ':print_suppressions=0';
+ . ':print_suppressions=0:use_tls=0';
}
break;
case '--repeat':Can you try it with this? I guess |
@iluuu1994 Hmmm... Unless I'm missing something, I can't see how this is any different from what I did in 8b2998b? |
|
@ju1ius It's missing the |
Aaah, right my bad, here we go. |
|
@ju1ius Lead cannot be suppressed 😉 |
ju1ius
force-pushed
the
ju1ius/free-prop-attrs2
branch
from
1fa9b73 to
3a3337d
Compare
|
@ju1ius This seems to break when debug symbols for the dynamic lib are stripped. Can you please try adding |
ju1ius
force-pushed
the
ju1ius/free-prop-attrs2
branch
from
3a3337d to
e265ce7
Compare
|
Seems to do the trick but we still have |
|
@ju1ius Ugh. Another approach we could try is wrapping the responsible code in |
Doesn't bother me but at that point it's just gonna be you telling me what to do, so you'll probably be much faster on your own... As you wish. |
|
@ju1ius I will investigate, thanks for your help so far! |
ju1ius
force-pushed
the
ju1ius/free-prop-attrs2
branch
from
bc1b8f5 to
ed16b07
Compare
|
@ju1ius Can you rebase this again? Hopefully the error is gone now. |
ju1ius
force-pushed
the
ju1ius/free-prop-attrs2
branch
from
ed16b07 to
d3531da
Compare
|
Thank you @ju1ius! |
* adds test case for internal class property attribute * releases property attributes of internal classes
Closes #11979