Fix GH-15210: phpdbg_print_changed_zvals working on a real copy instead.

[devnexen]

No description provided.

[Girgias]

We don't really use credit sections any longer, as those were more to go around the limitations of SVN

[devnexen]

okidoki

[Girgias]

I don't totally understand why we need to duplicate the memory? Is it because the HashTable might be allocated with ZMM?

[devnexen]

in fact is to avoid the hashtable corruption of the original watchpoint list, working on a copy then copy back once all is done. it fixed the issue for me.

[Girgias]

Would increasing the refcounter on the HashTable also fix it or not?

[devnexen]

seems to work if I allow cow violation.

[nielsdos]

That would be wrong, we need a temporary hash table so that resizes don't cause problems. Increasing the refcount won't protect you against that and only papers over the real issue.

[nielsdos]

This doesn't seem right.

First, the test can be simplified to:

header_register_callback(function() { echo "sent";});
$a = [0];
$a[0] = 1;

And since it wasn't simplified, it makes me think that the root cause is not understood.

What happens is that there's a watchpoint for the $a array, and so when we modify it we trigger a break. The break then prints out the old and new values of the array element. Notice how the output says Old value: [Breaking on watchpoint $a[0]]. This happens because outputting the old value uses zend_write which will send the headers and therefore the user code is ran. This causes a re-entry into the phpdbg_print_changed_zvals function. When we then abort execution, the original PHPDBG_G(watchlist_mem) pointer still points at the backup list, so we get a double destruction in the module shutdown.

To solve this, we should remember the original allocation values. We only need one additional field for this because the backup pointer is never changed.

Furthermore, when trying some variations, I noticed that adding an additional c command after the c that was already there causes another crash. In this case this seems to happen because we re-add the same watchpoint element that is already added and get into the degenerate case where the old element is equal to the new element, causing a uaf.

Then when retesting under ASAN, I noticed that the input buffer leaked. I also noticed that phpdbg used the bailout system, and so some leaks cannot be fixed without getting rid of that.

Taking all of that into account, this is the patch I suggest: https://gist.github.com/nielsdos/f15a2f0bfc01885b3cd1f843e75500d0

Please check this and:

1. Simplify the test like I've shown
2. Make a test variant with 2 c commands.

Also note that even after all this fixing, there's still issues. Just try running phpdbg with tracked allocations on. This seems to happen because there's request memory allocated when setting up the watchpoints but that memory is already freed by the request allocator prior to the destruction logic for watchpoints in phpdbg. This is a different problem though.

Also running the phpdbg tests with ASAN now produces less test failures with my patch than before, but it's still very buggy and the entire sapi should be audited because damn this is all so buggy.

[devnexen]

Ok I ll get to it in few hours. I stumbled across some of the bugs you mention even though, I might be wrong, I suppose is to cope with 3rd party extensions potential serious bugs.

[nielsdos]

Well to me this makes sense, but I may be biased :P