Skip to content

Commit d92fc14

Browse files
Dominik InführMichal Klocek
authored andcommitted
[462][Backport] Security bug 424905890
Manual cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/v8/v8/+/6668536: Disable code compaction with stack Since fast api calls can now also call back to JS there can be multiple active fast c calls at the same time. This means just checking Isolate::InFastCCall() is not enough anymore because this only returns the state of the last CEntry frame. This CL therefore disables code space compaction when a stack is present to allow for multiple/nested fast C calls. Alternatively we could also just e.g. pin code objects referenced from the stack but this would require a bit more work. Bug: 424905890 Change-Id: I2798e77bb2534253a1dc4b0079cdfa8e5d3bcac1 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6668536 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Andreas Haas <ahaas@chromium.org> Commit-Queue: Dominik Inführ <dinfuehr@chromium.org> Cr-Commit-Position: refs/heads/main@{#101043} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/665063 Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
1 parent a607634 commit d92fc14

File tree

3 files changed

+5
-16
lines changed

3 files changed

+5
-16
lines changed

chromium/v8/src/execution/frames.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -849,9 +849,9 @@ void StackFrame::IteratePc(RootVisitor* v, Address* constant_pool_address,
849849
// stack pointer is known. This means we cannot relocate InstructionStreams
850850
// for fast c calls.
851851
DCHECK(!InFastCCall());
852-
// Currently we turn off code space compaction fully when performing a GC in a
853-
// fast C call.
854-
DCHECK(!isolate()->InFastCCall());
852+
// Ensure that code space compaction is turned off with stack. This is
853+
// necessary because we cannot update return addresses for fast c calls.
854+
CHECK(!isolate()->heap()->IsGCWithStack());
855855

856856
Tagged<InstructionStream> istream =
857857
GCSafeCast<InstructionStream>(visited_istream, isolate()->heap());

chromium/v8/src/flags/flag-definitions.h

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -588,9 +588,6 @@ DEFINE_WEAK_IMPLICATION(maglev_future, maglev_speculative_hoist_phi_untagging)
588588
DEFINE_WEAK_IMPLICATION(maglev_future, maglev_inline_api_calls)
589589
DEFINE_WEAK_IMPLICATION(maglev_future, maglev_escape_analysis)
590590
DEFINE_WEAK_IMPLICATION(maglev_future, maglev_licm)
591-
// This might be too big of a hammer but we must prohibit moving the C++
592-
// trampolines while we are executing a C++ code.
593-
DEFINE_NEG_IMPLICATION(maglev_inline_api_calls, compact_code_space_with_stack)
594591

595592
DEFINE_UINT(
596593
concurrent_maglev_max_threads, 2,
@@ -2182,12 +2179,6 @@ DEFINE_BOOL(compact_on_every_full_gc, false,
21822179
"Perform compaction on every full GC")
21832180
DEFINE_BOOL(compact_with_stack, true,
21842181
"Perform compaction when finalizing a full GC with stack")
2185-
DEFINE_BOOL(
2186-
compact_code_space_with_stack, true,
2187-
"Perform code space compaction when finalizing a full GC with stack")
2188-
// Disabling compaction with stack implies also disabling code space compaction
2189-
// with stack.
2190-
DEFINE_NEG_NEG_IMPLICATION(compact_with_stack, compact_code_space_with_stack)
21912182
DEFINE_BOOL(shortcut_strings_with_stack, true,
21922183
"Shortcut Strings during GC with stack")
21932184
DEFINE_BOOL(stress_compaction, false,
@@ -2919,7 +2910,6 @@ DEFINE_BOOL(freeze_flags_after_init, true,
29192910
#endif
29202911
DEFINE_BOOL(cet_compatible, V8_CET_SHADOW_STACK_BOOL,
29212912
"Generate Intel CET compatible code")
2922-
DEFINE_NEG_IMPLICATION(cet_compatible, compact_code_space_with_stack)
29232913

29242914
// mksnapshot.cc
29252915
DEFINE_STRING(embedded_src, nullptr,

chromium/v8/src/heap/mark-compact.cc

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ bool MarkCompactCollector::StartCompaction(StartCompactionMode mode) {
354354
CollectEvacuationCandidates(heap_->trusted_space());
355355

356356
if (heap_->isolate()->AllowsCodeCompaction() &&
357-
(!heap_->IsGCWithStack() || v8_flags.compact_code_space_with_stack)) {
357+
!(mode == StartCompactionMode::kAtomic && heap_->IsGCWithStack())) {
358358
CollectEvacuationCandidates(heap_->code_space());
359359
} else if (v8_flags.trace_fragmentation) {
360360
TraceFragmentation(heap_->code_space());
@@ -4904,8 +4904,7 @@ void MarkCompactCollector::EvacuatePagesInParallel() {
49044904
for (PageMetadata* page : old_space_evacuation_pages_) {
49054905
ReportAbortedEvacuationCandidateDueToFlags(page->area_start(), page);
49064906
}
4907-
} else if (!v8_flags.compact_code_space_with_stack ||
4908-
heap_->isolate()->InFastCCall()) {
4907+
} else {
49094908
// For fast C calls we cannot patch the return address in the native stack
49104909
// frame if we would relocate InstructionStream objects.
49114910
for (PageMetadata* page : old_space_evacuation_pages_) {

0 commit comments

Comments
 (0)