Page MenuHomePhorge

Fix PHP 8.1 arc patch strlen(null) binary file error
ClosedPublic

Authored by Sten on Aug 18 2023, 09:02.
Tags
None
Referenced Files
F2182772: D25409.id.diff
Wed, May 8, 07:59
F2182667: D25409.diff
Wed, May 8, 07:32
Unknown Object (File)
Thu, May 2, 09:58
Unknown Object (File)
Thu, May 2, 09:58
Unknown Object (File)
Thu, May 2, 09:58
Unknown Object (File)
Thu, May 2, 09:58
Unknown Object (File)
Wed, May 1, 16:58
Unknown Object (File)
Thu, Apr 11, 23:19

Details

Summary

Fix issue in arcanist whereby when doing an arc patch involving adding or removing a binary file, it falls over with strlen(null) errors.

Fixes T15617

Test Plan

arc patch Dxxxx

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Sten requested review of this revision.Aug 18 2023, 09:02
src/parser/ArcanistBundle.php
752

Note: Changing the default to '' doesn't work, because $old_data might be set to null by $this->getBlob() on line 762.

765

Note: $old_data is explicitly allowed to be null, as per line 781 below.

790

Note: as with $old_data, $new_data can be set to null on line 795 below, so changing the default to '' here doesn't work.

phutil_nonempty_string is basically the exact same thing, but with stricter type control

src/parser/ArcanistBundle.php
765
This comment was removed by Sten.

phutil_nonempty_string is basically the exact same thing, but with stricter type control

Right - but we are expecting only a null or a string. If we get anything else then I want strlen to fall over, following the fail fast principle.

I think phutil_nonempty_string fails faster then strlen (strlen allowing automatic conversion of string-like things to string and phutil_nonempty_string doesn't), but I'm a little confused right now...

Switch to using phutil_nonempty_string

I think phutil_nonempty_string fails faster then strlen (strlen allowing automatic conversion of string-like things to string and phutil_nonempty_string doesn't), but I'm a little confused right now...

/me checks phutil_nonempty_string()
OK - I was not expecting that side effect of throwing an exception if passed something other than a null or string. I would have expected it to return false.

Macro technicallycorrect:

This revision is now accepted and ready to land.Aug 18 2023, 13:36