[wp-trac] [WordPress Trac] #44532: Extreme memory leak related to wp_is_stream in wp-includes/functions.php in WordPress 4.9.7

[WordPress Trac]

#44532: Extreme memory leak related to wp_is_stream in wp-includes/functions.php in
WordPress 4.9.7
------------------------------------------------+---------------------
 Reporter:  timbowesohft                        |       Owner:  (none)
     Type:  defect (bug)                        |      Status:  new
 Priority:  high                                |   Milestone:  4.9.8
Component:  Media                               |     Version:  4.9.7
 Severity:  major                               |  Resolution:
 Keywords:  has-patch reporter-feedback commit  |     Focuses:
------------------------------------------------+---------------------
Changes (by pento):

 * keywords:  has-patch reporter-feedback => has-patch reporter-feedback
     commit


Comment:

 Thanks for the suggestion, @itowhid06. In this case, we don't need to add
 a special check. `file://` is still a stream, it's just a local stream,
 same as `rar://`, for example. if a path starting with `file://` is sent
 to `wp_is_stream()`, it should return `true`.

 The discussion in #43054 makes it pretty clear that this is a PHP memory
 corruption issue: a non-string value is somehow being returned by
 `stream_get_wrappers()`, which has two possible effects:

 - The random data is small when interpreted as a string. (ie, reading it
 as string quickly encounters a `NULL` byte, indicating the end of the
 string.) This was fixed in #43054.
 - The random data is huge when interpreted as a string. This is the issue
 that we're seeing here.

 There are two ways of tackling this second issue:

 - As in [attachment:"44532.1.diff"], bailing if the path isn't a stream.
 This isn't perfect: it's still possible to trigger the PHP bug, but it
 does significantly reduce the chances of the bug being encountered. For
 many WordPress sites (those that don't have plugins that use streams), it
 reduces the chance to zero.

 - Parse each wrapper name. As @dd32 [ticket:43054#comment:5 mentioned on
 #43054], there are strict rules about what characters a wrapper name can
 contain. If we see an invalid character, bail on that wrapper name, and
 move onto the next one: it's a safe bet that a pointer to random memory
 will contain characters that aren't valid in a wrapper name: this would
 solve the issue for approximately 100% of cases. (There's a non-zero
 chance the random memory chunk we're given could actually be interpreted
 as data, and that the path sent to `wp_is_stream()` happens to match that
 random memory. 😉)

 I'm strongly inclined to go with option one. Option two is quite complex,
 its a performance drag for the vast majority of calls, and it'll hopefully
 be made unnecessary in a future PHP release. Option one is simple,
 understandable, and it provides a performance improvement!

 Unless any of the folks testing [attachment:"44532.1.diff"] are still
 seeing the problem, I'm fine with it being committed and backported to the
 4.9 branch in time for 4.9.8 beta 1.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/44532#comment:46>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform