[heap][sandbox] Enable external pointer table w/compressed pointers [v8/v8 : main]
Andy Wingo (Gerrit)
Andy Wingo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andy Wingo
| Commit-Queue | +1 |
1 comment
This is a large and horrible patch but which is mostly mechanical, as discussed on https://issues.chromium.org/issues/337580006.
Related details
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 2 comments![]( "Open in Gerrit")
MemOperand operand) {Here (and below) I am not sure whether these are external pointers with EPT entries or sandboxed pointers (indirectpointer).
DCHECK(ReadOnlyHeap::IsReadOnlySpaceShared());This dcheck fails on multi-cage mode. Have to figure out a solution.
Related details
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 5 comments![]( "Open in Gerrit")
Wohoo!
// Use this function instead of Internals::GetIsolateForSandbox for internalShould this be `GetIsolateForPointerCompression`?
MemOperand operand) {Here (and below) I am not sure whether these are external pointers with EPT entries or sandboxed pointers (indirectpointer).
Hmm maybe the method name is wrong here. It looks like these are "Sandboxed Pointers" (see https://source.chromium.org/chromium/chromium/src/+/main:v8/src/sandbox/GLOSSARY.md;l=74;drc=a78860780c0eebd129ff70772a81a546991055ac), used e.g. for referencing ArrayBuffer backing stores inside the sandbox.
// TODO(saelo): if we ever use the external pointer table for all external
// pointer fields in the no-sandbox-ptr-compression config, replace this code
// here and above with the respective external pointer accessors.Can we do this now? I.e. use `WriteInitializedExternalPointerField` here and `SetupLazilyInitializedExternalPointerField` in `init_extension`? If it doesn't work for some reason, feel free to just leave the TODO though
DCHECK(ReadOnlyHeap::IsReadOnlySpaceShared());This dcheck fails on multi-cage mode. Have to figure out a solution.
Ah hmm not sure how exactly this works, but I guess the problem is the this code currently assumes a shared RO space and a shared EPT RO space. If this isn't the case, we probably need one EPT RO space per heap or something like that?
Maybe worth checking if we even have any external pointers in the RO snapshot in that case though.
Related details
- Andy Wingo
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 5 comments![]( "Open in Gerrit")
Rebased, fixed some nits, still working on read-only spaces
// Use this function instead of Internals::GetIsolateForSandbox for internalShould this be `GetIsolateForPointerCompression`?
Done
MemOperand operand) {Samuel GroßHere (and below) I am not sure whether these are external pointers with EPT entries or sandboxed pointers (indirectpointer).
Hmm maybe the method name is wrong here. It looks like these are "Sandboxed Pointers" (see https://source.chromium.org/chromium/chromium/src/+/main:v8/src/sandbox/GLOSSARY.md;l=74;drc=a78860780c0eebd129ff70772a81a546991055ac), used e.g. for referencing ArrayBuffer backing stores inside the sandbox.
Ack. Just going to leave as is.
// TODO(saelo): if we ever use the external pointer table for all external
// pointer fields in the no-sandbox-ptr-compression config, replace this code
// here and above with the respective external pointer accessors.Can we do this now? I.e. use `WriteInitializedExternalPointerField` here and `SetupLazilyInitializedExternalPointerField` in `init_extension`? If it doesn't work for some reason, feel free to just leave the TODO though
It seems that we can't do it for the moment because of the need for acquire/release memory ordering; annoying. I will update the comments (and cheekily keep you on the TODO :). (Heretical thought: maybe we should just use acq/rel ordering for all external pointer accesses...)
DCHECK(ReadOnlyHeap::IsReadOnlySpaceShared());Samuel GroßThis dcheck fails on multi-cage mode. Have to figure out a solution.
Ah hmm not sure how exactly this works, but I guess the problem is the this code currently assumes a shared RO space and a shared EPT RO space. If this isn't the case, we probably need one EPT RO space per heap or something like that?
Maybe worth checking if we even have any external pointers in the RO snapshot in that case though.
Still puzzling over this one; I don't have a good understanding yet about how the RO heap works. Each snapshot has its own RO spaces? And deserializing an isolate might put those RO spaces into some sort of shared part of the pointer cage? If there are any design docs on read-only spaces I would be happy to peruse them :)
Related details
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
MemOperand operand) {Samuel GroßHere (and below) I am not sure whether these are external pointers with EPT entries or sandboxed pointers (indirectpointer).
Andy WingoHmm maybe the method name is wrong here. It looks like these are "Sandboxed Pointers" (see https://source.chromium.org/chromium/chromium/src/+/main:v8/src/sandbox/GLOSSARY.md;l=74;drc=a78860780c0eebd129ff70772a81a546991055ac), used e.g. for referencing ArrayBuffer backing stores inside the sandbox.
Ack. Just going to leave as is.
Acknowledged
Related details
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
It seems the node CI doesn't manage to build addons: see end of https://logs.chromium.org/logs/node-ci/buildbucket/cr-buildbucket/8748068638469800017/+/u/build/compile/stdout. No idea why as it doesn't seem to expose the logs!
Related details
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepak Mohan (Robo) (Gerrit)
Deepak Mohan (Robo) added 1 comment![]( "Open in Gerrit")
It seems the node CI doesn't manage to build addons: see end of https://logs.chromium.org/logs/node-ci/buildbucket/cr-buildbucket/8748068638469800017/+/u/build/compile/stdout. No idea why as it doesn't seem to expose the logs!
Drive-by comment: Incase it helps, addon build step happens via the target https://chromium.googlesource.com/v8/node-ci/+/refs/heads/main/node_tests/BUILD.gn#104 and it relies on node binary built from this change https://github.com/v8/node/blob/b5a7a8ad3b09405e5523f8da35bc3f393db79dcc/tools/build_addons.py#L30. There is a crash on startup of the binary. You can repro the issue by setting up https://chromium.googlesource.com/v8/node-ci/+/refs/heads/main#checking-out-source and replacing v8 checkout with this change, followed by
```
> ./tools/gn-gen.py out/Release
> ninja -C out/Release node_tests:node_test_addons_build
```
Related details
- Andy Wingo
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
Deepak Mohan (Robo)It seems the node CI doesn't manage to build addons: see end of https://logs.chromium.org/logs/node-ci/buildbucket/cr-buildbucket/8748068638469800017/+/u/build/compile/stdout. No idea why as it doesn't seem to expose the logs!
Drive-by comment: Incase it helps, addon build step happens via the target https://chromium.googlesource.com/v8/node-ci/+/refs/heads/main/node_tests/BUILD.gn#104 and it relies on node binary built from this change https://github.com/v8/node/blob/b5a7a8ad3b09405e5523f8da35bc3f393db79dcc/tools/build_addons.py#L30. There is a crash on startup of the binary. You can repro the issue by setting up https://chromium.googlesource.com/v8/node-ci/+/refs/heads/main#checking-out-source and replacing v8 checkout with this change, followed by
```
> ./tools/gn-gen.py out/Release
> ninja -C out/Release node_tests:node_test_addons_build
```
Firstly -- thank you Deepak !!! A very helpful hint. I can reproduce this locally now. The issue is basic, that calling toString on a node buffer fails somewhere, causing a segfault. Probably has to do with one-byte external strings.
Still figuring out how to get a debug build; passing --debug to gn-gen.py does residualize is_debug=true but somehow that doesn't translate to the binaries. Will keep at it.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Almothana Athamneh (Gerrit)
Almothana Athamneh removed almuthan...@chromium.org from this change![]( "Open in Gerrit")
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
Poking at this again: the node failure manifests itself as an EPT entry having the wrong type. The segfault occurs here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/x64/builtins-x64.cc;l=4523, which loads the callback for a FunctionTemplateInfo. Still poking to find how this happened.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 2 comments![]( "Open in Gerrit")
It would seem that the problem is that the
Poking at this again: the node failure manifests itself as an EPT entry having the wrong type. The segfault occurs here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/x64/builtins-x64.cc;l=4523, which loads the callback for a FunctionTemplateInfo. Still poking to find how this happened.
Somehow the EmbedderDataArray of node's NativeContext is corrupted when it is deserialized, containing non-null ExternalPointerHandles in it. Later writes to those context fields during node boot end up overwriting the handle that was correctly allocated when deserializing the FunctionTemplateInfo instance. Still poking; very perplexing.
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
It would seem that the problem is that the
Funny. So what happens is that the `NativeContext` gets deserialized, along with its `EmbedderDataArray`, which has 40 slots. Each slot is `EmbedderDataSlot`, with two 32-bit words. There is special post-processing for a context's `EmbedderDataArray` in `context-deserializer.cc` but that doesn't occur in this case because Node doesn't yet support custom context serialization/deserialization. Each slot in the array can have a heap object or an external pointer. Those external pointer slots are lazily initialized, so the first write to them should allocate an EPT slot. However the bug is that the `EmbedderDataArray` is getting deserialized with non-null EPT handles!
The bug occurs because in the serializer we skip over `ExternalPointerSlot` values for `EmbedderDataArray` and other types: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/snapshot/serializer.cc;l=1156
This causes the `bytes_processed_so_far_` to not advance when visiting the EPT slot, indicating these values will be filled in by copying, via `OutputRawData`. But of course this ends up copying a raw EPT handle from the serializer to the deserializer, leading to aliasing of the EPT handle and eventual stompling.
The way this works for other data types is that we null out the external pointer fields before writing. Perhaps that is what needs to happen with embedder fields on the context as well.
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
Andy WingoIt would seem that the problem is that the
Funny. So what happens is that the `NativeContext` gets deserialized, along with its `EmbedderDataArray`, which has 40 slots. Each slot is `EmbedderDataSlot`, with two 32-bit words. There is special post-processing for a context's `EmbedderDataArray` in `context-deserializer.cc` but that doesn't occur in this case because Node doesn't yet support custom context serialization/deserialization. Each slot in the array can have a heap object or an external pointer. Those external pointer slots are lazily initialized, so the first write to them should allocate an EPT slot. However the bug is that the `EmbedderDataArray` is getting deserialized with non-null EPT handles!
The bug occurs because in the serializer we skip over `ExternalPointerSlot` values for `EmbedderDataArray` and other types: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/snapshot/serializer.cc;l=1156
This causes the `bytes_processed_so_far_` to not advance when visiting the EPT slot, indicating these values will be filled in by copying, via `OutputRawData`. But of course this ends up copying a raw EPT handle from the serializer to the deserializer, leading to aliasing of the EPT handle and eventual stompling.
The way this works for other data types is that we null out the external pointer fields before writing. Perhaps that is what needs to happen with embedder fields on the context as well.
I should add that this doesn't happen in the existing +pc,-sandbox configuration because serializing a raw pointer does not appear to be a problem, if that pointer is always overwritten after deserialization. The problem is that if we serialize a raw EPT handle, it's an indirect write that aliases a storage location owned by something else.
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
}PTAL especially at this block, which fixes the node-ci errors. Would have been nice to use EmbedderDataSlot::load_raw / EmbedderDataSlot::store_raw to avoid allocating a fresh EPT entry but gc_safe_store balked at restoring a value for reasons I didn't investigate.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
PTAL especially at this block, which fixes the node-ci errors. Would have been nice to use EmbedderDataSlot::load_raw / EmbedderDataSlot::store_raw to avoid allocating a fresh EPT entry but gc_safe_store balked at restoring a value for reasons I didn't investigate.
The idea is good but the execution is bad :) Really I just want to null out external pointers, but with pointer compression this also nulls out heap objects (because of the way ToAlignedPointer works; https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/embedder-data-slot-inl.h;l=98). Will fix.
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
DCHECK(ReadOnlyHeap::IsReadOnlySpaceShared());Samuel GroßThis dcheck fails on multi-cage mode. Have to figure out a solution.
Andy WingoAh hmm not sure how exactly this works, but I guess the problem is the this code currently assumes a shared RO space and a shared EPT RO space. If this isn't the case, we probably need one EPT RO space per heap or something like that?
Maybe worth checking if we even have any external pointers in the RO snapshot in that case though.
Still puzzling over this one; I don't have a good understanding yet about how the RO heap works. Each snapshot has its own RO spaces? And deserializing an isolate might put those RO spaces into some sort of shared part of the pointer cage? If there are any design docs on read-only spaces I would be happy to peruse them :)
OK. Before this patch, the RO heap snapshot in non-sandbox configurations would not have EPT entries, and now it will. For a monocage configuration this is fine; closer to the sandbox configuration. But for a multi-cage configuration we need to refactor to make one read-only heap per isolate group, in a multi-cage configuration. That will require another patch or two.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andy Wingo
| Commit-Queue | +1 |
Andy WingoPTAL especially at this block, which fixes the node-ci errors. Would have been nice to use EmbedderDataSlot::load_raw / EmbedderDataSlot::store_raw to avoid allocating a fresh EPT entry but gc_safe_store balked at restoring a value for reasons I didn't investigate.
The idea is good but the execution is bad :) Really I just want to null out external pointers, but with pointer compression this also nulls out heap objects (because of the way ToAlignedPointer works; https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/embedder-data-slot-inl.h;l=98). Will fix.
DCHECK(ReadOnlyHeap::IsReadOnlySpaceShared());Samuel GroßThis dcheck fails on multi-cage mode. Have to figure out a solution.
Andy WingoAh hmm not sure how exactly this works, but I guess the problem is the this code currently assumes a shared RO space and a shared EPT RO space. If this isn't the case, we probably need one EPT RO space per heap or something like that?
Maybe worth checking if we even have any external pointers in the RO snapshot in that case though.
Andy WingoStill puzzling over this one; I don't have a good understanding yet about how the RO heap works. Each snapshot has its own RO spaces? And deserializing an isolate might put those RO spaces into some sort of shared part of the pointer cage? If there are any design docs on read-only spaces I would be happy to peruse them :)
OK. Before this patch, the RO heap snapshot in non-sandbox configurations would not have EPT entries, and now it will. For a monocage configuration this is fine; closer to the sandbox configuration. But for a multi-cage configuration we need to refactor to make one read-only heap per isolate group, in a multi-cage configuration. That will require another patch or two.
Dmitry is working on this in https://chromium-review.googlesource.com/c/v8/v8/+/5632279.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andy Wingo
| Commit-Queue | +1 |
2 comments
PTAL. This patch enables ExternalPointerHandle for all external pointers when pointer-compression is enabled, regardless of the sandbox. Had to rebase on top of Dmitry's patch to enabled shared read-only space in multi-cage contexts, because of external pointers in the snapshot. We are buying a reduction of the configuration-space size by paying some concessions to the multi-cage configuration; I think it pays off but do let me know.
DCHECK(ReadOnlyHeap::IsReadOnlySpaceShared());Samuel GroßThis dcheck fails on multi-cage mode. Have to figure out a solution.
Andy WingoAh hmm not sure how exactly this works, but I guess the problem is the this code currently assumes a shared RO space and a shared EPT RO space. If this isn't the case, we probably need one EPT RO space per heap or something like that?
Maybe worth checking if we even have any external pointers in the RO snapshot in that case though.
Andy WingoStill puzzling over this one; I don't have a good understanding yet about how the RO heap works. Each snapshot has its own RO spaces? And deserializing an isolate might put those RO spaces into some sort of shared part of the pointer cage? If there are any design docs on read-only spaces I would be happy to peruse them :)
Andy WingoOK. Before this patch, the RO heap snapshot in non-sandbox configurations would not have EPT entries, and now it will. For a monocage configuration this is fine; closer to the sandbox configuration. But for a multi-cage configuration we need to refactor to make one read-only heap per isolate group, in a multi-cage configuration. That will require another patch or two.
Dmitry is working on this in https://chromium-review.googlesource.com/c/v8/v8/+/5632279.
Dmitry's patch passes a dry run, yay. Rebased on top and resolving this point.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
- Samuel Groß
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß voted and added 2 comments![]( "Open in Gerrit")
Votes added by Samuel Groß
| Code-Review | +1 |
2 comments
Thanks! \o/ Overall looks good, just one comment
// ???... :)
I think the naming here is probably just wrong, I guess this should be `LoadSandboxedPointerField`. At least then the code makes sense
Related details
- Andy Wingo
- Deepak Mohan (Robo)
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 2 comments![]( "Open in Gerrit")
Looks good to me. Will stamp after the assembler issues have been resolved.
Related details
- Andy Wingo
- Deepak Mohan (Robo)
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Wingo (Gerrit)
Andy Wingo added 1 comment![]( "Open in Gerrit")
Looks good to me. Will stamp after the assembler issues have been resolved.
Thanks, will fix. FYI, will retake this once Dmitry and I manage to land a fix to allow multicage configurations to have the read-only space, because of external pointers in the snapshot.
Related details
- Deepak Mohan (Robo)
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 1 comment![]( "Open in Gerrit")
Hey Andy, do you still want to land this CL? I just ran into this issue again yesterday (https://chromium-review.googlesource.com/c/v8/v8/+/5938952) and remembered that we wanted to fix that :)
Related details
- Andy Wingo
- Deepak Mohan (Robo)
- Michael Lippautz
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |