php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Request #76292 The valid() method is deceptive
Submitted: 2018-05-02 08:43 UTC Modified: 2018-05-03 15:10 UTC
From: d dot negrier at thecodingmachine dot com Assigned: colder (profile)
Status: Assigned Package: Weakref (PECL)
PHP Version: 7.2.5 OS: Linux
Private report: No CVE-ID: None
Have you experienced this issue?
Rate the importance of this bug to you:

 [2018-05-02 08:43 UTC] d dot negrier at thecodingmachine dot com
Description:
------------
Hey! First of all, thanks for the Weakref extension. I'm using it extensively in my ORM (thecodingmachine/tdbm) and it is definitely a life-saver.

I've recently encountered a strange problem and I want to share it here.

The Weakref object has 2 methods to check for an object: `valid` and `get`.

So in most of the code using this extension, you will see:

```
if ($weakref->valid()) {
    $myobj = $weakref->get();
    $myobj->doSomeStuff();
}
```

This code is actually buggy. Indeed, you never know when the garbage collector might be triggered. If the garbage collector is triggered between `$weakref->valid()` and `$weakref->get()`, then `$weakref->get() === null`.

This actually happened to me today (for the first time after 5 years of use of Weakref):

https://github.com/thecodingmachine/tdbm/pull/80#issuecomment-385902994

If I disable the garbage collector, everything runs fine:

```
gc_disable();
if ($weakref->valid()) {
    $myobj = $weakref->get();
    $myobj->doSomeStuff();
}
gc_enable();
```

When I think about it, I come to realize that the `valid()` method is really deceptive (because you cannot trust it to perform a "get" request afterwards.

The only sane way to write this code is:

```
$myobj = $weakref->get();
if ($myobj !== null) {
    $myobj->doSomeStuff();
}
```


I'm not sure what is the good way to solve this but I think we should either:

- deprecate the "valid()" method
- or write a huge warning in the "valid()" method documentation




Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-05-02 11:54 UTC] colder@php.net
-Status: Open +Status: Feedback -Assigned To: +Assigned To: colder
 [2018-05-02 11:54 UTC] colder@php.net
Hello,

you are right that there is some race-condition to valid() followed by get() that makes it somewhat dangerous to trust as is.

Have you tried to acquire() the weak ref instead of checking valid()? If acquire is successful you should be safe to get() it until you release().

Best
 Instead of deprecating valid() or disabling the GC in the middle of the critical section, how about you simply acquire() the weakref? If this returns true, you know that it is safe to get, which you can then release() later.
 [2018-05-02 13:32 UTC] d dot negrier at thecodingmachine dot com
-Status: Feedback +Status: Assigned
 [2018-05-02 13:32 UTC] d dot negrier at thecodingmachine dot com
Hey Etienne,

Thanks a lot for the quick reply.

You are perfectly right that using "acquire" makes a lot of sense and solves the race condition:

```
if ($weakref->acquire()) {
    $myobj = $weakref->get();
    $myobj->doSomeStuff();
    $weakref->release();
}
```

This is great.

But you are kind of proving my point: the "valid" method is useless in this case since it is indeed replaced by acquire.
There are very few cases where "valid()" is useful and it is too tempting to use it. A proper warning in the doc might save some people.
 [2018-05-02 22:16 UTC] cmb@php.net
It occurs to me that any action on an object returned by ::get()
without an explicit ::acquire() is potentially prone to memory
management issues.
 [2018-05-03 05:51 UTC] colder@php.net
Hello,

I'm not sure what you mean?

If at the time of the get() call the weakref is still valid, it will increase its refcount as it returns an actual zval of it. From that point on and until it is destroyed, the weakref reference will not be collectible as it has a >=1 refcount.
 [2018-05-03 10:33 UTC] cmb@php.net
Ah, thanks!  So ::acquire()/::release() is not strictly
necessary, so that the example above could also be written as:

  if (($myobj = $weakref->get())) {
      $myobj->doSomeStuff();
  }
 [2018-05-03 15:10 UTC] d dot negrier at thecodingmachine dot com
@cmb: absolutely.

Actually, if you want to fetch an object from a WeakRef, the correct way to go is:

<?php
if ($myobj = $weakref->get()) {
    $myobj->doSomeStuff();
}
?>

My whole point is that it is too easy for a beginner using this function to write:

<?php
if ($weakref->valid()) { // returns true
    // If you are unlucky, the garbage collector might be triggered here
    $myobj = $weakref->get(); // will return null
    $myobj->doSomeStuff(); // boom
}
?>

This is wrong because the garbage collector may be triggered between first and second line.

I added a user contributed note on the "WeakRef::valid()" method page. It should be published soon. http://php.net/manual/en/weakref.valid.php
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Wed Nov 13 18:01:29 2019 UTC