[compiler] Fix sandbox violation with RestParameters [v8/v8 : main]

[Darius Mercadier (Gerrit)]

Attention needed from Igor Sheludko

Darius Mercadier voted and added 1 comment

Votes added by Darius Mercadier

Auto-Submit

+1

1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Darius Mercadier . resolved

PTAL, Igor (and keep in mind that I'm not super familiar with those sandbox stuff, so don't assume that I know what I'm doing 😭)

Open in Gerrit

Related details

Attention is currently required from:

• Igor Sheludko

Submit Requirements:

• Code-Owners
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I7ebb4607a47c88115d824a9a56f5443fd404a3f1

Gerrit-Change-Number: 7172454

Gerrit-PatchSet: 2

Gerrit-Owner: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Igor Sheludko <ish...@chromium.org>

Gerrit-CC: Samuel Groß <sa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Igor Sheludko <ish...@chromium.org>

Gerrit-Comment-Date: Thu, 20 Nov 2025 08:31:51 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Samuel Groß (Gerrit)]

Attention needed from Darius Mercadier and Igor Sheludko

Samuel Groß added 2 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Samuel Groß . resolved

Thanks! Overall looks good to me

File src/compiler/js-create-lowering.cc

Line 411, Patchset 2 (Latest): shared.GetBytecodeArray(broker()).parameter_count_without_receiver();

Samuel Groß . unresolved

I think this code pattern is also not fully safe since the SFI is inside the sandbox so an attacker could make it reference different bytecode arrays (or does `GetBytecodeArray(broker())` load from a serialized version of the SFI?). I guess for this code here it doesn't really matter though since we're just creating/accessing more in-sandbox data, so corruption doesn't matter.

Might be worth noting in a comment or so though? Alternatively, the correct way to do it would be to fetch the bytecode array exactly once during compilation.

Open in Gerrit

Related details

Attention is currently required from:

• Darius Mercadier
• Igor Sheludko

Submit Requirements:

• Code-Owners
• Code-Review

• No-Unresolved-Comments

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I7ebb4607a47c88115d824a9a56f5443fd404a3f1

Gerrit-Change-Number: 7172454

Gerrit-PatchSet: 2

Gerrit-Owner: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Igor Sheludko <ish...@chromium.org>

Gerrit-CC: Samuel Groß <sa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Attention: Igor Sheludko <ish...@chromium.org>

Gerrit-Comment-Date: Thu, 20 Nov 2025 12:45:21 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Igor Sheludko (Gerrit)]

Attention needed from Darius Mercadier

Igor Sheludko added 1 comment

File src/compiler/js-create-lowering.cc

Line 411, Patchset 2 (Latest): shared.GetBytecodeArray(broker()).parameter_count_without_receiver();

Samuel Groß . unresolved

I think this code pattern is also not fully safe since the SFI is inside the sandbox so an attacker could make it reference different bytecode arrays (or does `GetBytecodeArray(broker())` load from a serialized version of the SFI?). I guess for this code here it doesn't really matter though since we're just creating/accessing more in-sandbox data, so corruption doesn't matter.

Might be worth noting in a comment or so though? Alternatively, the correct way to do it would be to fetch the bytecode array exactly once during compilation.

Igor Sheludko

Can we use `state_info.parameter_count_without_receiver()` here and below too?

Open in Gerrit

Related details

Attention is currently required from:

• Darius Mercadier

Submit Requirements:

• Code-Owners
• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I7ebb4607a47c88115d824a9a56f5443fd404a3f1

Gerrit-Change-Number: 7172454

Gerrit-PatchSet: 2

Gerrit-Owner: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Igor Sheludko <ish...@chromium.org>

Gerrit-CC: Samuel Groß <sa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Comment-Date: Thu, 20 Nov 2025 12:50:42 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Samuel Groß <sa...@chromium.org>

[Darius Mercadier (Gerrit)]

Darius Mercadier voted and added 1 comment

Votes added by Darius Mercadier

Auto-Submit

+1

1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Darius Mercadier . resolved

PTAL again. Not fully fixed because I'm not getting the initial BytecodeArray from a trusted place, but this doesn't look trivial to do. Still, hopefully we're consistent w.r.t. the BytecodeArray that we initially get..

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Owners
• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I7ebb4607a47c88115d824a9a56f5443fd404a3f1

Gerrit-Change-Number: 7172454

Gerrit-PatchSet: 7

Gerrit-Owner: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Igor Sheludko <ish...@chromium.org>

Gerrit-CC: Samuel Groß <sa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Tue, 13 Jan 2026 10:41:34 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Darius Mercadier (Gerrit)]

Attention needed from Igor Sheludko and Samuel Groß

Darius Mercadier added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Darius Mercadier . resolved

(moving Samuel to reviewers as well; 2 reviews are better than 1 :) )

Open in Gerrit

Related details

Attention is currently required from:

• Igor Sheludko
• Samuel Groß

Submit Requirements:

• Code-Owners
• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I7ebb4607a47c88115d824a9a56f5443fd404a3f1

Gerrit-Change-Number: 7172454

Gerrit-PatchSet: 7

Gerrit-Owner: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Igor Sheludko <ish...@chromium.org>

Gerrit-Reviewer: Samuel Groß <sa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Igor Sheludko <ish...@chromium.org>

Gerrit-Attention: Samuel Groß <sa...@chromium.org>

Gerrit-Comment-Date: Tue, 13 Jan 2026 10:41:58 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Samuel Groß (Gerrit)]

Attention needed from Darius Mercadier and Igor Sheludko

Samuel Groß added 3 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Samuel Groß . resolved

Thanks!

File src/codegen/optimized-compilation-info.cc

Line 38, Patchset 7 (Latest): bytecode_array_ = handle(shared->GetBytecodeArray(isolate), isolate);

Samuel Groß . unresolved

Where is this used? I think we will always have one place (basically the compiler entrypoint) where we do an untrusted load of a BytecodeArray from the JSFunction (or SFI) that we're compiling. But then all other accesses to the BytecodeArray should fetch it from a trusted location (e.g. from the OptimizedCompilationInfo).

File src/compiler/js-create-lowering.cc

Line 1565, Patchset 7 (Latest): shared.GetBytecodeArray(broker()).parameter_count_without_receiver();

Samuel Groß . unresolved

I guess this still reads from inside the sandbox (?) so maybe add a comment here that this is either safe or that it would be good to fix in the future?

Open in Gerrit

Related details

Attention is currently required from:

• Darius Mercadier
• Igor Sheludko

Submit Requirements:

• Code-Owners
• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I7ebb4607a47c88115d824a9a56f5443fd404a3f1

Gerrit-Change-Number: 7172454

Gerrit-PatchSet: 7

Gerrit-Owner: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Reviewer: Igor Sheludko <ish...@chromium.org>

Gerrit-Reviewer: Samuel Groß <sa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>

Gerrit-Attention: Igor Sheludko <ish...@chromium.org>

Gerrit-Comment-Date: Tue, 13 Jan 2026 11:59:01 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No