Page MenuHomePhorge

Fix PHP 8.1 "preg_match()" exception when pasting malformed Raw Diff into "Create Diff"
ClosedPublic

Authored by aklapper on May 3 2023, 19:51.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 17:32
Unknown Object (File)
Fri, Mar 8, 11:41
Unknown Object (File)
Feb 10 2024, 11:36
Unknown Object (File)
Feb 9 2024, 03:39
Unknown Object (File)
Feb 9 2024, 03:38
Unknown Object (File)
Feb 9 2024, 02:59
Unknown Object (File)
Feb 9 2024, 02:51
Unknown Object (File)
Jan 28 2024, 22:21

Details

Summary

Entering a malformed string (like "pHoRgE rOcKs!!") into the "Raw Diff" field, Arcanist's tryMatchHeader() function is first called in $ok = $this->tryMatchHeader($patterns, $line, $match) with a non-null $line value (the first line entered in the "Raw Diff" field) being passed.

Afterwards, tryMatchHeader() is called for a second time after assigning $line = $this->nextLineThatLooksLikeDiffStart().
This time $line is null and a RuntimeException is thrown, as tryMatchHeader() calls preg_match() which does not accept passing null as the $subject string parameter in PHP 8.1.

Thus add a phutil_nonempty_string() check if the $subject parameter (in this case, $line) is a non-empty string.

Arcanist's tryMatchHeader() function is not called outside of the file in which it is defined.
Thus catch the exception in the second call to tryMatchHeader() and not in the code of the tryMatchHeader() function itself.

Closes T15338

Test Plan

After adding the additional check, /differential/diff/create/ showed the expected Diff Parse Exception instead.

Diff Detail

Repository
rARC Arcanist
Branch
createDiff (branched from master)
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 349
Build 349: arc lint + arc unit

Unit TestsFailed

TimeTest
1,025 msArcanistBundleTestCase::testGitRepository
EXCEPTION (RuntimeException): strlen(): Passing null to parameter #1 ($string) of type string is deprecated #0 /var/www/html/phorge/arcanist/src/parser/ArcanistBundle.php(798): PhutilErrorHandler::handleError(8192, '...', '...', 798) #1 /var/www/html/phorge/arcanist/src/parser/ArcanistBundle.php(409): ArcanistBundle->buildBinaryChange(Object(ArcanistDiffChange), NULL)
0 msArcanistBaseCommitParserTestCase::testBasics
2 assertion(s) passed.
0 msArcanistBaseCommitParserTestCase::testHalt
1 assertion(s) passed.
1 msArcanistBaseCommitParserTestCase::testJump
1 assertion(s) passed.
0 msArcanistBaseCommitParserTestCase::testJumpReturn
1 assertion(s) passed.
View Full Test Results (1 Failed · 74 Passed)

Event Timeline

aklapper requested review of this revision.May 3 2023, 19:51

Ouch reading this description I feel it was a painful troubleshooting

Thank you so much also for this patch,

I tested this locally, examining $line with pht() and testing both in PHP 7.4 and PHP 8.1, following the test plan and some other things, without any unexpected nuclear implosion.

The function phutil_nonempty_string() will report any value that is not null and is not a string, and that is OK.

Green light

sgtm

This revision is now accepted and ready to land.May 4 2023, 06:42
This revision was landed with ongoing or failed builds.May 4 2023, 08:56
This revision was automatically updated to reflect the committed changes.