[sandbox] Invalidate external pointer slots when objects change layout [v8/v8 : main]
Samuel Groß (Gerrit)
Attention is currently required from: Patrick Thier.
Samuel Groß would like Patrick Thier to review this change.
[sandbox] Invalidate external pointer slots when objects change layout
Similar to recorded slots, we also need to invalidate external pointer
slots when a heap object is in-place converted to a different object
type. The reason is that during external pointer table compaction, we
record the addresses of external pointer slots that hold entries that
should be evacuated during sweeping. However, if the object owning that
entry turns into a different type of object between marking and
sweeping, then we'll see an invalid external pointer handle and might
crash when trying to dereference it.
Bug: chromium:1459413
Change-Id: I49192a019f458dca1f65747ebd0dc0114ce1d0da
---
M src/heap/heap.cc
M src/heap/heap.h
M src/objects/slots-inl.h
M src/objects/slots.h
M src/objects/string.cc
M src/sandbox/external-pointer-table-inl.h
M src/sandbox/external-pointer-table.cc
M src/sandbox/external-pointer-table.h
8 files changed, 161 insertions(+), 33 deletions(-)
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Patrick Thier.
Patrick Thier (Gerrit)
Attention is currently required from: Samuel Groß.
4 comments:
Patchset:
Nice Fix! Overall LGTM, just some comments.
File src/heap/heap.h:
Patch Set #4, Line 1083:
HasExternalPointerSlotsToInvalidatenit: I would move this argument right after `invalidate_recorded_slots` and remove the default. By enforcing to explicitly pass the argument it is less likely that it's forgotten.
File src/heap/heap.cc:
I would only execute this whole block if `has_external_pointer_slots_to_invalidate == kYes` to avoid unnecessary overhead.
File src/sandbox/external-pointer-table-inl.h:
Shouldn't this be `!=`?
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Samuel Groß uploaded patch set #5 to this change.
[sandbox] Invalidate external pointer slots when objects change layout
Similar to recorded slots, we also need to invalidate external pointer
slots when a heap object is in-place converted to a different object
type. The reason is that during external pointer table compaction, we
record the addresses of external pointer slots that hold entries that
should be evacuated during sweeping. However, if the object owning that
entry turns into a different type of object between marking and
sweeping, then we'll see an invalid external pointer handle and might
crash when trying to dereference it.
Bug: chromium:1459413
Change-Id: I49192a019f458dca1f65747ebd0dc0114ce1d0da
---
M src/deoptimizer/translated-state.cc
M src/heap/heap.cc
M src/heap/heap.h
M src/objects/shared-function-info-inl.h
M src/objects/slots-inl.h
M src/objects/slots.h
M src/objects/string.cc
M src/runtime/runtime-object.cc
M src/sandbox/external-pointer-table-inl.h
M src/sandbox/external-pointer-table.cc
M src/sandbox/external-pointer-table.h
11 files changed, 177 insertions(+), 42 deletions(-)
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Patrick Thier.
4 comments:
Patchset:
Thanks! PTAL!
File src/heap/heap.h:
Patch Set #4, Line 1083:
HasExternalPointerSlotsToInvalidate
nit: I would move this argument right after `invalidate_recorded_slots` and remove the default. […]
Done
File src/heap/heap.cc:
I would only execute this whole block if `has_external_pointer_slots_to_invalidate == kYes` to avoid […]
Ok, I've now changed it to `InvalidateExternalPointerSlots` so it's similar to `InvalidateRecordedSlots`. WDYT of that?
File src/sandbox/external-pointer-table-inl.h:
Shouldn't this be `!=`?
Yes, good catch!
I guess the v8 tests don't trigger table compaction (too frequently). Some of the (optional) chromium bots definitely do, but maybe some V8 tests should stress this as well. Anyway, that's a different topic...
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Michael Lippautz, Patrick Thier.
Samuel Groß would like Michael Lippautz to review this change.
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Michael Lippautz, Patrick Thier.
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Thier (Gerrit)
Attention is currently required from: Michael Lippautz, Samuel Groß.
Patch set 5:Code-Review +1
3 comments:
Patchset:
Thanks, LGTM!
File src/heap/heap.cc:
Ok, I've now changed it to `InvalidateExternalPointerSlots` so it's similar to `InvalidateRecordedSl […]
sg!
File src/heap/heap.cc:
Patch Set #5, Line 4017:
DCHECK_GEDid you mean `DCHECK_GT`? at least that's how it was expressed before.
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Samuel Groß.
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Samuel Groß.
Samuel Groß uploaded patch set #6 to this change.
The following approvals got outdated and were removed: Commit-Queue+1 by Samuel Groß
[sandbox] Invalidate external pointer slots when objects change layout
Similar to recorded slots, we also need to invalidate external pointer
slots when a heap object is in-place converted to a different object
type. The reason is that during external pointer table compaction, we
record the addresses of external pointer slots that hold entries that
should be evacuated during sweeping. However, if the object owning that
entry turns into a different type of object between marking and
sweeping, then we'll see an invalid external pointer handle and might
crash when trying to dereference it.
Bug: chromium:1459413
Change-Id: I49192a019f458dca1f65747ebd0dc0114ce1d0da
---
M src/deoptimizer/translated-state.cc
M src/heap/heap.cc
M src/heap/heap.h
M src/objects/shared-function-info-inl.h
M src/objects/slots-inl.h
M src/objects/slots.h
M src/objects/string.cc
M src/runtime/runtime-object.cc
M src/sandbox/external-pointer-table-inl.h
M src/sandbox/external-pointer-table.cc
M src/sandbox/external-pointer-table.h
11 files changed, 177 insertions(+), 42 deletions(-)
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Patch Set #5, Line 4017:
DCHECK_GEDid you mean `DCHECK_GT`? at least that's how it was expressed before.
Yes, thanks!
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Patch set 6:Commit-Queue +2
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/heap/heap.cc
Insertions: 1, Deletions: 1.
@@ -4014,7 +4014,7 @@
ExternalPointerSlotInvalidator external_pointer_slot_invalidator(isolate());
int num_invalidated_slots = external_pointer_slot_invalidator.Visit(object);
USE(num_invalidated_slots);
- DCHECK_GE(num_invalidated_slots, 0);
+ DCHECK_GT(num_invalidated_slots, 0);
}
#endif
```
[sandbox] Invalidate external pointer slots when objects change layout
Similar to recorded slots, we also need to invalidate external pointer
slots when a heap object is in-place converted to a different object
type. The reason is that during external pointer table compaction, we
record the addresses of external pointer slots that hold entries that
should be evacuated during sweeping. However, if the object owning that
entry turns into a different type of object between marking and
sweeping, then we'll see an invalid external pointer handle and might
crash when trying to dereference it.
Bug: chromium:1459413
Change-Id: I49192a019f458dca1f65747ebd0dc0114ce1d0da
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4942593
Reviewed-by: Patrick Thier <pth...@chromium.org>
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Commit-Queue: Samuel Groß <sa...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#90433}
---
M src/deoptimizer/translated-state.cc
M src/heap/heap.cc
M src/heap/heap.h
M src/objects/shared-function-info-inl.h
M src/objects/slots-inl.h
M src/objects/slots.h
M src/objects/string.cc
M src/runtime/runtime-object.cc
M src/sandbox/external-pointer-table-inl.h
M src/sandbox/external-pointer-table.cc
M src/sandbox/external-pointer-table.h
11 files changed, 177 insertions(+), 42 deletions(-)
Clemens Backes (Gerrit)
Attention is currently required from: Samuel Groß.
1 comment:
File src/sandbox/external-pointer-table.h:
Patch Set #7, Line 379:
std::vector<Address> invalidated_fields_;Looks like this is missing an `#include <vector>`, which causes build problems on some systems: https://groups.google.com/g/v8-dev
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Clemens Backes (Gerrit)
File src/sandbox/external-pointer-table.h:
Patch Set #7, Line 379:
std::vector<Address> invalidated_fields_;
Looks like this is missing an `#include <vector>`, which causes build problems on some systems: http […]
Sorry, this is the concrete post: https://groups.google.com/g/v8-dev/c/RsUTOWRu3dk/m/0R2XDxfBBgAJ
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Patch Set #7, Line 379:
std::vector<Address> invalidated_fields_;
Sorry, this is the concrete post: https://groups.google. […]
Ah yes thanks! Prepared a fix here: https://chromium-review.googlesource.com/c/v8/v8/+/4952334
To view, visit change 4942593. To unsubscribe, or for help writing mail filters, visit settings.