Nightlies for security branches?

[cmb69]

Description

When I applied #16097 the other day, I was prepared to see many of our 8.1 nightly jobs fail (Linux due to not yet applied #16107, and macOS due to some build error). Checking the builds today revealed that Linux already fails due to a missing backport of #16011 (or some alternative solution), and the some of the Community steps are also failing (apparently, due to newer versions of the libraries/frameworks/tools, which are no longer compatible with PHP 8.1).

It is obviously hard to keep these builds green, if the run only occasionally (often not for weeks), and @iluuu1994 convinced me that nightly runs of all branches would be too much. And it also might not be the best idea to target security branches for all potentially relevant CI fixes. While something like #16144 (if targeted at PHP-8.1) might help, it may still be a lot of work to cater to all those issues.

So I wonder what to do. It seems to me that the Community builds don't make much sense anyway (since PHP-8.1 should be stable enough to not introduce unexpected breaks). And assuming the we'll merge #16148 (which I'm planning to do), we already have the ability to run the push workflows, which should catch the most serious issues. Okay, no Asan and MSan builds, and no uncommon configurations, but these should ideally be run before we merge security fixes anyway.

So I tend to suggest to exclude PHP-8.1 from the nightly runs. If not, we should at least resolve the most serious issues, so that the nightly runs are somewhat meaningful.

[iluuu1994]

I agree that running old branches in nightly is meaningless if they are all red. I would not object to trying to fix the 8.1 issues now and seeing how much work it is to keep them green (or mostly green, at least). It could make sense to run all branches on Monday, for example. Then, we would find new issues on 8.1 every week instead of every few months. As mentioned previously, the configurations for the branches are diverging, which is one of the main annoyances. I have not yet looked into shared workflows, which might be a solution. Another might be always checking out ./.github from master, so that we can manage it in one place.

Community builds don't make much sense anyway (since PHP-8.1 should be stable enough to not introduce unexpected breaks).

Note that community builds run with ASan, so they are actually quite useful. It is possible though that they start dropping PHP versions that still receive security patches, in that case it would definitely make sense dropping them.

[iluuu1994]

I brought this up in todays foundation meeting. Here's the summary:

We should strive for CI of security branches to stay green. Fixes to CI itself and tests should target security branches from now on. Small code fixes may be discussed on a case-by-case basis with RMs of the given security branch. Alternatively, the given errors/warnings may be ignored with -Wno- flags. Security branches should be run in the nightly workflow on a weekly basis.

Does this sound ok to you? I will now work on making the builds green, and then update the #general channel on Slack to inform everybody to send CI patches (including fixing of tests) to security branches from now on.

[cmb69]

That sounds great!

[nielsdos]

Is this resolved now by 562677a ? I would think so unless I'm missing something?

[cmb69]

I think so. Maybe @iluuu1994 wants to keep it open for a while to check whether everything works as expected.

[iluuu1994]

I think other builds still fail (freebsd). Unfortunately, I can't check as we're out of free credits.

[cmb69]

FreeBSD has been switched to GH (using emulation), and that works fine. However, macOS fails despite #16789, and despite that icu4c@74 is in PKG_CONFIG_PATH but not icu4c@76 (and the latter is not linked either).

[iluuu1994]

@cmb69 Any suggestions on that part? We should also backport all of the flakiness checks containing the message "Occasionally segfaults on macOS for unknown reasons", as this seems to happen very frequently. The last issue is a JIT issue in the community build for one of the Symfony packages, which was fixed for stable branches in GH-16858. I just asked the RMs whether it's ok to backport it. If not, I will skip the affected package.

[cmb69]

No idea how to solve this. And even if we solve this, there will be new issues (at least when icu@74 will be no longer supported by Homebrew in May).

[iluuu1994]

@cmb69 So, drop intl from the build?

[cmb69]

Or build ICU 74.2 manually, and cache it.

[iluuu1994]

@cmb69 But if PHP 8.1 only works with ICU 74, and homebrew drops support for ICU 74, then what's the point of testing it? I highly doubt macOS folks will compile ICU on their own.

[cmb69]

I really don't know how people work on macOS, but there are other package managers, and it might even possible to simply avoid the updates. I'm also fine with dropping ext/intl from macOS CI builds, but I guess that RMs should have a say about this.@patrickallaert, @ramsey, thoughts?

[ramsey]

despite that icu4c@74 is in PKG_CONFIG_PATH but not icu4c@76 (and the latter is not linked either)

I've been struggling with this locally. Something is still referencing icu4c@76 somehow. In the compilation logs, I continue to see attempts to use the 76 headers instead of the 74 ones.

What would cause this?

[iluuu1994]

and it might even possible to simply avoid the updates

I don't use homebrew anymore, but I specifically remember running into major issues because homebrew removes unsupported packages immediately. When just installing a new package, homebrew would also update all of the installed packages, and even removed unsupported ones. OpenSSL specifically was an issue. I wouldn't count on things continuing to work once the package is removed.

[patrickallaert]

