[SPL] Prevent fclose on underlying SplFileObject file stream.

[camporter]

To address https://bugs.php.net/bug.php?id=81691

I'm not sure if this is the correct way to address this issue, hence the draft PR.

[cmb69]

Well, this wouldn't fix the general issue that the stream must not manipulated without going through the object. But since this can't be prevented as long as the stream is a resource, this looks like a good stop-gap measure for the worst case. Thank you for the PR!

[camporter]

In the process of testing this I got curious about something with get_resources(). It seems like there's a case where it might be leaking memory? I didn't take the time to dive into it very much...

Can open a separate issue for it though.

Increasing memory usage:

<?php
  ini_set('memory_limit', '8M');
  $i = 0;
  while(true) {
    $file = new SplFileObject(__FILE__);
    $res = get_resources();

    // not setting $res to null
    $file = null;
  
    if ($i == 0) {
      print("mem: " . memory_get_usage() . "\n");
    }
    $i++;
    if ($i == 10000) {
      $i = 0;
    }
  }

Constant memory usage:

<?php
  ini_set('memory_limit', '8M');
  $i = 0;
  while(true) {
    $file = new SplFileObject(__FILE__);
    $res = get_resources();

    $res = null; // set to null
    $file = null;
  
    if ($i == 0) {
      print("mem: " . memory_get_usage() . "\n");
    }
    $i++;
    if ($i == 10000) {
      $i = 0;
    }
  }

[cmb69]

In the process of testing this I got curious about something with get_resources(). It seems like there's a case where it might be leaking memory?

That is not a memory leak in php-src, though. It is basically the same as:

<?php

$all_objs = new WeakMap();
function get_all_objs() {
    global $all_objs;

    $res = [];
    foreach ($all_objs as $obj => $dummy) {
        $res[] = $obj;
    }
    return $res;
}

ini_set('memory_limit', '8M');
$i = 0;
while(true) {
  $obj = new stdClass();
  $all_objs[$obj] = $i;
  $objs = get_all_objs();

//   $objs = null;
  $obj = null;

  if ($i%1000 == 0) {
    print("mem: " . memory_get_usage() . "\n");
  }
  $i++;
}

Calling get_resources() and get_all_objs(), respectively, increases the refcount, and only afterwards the assignment is done where the refcount is decreased. This keeps all resource and objects, respectively, alive forever. If you unset before calling the refcount increasing functions, the refcount drops to zero, and everything is fine.

Anyhow, get_resources() has been introduced with d37820f, and the commit message states:

It may be used for debugging and testing to identify resource leaks.

So it seems important to document that in the PHP manual; users should not use that for any production code (not because of the "memory leak", but as it allows to get to resources which should be in control of an object).

[camporter]

Ah! Sorry, that makes sense. Thank you 🙂

I opened doc issue php/doc-en#1318 with your suggestions.

[Girgias]

@cmb69 should this be backported to 8.0 or only master?

[cmb69]

I think targeting "master" only makes sense, since get_resources() shouldn't be used in production, and we only catch one issue anyway (you could still do other stream manipulations, which likely break SplFileObjects internals, but some code might actually do that deliberately).