Fix phpdbg over-read watching an element of a packed array#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
2 participants
Packed arrays store bare zvals, but phpdbg watches every array element as a
Bucketand readsBucket.h/.keyin its relocation check, over-reading the neighbouring element. A sibling write then fires a phantom watchpoint, which crashed on 32-bit Windows (WINDOWS_X86_NTSin the nightly) once GH-22480 un-xfailedwatch_006. For a packed parent, relocation is now detected by re-resolving the element and only the zval value is compared.Note:
watch_005still carries aPHP_INT_SIZE == 4xfail with the same "flaws in the implementation of watchpoints" wording, but on a different path (string/reference indirection, not packed buckets). It likely hides a related but distinct 32-bit flaw masked by that skip; this change does not touch it, so it stays skipped.