I really don't know how people work on macOS, but there are other package managers, and it might even possible to simply avoid the updates. I'm also fine with dropping ext/intl from macOS CI builds, but I guess that RMs should have a say about this.@patrickallaert, @ramsey, thoughts?

No opinion here, also never ever used a Mac. @ramsey can have a final word here.

[ramsey]

I really don't want to drop ext/intl from macOS builds, but this is a big PITA for building PHP 8.1 on macOS. I haven't heard a lot of complaints about this, though, so maybe there aren't enough folks still building PHP 8.1 on macOS, which means we can probably drop this from the macOS CI builds.

[bukka]

I would prefer to patch it. It's really small change and we have already done something similar for libxml . I don't see an issue to introduce fixes of this kind to security only branches. We should probably update our policy though.

[bukka]

Hmm looking at the intl ext, it seems to me like updating might be a bit more effort and possibly something that should not be done in 8.1. We could potentially build our own version but it might be too much effort. I think it might be best to drop it from MacOS build and look into it only if we have some security issue in intl extension (which is not that common so it might not actually happen).

[bukka]

@iluuu1994 I assume this can be now closed, right?

[iluuu1994]

I suppose, although it's not fully solved yet. I think the community tests are inherently problematic. For example, Symfony seems to have already dropped 8.1 support on their default branch (7.3). It also still errors with asan, and new issues have also popped up. Not sure what the best way forward is for those. ext/fileinfo/tests/cve-2014-3538-nojit.phpt now apparently also fails on macOS release builds (or we got unlucky with the same flaky test failing twice, not sure, https://github.com/php/php-src/actions/runs/12540512801/job/34967916493).

[bukka]

Maybe we could use some stable versions that till supports 8.1 for community tests? Not sure why ext/fileinfo/tests/cve-2014-3538-nojit.phpt does not work. I would expect it to still work so it might be a problem with the test but don't know that much about fileinfo ext.

[cmb69]

Not sure why ext/fileinfo/tests/cve-2014-3538-nojit.phpt does not work.

The test is performance sensitive, and terminates after 1 second. Perhaps just a timing issue (especially when running tests in parallel).

[iluuu1994]

Maybe we could use some stable versions that till supports 8.1 for community tests?

We could switch to Symfony LTS (6.4) for 8.1. This may also become an issue for other libraries, so it may become another slightly annoying maintenance burden.

[bukka]

So I think we should create a new issue for ext/fileinfo/tests/cve-2014-3538-nojit.phpt.

Then we should switch the community tests to stable versions and all stable PHP branches because that's what we don't want to break most. Master could stay on the latest so it would just require some steps to add before releasing the stable so we don't forget about switching that to stable (LTS) version. What do you think?

The only issue is that running nightly on final branch (PHP-8.1 in this case) is a bit late because it doesn't allow to check the changes before they are released. I usually create special branches for RM's to merge in so maybe we could set some filter so nightly run on them in our security repo. But that's more an improvement.

[cmb69]

The only issue is that running nightly on final branch (PHP-8.1 in this case) is a bit late because it doesn't allow to check the changes before they are released.

Well, depends on what we regard as "released". We usually tag on Tuesday (yeah, timezones), so it should run Tuesday or Wednesday night, but official release is on Thursday.

Also, you can trigger nightly CI manually (see https://github.com/php/php-src/actions/workflows/root.yml).

[bukka]

Well, depends on what we regard as "released". We usually tag on Tuesday (yeah, timezones), so it should run Tuesday or Wednesday night, but official release is on Thursday.

That's not enough time to fix anything and the tag is already created so it requires a new tag. On top of that we really need to get rid of this delay as it's terrible for security (we basically make the fixes public but don't annouce them).

Anyway it's not a big deal, it just need to tweak a bit filter - just saying that as a nice possible improvement. As you say it can be done manually in the meantime but would be nice if it would run automatically on those branches.

[cmb69]

On top of that we really need to get rid of this delay as it's terrible for security

Yeah, but besides that the Windows builds would need to be fully automated, release managers may have a hard time; different branches may need the same fix, but RMs may live all around the globe.

[bukka]

Yeah, but besides that the Windows builds would need to be fully automated, release managers may have a hard time; different branches may need the same fix, but RMs may live all around the globe.

Yeah Windows part needs to be done first ofc. That should help to reduce this delay to a single day which would be already a big improvement (we would get from days to hours which is much better). Syncing the branches to be released at the same time would be harder and that's probably for another discussion. A bit off topic here though.

[iluuu1994]

I consulted Dmitry regarding the JIT leaks in 8.1 and he said he preferred not to fix them. There are many JIT changes in 8.2 and backporting them risks introducing new issues. So, I think our only option for now is to disable the problematic steps in the community build. After that, we can close this issue.

[iluuu1994]

So I think we should create a new issue for ext/fileinfo/tests/cve-2014-3538-nojit.phpt.

#17600

Done. I also added the changes to skip the Symfony/Wordpress community builds on 8.1, and fixed some CircleCI tests on 8.1.