Fix phpdbg over-read when watching a packed array element - #22756
Conversation
|
This doesn't work for me. The PR still fails with the same error. I'll try to create a PR by tomorrow. |
|
Yeah I see now, I thought the linux 32 would validate it fully, but it doesn't. I see the issue on Win32. Will try to take a stab @ it tonight if I get anywhere I'll tag you on this PR, otherwise all yours :( |
|
You should run with MSAN, it reproduces on Linux. I think there are multiple issues.
|
phpdbg watched every array element as a Bucket, but a packed array stores bare zvals, so reading Bucket.h/.key over-read the neighbouring element and a sibling write tripped a phantom break. De-indirected stack variables are not buckets either. Watch such elements as WATCH_ON_ZVAL, re-resolve them in the parent on relocation, drop the watch when the stack frame releases (IS_UNDEF), and compare the type info before the value so uninitialised CV bytes are never read. phpdbg_btree_insert_or_update published each freshly allocated node into the tree before initialising its child pointers. When the node landed on a watched page the write that initialised it faulted into the watchpoint handler, which walked the half-built node and dereferenced a wild pointer. Build the node fully, then link it in with a single store. This crashed only on 32-bit, where the compact heap places the node on the same page as the watched zval. Closes phpGH-22756
d093c0e to
c92ca4b
Compare
|
Thanks for the pointers. That fixed the over-read but Win32 still crashed, on a separate pre-existing bug. watch_005/006 pass, full phpdbg suite clean on a local x86 Windows build and under ASAN. |
phpdbg watched every array element as a Bucket, but a packed array stores bare zvals, so reading Bucket.h/.key over-read the neighbouring element and a sibling write tripped a phantom break. De-indirected stack variables are not buckets either. Watch such elements as WATCH_ON_ZVAL, re-resolve them in the parent on relocation, drop the watch when the stack frame releases (IS_UNDEF), and compare the type info before the value so uninitialised CV bytes are never read. phpdbg_btree_insert_or_update published each freshly allocated node into the tree before initialising its child pointers. When the node landed on a watched page the write that initialised it faulted into the watchpoint handler, which walked the half-built node and dereferenced a wild pointer. Build the node fully, then link it in with a single store. This crashed only on 32-bit, where the compact heap places the node on the same page as the watched zval. Closes phpGH-22756
c92ca4b to
5f279e6
Compare
e86f282 to
0c76639
Compare
|
Dropped both blocks and rebuilt the x86 NTS phpdbg; the full phpdbg suite is unchanged from before the removal. |
iluuu1994
left a comment
There was a problem hiding this comment.
Looks correct now. I'll add Bob for a review, since he wrote most of this code (albeit a very long time ago).
phpdbg watched every array element as a Bucket, but a packed array stores bare zvals, so reading Bucket.h/.key over-read the neighbouring element and a sibling write tripped a phantom break. De-indirected stack variables are not buckets either. Watch such elements as WATCH_ON_ZVAL and compare the type info before the value so uninitialised CV bytes are never read. phpdbg_btree_insert_or_update published each freshly allocated node into the tree before initialising its child pointers. When the node landed on a watched page the write that initialised it faulted into the watchpoint handler, which walked the half-built node and dereferenced a wild pointer. Build the node fully, then link it in with a single store. This crashed only on 32-bit, where the compact heap places the node on the same page as the watched zval. Closes phpGH-22756
0c76639 to
304b67c
Compare
|
Feel free to merge this so nightly CI is finally green, we can still adjust if Bob finds something later. Just make sure to fix the description to something more accurate. Thanks! |
phpdbg watched every array element as a Bucket, but packed arrays store bare zvals and de-indirected stack variables aren't buckets, so reading Bucket.h/.key over-read the neighbour and a sibling write tripped a phantom break. Watch such elements as WATCH_ON_ZVAL and compare the type info first, reading the value union only for initialised slots so an uninitialised CV is never compared by value. A separate pre-existing bug crashed 32-bit: phpdbg_btree_insert_or_update linked each new node into the tree before initialising its child pointers, so a write to a node on a watched page faulted into the watchpoint handler, which walked the half-built node. Build the node fully, then publish it with a single store. Closes phpGH-22756
304b67c to
20259f8
Compare
phpdbg watched every array element as a
Bucket, but packed arrays store bare zvals and de-indirected stack variables aren't buckets, so readingBucket.h/.keyover-read the neighbour and a sibling write tripped a phantom watchpoint. Those elements are now watched asWATCH_ON_ZVAL, comparing the type info first and reading the value union only for initialised slots, so uninitialised CV bytes are never compared.This also surfaced a pre-existing 32-bit crash:
phpdbg_btree_insert_or_update()published each new node before initialising its child pointers, so a write to a node on a watched page faulted into the watchpoint handler and walked the half-built node. It now builds the node fully before linking it in. Both showed up onWINDOWS_X86_NTSonce GH-22480 un-xfailedwatch_006;watch_005andwatch_006are un-skipped / re-recorded to match.