Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exception which blocks rendering the Projects page (and log alien values)
ClosedPublic

Authored by aklapper on Apr 30 2023, 09:15.
Tags
None
Referenced Files
F2141697: D25153.id609.diff
Sat, Apr 13, 08:22
Unknown Object (File)
Thu, Apr 11, 05:26
Unknown Object (File)
Thu, Apr 11, 04:20
Unknown Object (File)
Sun, Apr 7, 22:06
Unknown Object (File)
Mon, Apr 1, 01:22
Unknown Object (File)
Mon, Apr 1, 00:34
Unknown Object (File)
Sun, Mar 31, 12:20
Unknown Object (File)
Wed, Mar 27, 15:48

Details

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 general replacement.

In this specific case we use phutil_nonempty_stringlike() since we are not sure
if the variable href should be just a string or other objects.

In order not to leave these cases to chance, we have added a log line, which can be
removed in the future.

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.

Closes T15303
Ref T15316

Test Plan

Applied this change (on top of D25144, D25145, D25146, D25147, D25150,
D25142) and /project/ rendered in web browser.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

href can be an object - PhutilUri, and maybe some other things.
what is this method for anyway? ie what usecases is a PhuiTag not know what kind of tag it is?

In D25153#4709, @avivey wrote:

href can be an object - PhutilUri, and maybe some other things.
what is this method for anyway? ie what usecases is a PhuiTag not know what kind of tag it is?

It seems a nice case for phutil_nonempty_stringlike() that - on objects - it calls the internal __toString()

But, it would be nice to add an inline PHPDoc for the href attribute to mention that thing like

/**
 * Destination URI
 * @var string|PhutilUri
 */
private $href;

I'd like to know more about why this method even exists, and what other things might end up in href.

maybe add call to phlog() if it's of an unexpected type.

In D25153#4715, @avivey wrote:

I'd like to know more about why this method even exists, and what other things might end up in href.

The thing is, before this change, strlen() was casting everything to string under the hood (so, objects were fine, if the developer implemented a __toString()).

After this change:

  • phutil_nonempty_string(): it requires to be null or a string, and not "" but does not accept any object - so it's not OK here
  • phutil_nonempty_stringlike(): it also accepts objects but only if you can cast them to a string (→ if the developer implemented a __toString()) and only if the result is not "" - so it should be 99.99% OK here

With this premise I'm 99% sure that phutil_nonempty_stringlike() is just what fits here. No need to phlog().

The phutil_nonempty_stringlike() already explodes by design if an object without a good __toString() is passed, creating a log line.

a phlog would be useful, because anything other than string or PhutilUri would be very surprising, and possibly a bug. My desire is to expose bugs as much as possible, not hide them.

In D25153#4717, @avivey wrote:

a phlog would be useful, because anything other than string or PhutilUri would be very surprising, and possibly a bug. My desire is to expose bugs as much as possible, not hide them.

I understand but in these cases the bug emerges because we can see the value of _toString() exactly as before.

I don't think Evan would put a phlog() in this position. But I don't have a strong opinion.

src/view/phui/PHUITagView.php
183

The value of href is passed as-is, and, the frontend just casts it to a string as expected:

function phutil_tag($tag, array $attributes = array(), $content = null) {
  // If the `href` attribute is present, make sure it is not a "javascript:"
  // URI. We never permit these.
  if (!empty($attributes['href'])) {
    // This might be a URI object, so cast it to a string.
    $href = (string)$attributes['href']; // ← ← ← ← ← ←
    if (isset($href[0])) { // (weird check) to check if the string it's not empty

So, whenever you have a string, or something that can be converted to a string, that value is OK.

I'd argue that almost any use of phutil_nonempty_stringlike() is hiding a bug, because it hides an object we don't know the type of - which is by itself a problem.

In D25153#4720, @avivey wrote:

I'd argue that almost any use of phutil_nonempty_stringlike() is hiding a bug, because it hides an object we don't know the type of - which is by itself a problem.

Yep I agree since that was also obviously true with strlen().

So if you want to proceed with a stricter check, I can suggest to implement that in the href setter method (to slow down once), and not in the getter (to slow down every read call)

putting it in the setter is fine...

src/view/phui/PHUITagView.php
123

AHAHAHAHAHAHAHAH WHAT THE F****

With much surprise, even if visiting 100+ pages and logging anything that is not null or a string, I was NOT able to log any object.

Hoping to be useful I propose a general logger in the setter without even checking for a particular instanceof something. And, I think we can keep the phutil_nonempty_string() and just fix on any incoming report.

log every alien class in href

I accept this patch only as behalf of myself and not as O1.

It's up to @avivey to decide whenever to pass on an early adoption of phutil_nonempty_stringlike() to accept objects - having said I was really not able to identify object usages during my runtime local test: I've found just NULL or strings, so, the input domain of phutil_nonempty_string() seems respected and fine.

src/view/phui/PHUITagView.php
146

✅ I tested this in 100+ pages without finding any explosion related to non-null or non-string. I think it's good if phutil_nonempty_string() will report any alien value. In case, we will pass to phutil_nonempty_stringlike().

adopt phutil_nonempty_stringlike() just in case

valerio.bozzolan retitled this revision from Fix PHP 8.1 "strlen(null)" exception which blocks rendering the Projects page to Fix PHP 8.1 "strlen(null)" exception which blocks rendering the Projects page (and log alien values).May 1 2023, 14:41
valerio.bozzolan edited the summary of this revision. (Show Details)

Hoping to be useful I proposed a version that I can surely accept as O1.

The idea is to just adopt phutil_nonempty_stringlike() that also accepts objects that can be casted to strings. That is really the minimal change to reproduce what was going on before PHP 8.1.

Here we introduce also a comodity phlog() line, in order to have a better overview of what's going on under the hood in the future.

sgtm

This revision is now accepted and ready to land.May 1 2023, 14:44