Fix GH-17137: Segmentation fault ext/phar/phar.c #17150
Conversation
Commit edae243 attempted to fix a leak and double free, but didn't properly understand what was going on, causing a reference count mistake and subsequent segfault in this case. The first mistake of that commit is that the reference count should've been increased because we're reusing a phar object. The error handling path should've gotten changed instead to undo this refcount increase instead of not refcounting at all (root cause of this bug). The second mistake is that the alias isn't supposed to be transferred or whatever, that just doesn't make sense. The reason the test bug69958.phpt originally leaked is because in the non-reuse case we borrowed the alias and otherwise we own the alias. If we own the alias the alias information shouldn't get deleted anyway as that would desync the alias map. Fixing these will reveal a third issue in which the alias memory is not always properly in sync with the persistence-ness of the phar, fix this as well.
|
All these duplications and repeated operations are also quite sad, this code needs some work in master... |
|
Asking for an additional reviewer, because you know... it's phar after all |
There was a problem hiding this comment.
Good analysis! Besides some nits, this looks good to me.
this code needs some work in master
Maybe we should get rid of ZIP and tar support in ext/phar (remove PharData). That adds more complexity, and we already have ext/zip, and the tar implementation is doubtful (I remember a bug report, but can't find, where it turned out that adding a file would rebuilt the whole archive, defeating the idea of tar).
| /* NOTE: we know it's not the last reference because the phar is reused. */ | ||
| phar->refcount--; | ||
| } | ||
| err_oldpath: |
There was a problem hiding this comment.
Maybe add a comment that the fallthrough is intentional?
There was a problem hiding this comment.
I usually don't do this when I write goto-style error-handling code, but perhaps I should. I'll add it here.
| var_dump($phar, $phar->decompress()); | ||
| echo "OK\n"; | ||
| ?> | ||
| --EXPECT-- |
There was a problem hiding this comment.
Expecting certain object ID might be too specific; maybe use EXPECTF and %d instead.
| pphar->alias = phar->alias; /* Transfer alias to pphar to */ | ||
| phar->alias = NULL; /* avoid being free'd twice */ | ||
| /* The alias is not owned by the phar, so set it to NULL to avoid freeing it. */ | ||
| phar->alias = NULL; |
There was a problem hiding this comment.
Do we also need phar->alias_len = 0 here?
There was a problem hiding this comment.
Not necessary because the phar will be freed due to it being the temporary object created by the caller. The free code does not care about alias_len
Breaks BC most likely.
Yeah I also remember something like this, I thought it was related to compression though. |
Yeah, would need proper deprecation RFC anyway.
I concur.
I found https://bugs.php.net/80399 now. |
Commit edae243 attempted to fix a leak and double free, but didn't properly understand what was going on, causing a reference count mistake and subsequent segfault in this case. The first mistake of that commit is that the reference count should've been increased because we're reusing a phar object. The error handling path should've gotten changed instead to undo this refcount increase instead of not refcounting at all (root cause of this bug). The second mistake is that the alias isn't supposed to be transferred or whatever, that just doesn't make sense. The reason the test bug69958.phpt originally leaked is because in the non-reuse case we borrowed the alias and otherwise we own the alias. If we own the alias the alias information shouldn't get deleted anyway as that would desync the alias map. Fixing these will reveal a third issue in which the alias memory is not always properly in sync with the persistence-ness of the phar, fix this as well. Closes phpGH-17150.
Commit edae243 attempted to fix a leak and double free, but didn't properly understand what was going on, causing a reference count mistake and subsequent segfault in this case.
The first mistake of that commit is that the reference count should've been increased because we're reusing a phar object. The error handling path should've gotten changed instead to undo this refcount increase instead of not refcounting at all (root cause of this bug).
The second mistake is that the alias isn't supposed to be transferred or whatever, that just doesn't make sense. The reason the test bug69958.phpt originally leaked is because in the non-reuse case we borrowed the alias and otherwise we own the alias. If we own the alias the alias information shouldn't get deleted anyway as that would desync the alias map.
Fixing these will reveal a third issue in which the alias memory is not always properly in sync with the persistence-ness of the phar, fix this as well.