Paths

Table of Contentst

Differential D25182

Fix PHP 8.1 "strlen(null)" and "preg_match()" exceptions which block repository creation
ClosedPublic
Actions

Authored by aklapper on May 3 2023, 18:08.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15337: PHP 8.1 "strlen(null)" and "preg_match()" exceptions block repository creation

Commits

rP9f1814f490bf: Fix PHP 8.1 "strlen(null)" and "preg_match()" exceptions which block repository…

Summary

strlen() was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts phutil_nonempty_string() as a replacement.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Similarly, preg_match() does not accept passing null as $subject parameter since PHP 8.1.

Closes T15337

Test Plan

Applied these four changes and /diffusion/4/manage/ finally rendered in web browser.

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25182

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 375
Build 375: arc lint + arc unit

Event Timeline

aklapper created this revision.May 3 2023, 18:08

Owners added a reviewer: O1: Blessed Committers.May 3 2023, 18:08

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 3 2023, 18:08

aklapper requested review of this revision.May 3 2023, 18:08

Harbormaster completed remote builds in B348: Diff 641.May 3 2023, 18:09

According to phlog, both $content_type and $request_type in DiffusionServeController.php are null. It is unclear to me why the HTTP Header sent by my web browser is not read.

In D25182#5597, @aklapper wrote:

According to phlog, both $content_type and $request_type in DiffusionServeController.php are null. It is unclear to me why the HTTP Header sent by my web browser is not read.

It would be interesting to discover if it has to do with

Browser → Apache/nginx → PHP-FPM

Firefox; Apache HTTPD; php-fpm here.

valerio.bozzolan added inline comments.May 4 2023, 08:19

src/applications/diffusion/controller/DiffusionServeController.php
45–47	To avoid null, interestingly there is a `$default` parameter

Also here I will start approving as behalf of myself, but not as O1 since they would very probably share my very same tips in the "Not Done" comments.

Note that: if you have no time I can surely amend for you (you will still keep full authorship)

Thanks again :)

src/applications/diffusion/controller/DiffusionRepositoryManagePanelsController.php
43	✅ I verified the above line The `$selected` variable assumes the names of the panels like `"uris"` or `"limits"` etc., default to `null`. The function `phutil_nonempty_string()` will report alien types, and that is OK.
src/applications/diffusion/controller/DiffusionServeController.php
47	⚠ Here I suggest to set a default empty string Since, interestingly, removing the user agent in my browser caused our well known server crash in PHP 8.1 Here how I tested this: https://superuser.com/q/692530/390314
src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php
222	✅ I verified the above line The `$callsign` is really always a string or null. The function `phutil_nonempty_string()` will report any alien type, and that is OK.
src/applications/repository/editor/PhabricatorRepositoryEditor.php
53	✅ I verified the above line The `$local_path` assumes strings like `/var/repo/9/` or null. The function `phutil_nonempty_string()` will report any alien type, and that is OK.

valerio.bozzolan awarded a token.May 5 2023, 12:58

In D25182#5597, @aklapper wrote:

$content_type and $request_type ... are null. It is unclear to me why the HTTP Header sent by my web browser is not read.

In short you are OK

I deployed a PHP FPM locally and I can reproduce your situation. The good news is: that is a feature, and it seems OK.

My brain is exploding but I'm quite sure we are assisting to this magic:

http://www.ietf.org/rfc/rfc3875

In particular, it SHOULD remove any
   header fields carrying authentication information, such as
   'Authorization'; or that are available to the script in other
   variables, such as 'Content-Length' and 'Content-Type'.  The server
   MAY remove header fields that relate solely to client-side
   communication issues, such as 'Connection'.

If you want to test a non-null Content-Type etc. I can suggest to just adopt the quick & dirty mod_php - or - heavily hammer your local stuff with unknown dark magic in ModRewrite or Proxy or I don't know :D

Set default empty strings for $content_type and $user_agent, as requested by valerio.bozzolan

Harbormaster completed remote builds in B375: Diff 681.May 7 2023, 11:14

Thanks

I invested so much energies in this patch in trying some facets, and I think I really cannot verify more stuff without becoming completely insane.

I just reasonably rely on the fact that no weird objects are incoming, and we handle just strings or null. This is really the minimal change to make that page operational at least for visitors.

The function phutil_nonempty_string() will report any alien type, and that is OK here.

Green light from me and thanks for your fixes

sgtm