Fix DifferentialGetCommitMessageConduitAPIMethod execute strlen(null)
ClosedPublic
Actions

Authored by Sten on Jul 5 2023, 08:16.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15527: arc diff <differential.getcommitmessage> strlen(null) PHP 8.1 error

Commits

rPb3ac5ceb387f: Fix DifferentialGetCommitMessageConduitAPIMethod execute strlen(null)

Summary

When iterating through the fields of a differential commit, the DifferentialGetCommitMessageConduitAPIMethod execute method explicitly allows a value to be either a string or a null. It then calls strlen upon this possibly null value.

We could replace the strlen with phutil_nonempty_string, but as the code has already eliminated variable types other than string or null, it is more efficient to explicitly check for null or ''

$value === null or $value == ''

Fixes T15527

Test Plan

Run

arc diff

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Sten created this revision.Jul 5 2023, 08:16

Owners added a reviewer: O1: Blessed Committers.Jul 5 2023, 08:16

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJul 5 2023, 08:16

Sten requested review of this revision.Jul 5 2023, 08:16

Harbormaster completed remote builds in B637: Diff 1072.Jul 5 2023, 08:16

valerio.bozzolan added inline comments.Jul 5 2023, 08:25

src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php
107	Nice, at this point `$value` MUST be a string or NULL <3
118	Unfortunately `$value == ''` involves a PHP pitfall that causes the exclusion of a commit message consisting in the string "`0`". THE COMMENT "0" IS TOTALLY AWESOME AND I MAKE IT ALL THE TIME SO YOU HAD BETTER NOT BREAK IT!!! https://we.phorge.it/book/flavor/article/php_pitfalls/ Also fortunately, at this point, `$value` must be a string or null so we can just do a lovely native strict check, to exclude the case of an empty string. Another possibility: feel free to adopt `if(!phutil_nonempty_string($value))` but I'm not a fan of "not non-empty" conditions.

Update empty string check to === '' as per review

Harbormaster completed remote builds in B650: Diff 1087.Jul 5 2023, 16:21

Wouldn't want to deny you your '0' comments!

Tested, thanks! I was able to debug both NULL, empty string, and a populated string, just putting some stuff in the fields.

For the records, note again that !is_string($value) && !is_null($value) at the top, that is the reason why here we can avoid the strlen() at all, since we never handle objects, or weird stuff to be casted to string. It's just a string, or NULL. And the only string with zero bytes is ''.

sgtm