Fix #78919: CLI server: insufficient cleanup if request startup fails

[cmb69]

We need to run the full php_cli_server_request_shutdown() in case of
failing php_cli_server_request_startup().

Patch contributed by @cataphract.

[nikic]

Not really sure about this one. It generally makes sense to me, but I'm not sure it's safe for all possible startup failure points. I think most places bail entirely on request startup failure (and tbh it's not clear how we can hope to continue in that case -- pray that the new request startup is magically not going to fail as well?)

[cataphract]

Thanks for looking into this.

tbh it's not clear how we can hope to continue in that case -- pray that the new request startup is magically not going to fail as well?

@nikic What we're doing in the extension is that we're looking at some properties of the request itself (say, request uri) and deciding whether to block the request; this means preventing the PHP script from running. One of the possible strategies is to raise an error. Unfortunately, this has different behaviors across the SAPIs: works well in the Apache SAPI, as described in the bug report, FPM just exists the process, which is terrible for performance, and requires a different strategy, and the CLI SAPI leaks memory.

I'm not sure it's safe for all possible startup failure points
I can't be sure as well; for our purposes we need only recover from the error that we emit. But note that, as I said above, the Apache SAPI does call php_request_shutdown.

[cmb69]

If there are concerns about that, should we apply the patch to the "master" branch only?

[cmb69]

Well, the partial shutdown as implemented makes no sense. A bailout might, but I think @cataphract has a point that this can be a valid hook to block certain requests. And even if all request startups would fail, we keep a clean state, even if the built-in Webserver is basically unusable, and the user would have to shut it down manually.

[nikic]

Let's give this a try for master.

It looks like at least we don't try to run RSHUTDOWN if any RINIT failed (because then modules_activated=0), so this looks reasonably safe.