Skip to content

Commit b9342b7

Browse files
otherdanielCommit Bot
otherdaniel
authored and
Commit Bot
committed
Allow embedder to block or modify eval arguments.
This extends the existing Isolate::SetAllowCodeGenerationFromStringsCallback mechanism, by adding SetModifyCodeGenerationFromStringCallback, which can also modify the eval argument (it could e.g. add escaping). Bug: chromium:940927 Change-Id: I2b72ec2e3b77a5a33f428a0db5cef3f9f8ed6ba2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1593336 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org> Cr-Commit-Position: refs/heads/master@{#62185}
1 parent bc8106d commit b9342b7

9 files changed

Lines changed: 198 additions & 44 deletions

File tree

‎include/v8.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6837,6 +6837,8 @@ typedef void (*FailedAccessCheckCallback)(Local<Object> target,
68376837
*/
68386838
typedef bool (*AllowCodeGenerationFromStringsCallback)(Local<Context> context,
68396839
Local<String> source);
6840+
typedef MaybeLocal<String> (*ModifyCodeGenerationFromStringsCallback)(
6841+
Local<Context> context, Local<Value> source);
68406842

68416843
// --- WebAssembly compilation callbacks ---
68426844
typedef bool (*ExtensionCallback)(const FunctionCallbackInfo<Value>&);
@@ -8437,6 +8439,8 @@ class V8_EXPORT Isolate {
84378439
*/
84388440
void SetAllowCodeGenerationFromStringsCallback(
84398441
AllowCodeGenerationFromStringsCallback callback);
8442+
void SetModifyCodeGenerationFromStringsCallback(
8443+
ModifyCodeGenerationFromStringsCallback callback);
84408444

84418445
/**
84428446
* Set the callback to invoke to check if wasm code generation should

‎src/api/api.cc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8424,6 +8424,9 @@ CALLBACK_SETTER(FatalErrorHandler, FatalErrorCallback, exception_behavior)
84248424
CALLBACK_SETTER(OOMErrorHandler, OOMErrorCallback, oom_behavior)
84258425
CALLBACK_SETTER(AllowCodeGenerationFromStringsCallback,
84268426
AllowCodeGenerationFromStringsCallback, allow_code_gen_callback)
8427+
CALLBACK_SETTER(ModifyCodeGenerationFromStringsCallback,
8428+
ModifyCodeGenerationFromStringsCallback,
8429+
modify_code_gen_callback)
84278430
CALLBACK_SETTER(AllowWasmCodeGenerationCallback,
84288431
AllowWasmCodeGenerationCallback, allow_wasm_code_gen_callback)
84298432

‎src/builtins/builtins-global.cc‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,17 +86,27 @@ BUILTIN(GlobalEval) {
8686
Handle<Object> x = args.atOrUndefined(isolate, 1);
8787
Handle<JSFunction> target = args.target();
8888
Handle<JSObject> target_global_proxy(target->global_proxy(), isolate);
89-
if (!x->IsString()) return *x;
9089
if (!Builtins::AllowDynamicFunction(isolate, target, target_global_proxy)) {
9190
isolate->CountUsage(v8::Isolate::kFunctionConstructorReturnedUndefined);
9291
return ReadOnlyRoots(isolate).undefined_value();
9392
}
93+
94+
// Run embedder pre-checks before executing eval. If the argument is a
95+
// non-String (or other object the embedder doesn't know to handle), then
96+
// return it directly.
97+
MaybeHandle<String> source;
98+
bool unhandled_object;
99+
std::tie(source, unhandled_object) =
100+
Compiler::ValidateDynamicCompilationSource(
101+
isolate, handle(target->native_context(), isolate), x);
102+
if (unhandled_object) return *x;
103+
94104
Handle<JSFunction> function;
95105
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
96106
isolate, function,
97-
Compiler::GetFunctionFromString(handle(target->native_context(), isolate),
98-
Handle<String>::cast(x),
99-
NO_PARSE_RESTRICTION, kNoSourcePosition));
107+
Compiler::GetFunctionFromValidatedString(
108+
handle(target->native_context(), isolate), source,
109+
NO_PARSE_RESTRICTION, kNoSourcePosition));
100110
RETURN_RESULT_OR_FAILURE(
101111
isolate,
102112
Execution::Call(isolate, function, target_global_proxy, 0, nullptr));

‎src/codegen/compiler.cc‎

Lines changed: 101 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1599,33 +1599,103 @@ MaybeHandle<JSFunction> Compiler::GetFunctionFromEval(
15991599
return result;
16001600
}
16011601

1602-
bool Compiler::CodeGenerationFromStringsAllowed(Isolate* isolate,
1603-
Handle<Context> context,
1604-
Handle<String> source) {
1602+
// Check whether embedder allows code generation in this context.
1603+
// (via v8::Isolate::SetAllowCodeGenerationFromStringsCallback)
1604+
bool CodeGenerationFromStringsAllowed(Isolate* isolate, Handle<Context> context,
1605+
Handle<String> source) {
16051606
DCHECK(context->allow_code_gen_from_strings().IsFalse(isolate));
1606-
// Check with callback if set.
1607+
DCHECK(isolate->allow_code_gen_callback());
1608+
1609+
// Callback set. Let it decide if code generation is allowed.
1610+
VMState<EXTERNAL> state(isolate);
1611+
RuntimeCallTimerScope timer(
1612+
isolate, RuntimeCallCounterId::kCodeGenerationFromStringsCallbacks);
16071613
AllowCodeGenerationFromStringsCallback callback =
16081614
isolate->allow_code_gen_callback();
1609-
if (callback == nullptr) {
1610-
// No callback set and code generation disallowed.
1611-
return false;
1612-
} else {
1613-
// Callback set. Let it decide if code generation is allowed.
1614-
VMState<EXTERNAL> state(isolate);
1615-
return callback(v8::Utils::ToLocal(context), v8::Utils::ToLocal(source));
1615+
return callback(v8::Utils::ToLocal(context), v8::Utils::ToLocal(source));
1616+
}
1617+
1618+
// Check whether embedder allows code generation in this context.
1619+
// (via v8::Isolate::SetModifyCodeGenerationFromStringsCallback)
1620+
bool ModifyCodeGenerationFromStrings(Isolate* isolate, Handle<Context> context,
1621+
Handle<i::Object>* source) {
1622+
DCHECK(context->allow_code_gen_from_strings().IsFalse(isolate));
1623+
DCHECK(isolate->modify_code_gen_callback());
1624+
DCHECK(source);
1625+
1626+
// Callback set. Run it, and use the return value as source, or block
1627+
// execution if it's not set.
1628+
VMState<EXTERNAL> state(isolate);
1629+
ModifyCodeGenerationFromStringsCallback modify_callback =
1630+
isolate->modify_code_gen_callback();
1631+
RuntimeCallTimerScope timer(
1632+
isolate, RuntimeCallCounterId::kCodeGenerationFromStringsCallbacks);
1633+
MaybeLocal<v8::String> modified_source =
1634+
modify_callback(v8::Utils::ToLocal(context), v8::Utils::ToLocal(*source));
1635+
if (modified_source.IsEmpty()) return false;
1636+
1637+
// Use the new source (which might be the same as the old source) and return.
1638+
*source = Utils::OpenHandle(*modified_source.ToLocalChecked(), false);
1639+
return true;
1640+
}
1641+
1642+
// Run Embedder-mandated checks before generating code from a string.
1643+
//
1644+
// Returns a string to be used for compilation, or a flag that an object type
1645+
// was encountered that is neither a string, nor something the embedder knows
1646+
// how to handle.
1647+
//
1648+
// Returns: (assuming: std::tie(source, unknown_object))
1649+
// - !source.is_null(): compilation allowed, source contains the source string.
1650+
// - unknown_object is true: compilation allowed, but we don't know how to
1651+
// deal with source_object.
1652+
// - source.is_null() && !unknown_object: compilation should be blocked.
1653+
//
1654+
// - !source_is_null() and unknown_object can't be true at the same time.
1655+
std::pair<MaybeHandle<String>, bool> Compiler::ValidateDynamicCompilationSource(
1656+
Isolate* isolate, Handle<Context> context,
1657+
Handle<i::Object> source_object) {
1658+
Handle<String> source;
1659+
if (source_object->IsString()) source = Handle<String>::cast(source_object);
1660+
1661+
// Check if the context unconditionally allows code gen from strings.
1662+
// allow_code_gen_from_strings can be many things, so we'll always check
1663+
// against the 'false' literal, so that e.g. undefined and 'true' are treated
1664+
// the same.
1665+
if (!context->allow_code_gen_from_strings().IsFalse(isolate)) {
1666+
return {source, !source_object->IsString()};
1667+
}
1668+
1669+
// Check if the context allows code generation for this string.
1670+
// allow_code_gen_callback only allows proper strings.
1671+
// (I.e., let allow_code_gen_callback decide, if it has been set.)
1672+
if (isolate->allow_code_gen_callback()) {
1673+
if (source_object->IsString() &&
1674+
CodeGenerationFromStringsAllowed(isolate, context, source)) {
1675+
return {source, !source_object->IsString()};
1676+
}
16161677
}
1678+
1679+
// Check if the context wants to block or modify this source object.
1680+
// Double-check that we really have a string now.
1681+
// (Let modify_code_gen_callback decide, if it's been set.)
1682+
if (isolate->modify_code_gen_callback()) {
1683+
if (ModifyCodeGenerationFromStrings(isolate, context, &source_object) &&
1684+
source_object->IsString())
1685+
return {Handle<String>::cast(source_object), false};
1686+
}
1687+
1688+
return {MaybeHandle<String>(), !source_object->IsString()};
16171689
}
16181690

1619-
MaybeHandle<JSFunction> Compiler::GetFunctionFromString(
1620-
Handle<Context> context, Handle<String> source,
1691+
MaybeHandle<JSFunction> Compiler::GetFunctionFromValidatedString(
1692+
Handle<Context> context, MaybeHandle<String> source,
16211693
ParseRestriction restriction, int parameters_end_pos) {
16221694
Isolate* const isolate = context->GetIsolate();
16231695
Handle<Context> native_context(context->native_context(), isolate);
16241696

1625-
// Check if native context allows code generation from
1626-
// strings. Throw an exception if it doesn't.
1627-
if (native_context->allow_code_gen_from_strings().IsFalse(isolate) &&
1628-
!CodeGenerationFromStringsAllowed(isolate, native_context, source)) {
1697+
// Raise an EvalError if we did not receive a string.
1698+
if (source.is_null()) {
16291699
Handle<Object> error_message =
16301700
native_context->ErrorMessageForCodeGenerationFromStrings();
16311701
THROW_NEW_ERROR(
@@ -1639,9 +1709,20 @@ MaybeHandle<JSFunction> Compiler::GetFunctionFromString(
16391709
int eval_position = kNoSourcePosition;
16401710
Handle<SharedFunctionInfo> outer_info(
16411711
native_context->empty_function().shared(), isolate);
1642-
return Compiler::GetFunctionFromEval(
1643-
source, outer_info, native_context, LanguageMode::kSloppy, restriction,
1644-
parameters_end_pos, eval_scope_position, eval_position);
1712+
return Compiler::GetFunctionFromEval(source.ToHandleChecked(), outer_info,
1713+
native_context, LanguageMode::kSloppy,
1714+
restriction, parameters_end_pos,
1715+
eval_scope_position, eval_position);
1716+
}
1717+
1718+
MaybeHandle<JSFunction> Compiler::GetFunctionFromString(
1719+
Handle<Context> context, Handle<Object> source,
1720+
ParseRestriction restriction, int parameters_end_pos) {
1721+
Isolate* const isolate = context->GetIsolate();
1722+
Handle<Context> native_context(context->native_context(), isolate);
1723+
return GetFunctionFromValidatedString(
1724+
context, ValidateDynamicCompilationSource(isolate, context, source).first,
1725+
restriction, parameters_end_pos);
16451726
}
16461727

16471728
namespace {

‎src/codegen/compiler.h‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -132,17 +132,22 @@ class V8_EXPORT_PRIVATE Compiler : public AllStatic {
132132
v8::ScriptCompiler::CompileOptions compile_options,
133133
v8::ScriptCompiler::NoCacheReason no_cache_reason);
134134

135-
// Returns true if the embedder permits compiling the given source string in
136-
// the given context.
137-
static bool CodeGenerationFromStringsAllowed(Isolate* isolate,
138-
Handle<Context> context,
139-
Handle<String> source);
140-
141135
// Create a (bound) function for a String source within a context for eval.
142136
V8_WARN_UNUSED_RESULT static MaybeHandle<JSFunction> GetFunctionFromString(
143-
Handle<Context> context, Handle<String> source,
137+
Handle<Context> context, Handle<i::Object> source,
144138
ParseRestriction restriction, int parameters_end_pos);
145139

140+
// Decompose GetFunctionFromString into two functions, to allow callers to
141+
// deal seperately with a case of object not handled by the embedder.
142+
V8_WARN_UNUSED_RESULT static std::pair<MaybeHandle<String>, bool>
143+
ValidateDynamicCompilationSource(Isolate* isolate, Handle<Context> context,
144+
Handle<i::Object> source_object);
145+
V8_WARN_UNUSED_RESULT static MaybeHandle<JSFunction>
146+
GetFunctionFromValidatedString(Handle<Context> context,
147+
MaybeHandle<String> source,
148+
ParseRestriction restriction,
149+
int parameters_end_pos);
150+
146151
// Create a shared function info object for a String source.
147152
static MaybeHandle<SharedFunctionInfo> GetSharedFunctionInfoForScript(
148153
Isolate* isolate, Handle<String> source,

‎src/execution/isolate.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,8 @@ using DebugObjectCache = std::vector<Handle<HeapObject>>;
398398
V(OOMErrorCallback, oom_behavior, nullptr) \
399399
V(LogEventCallback, event_logger, nullptr) \
400400
V(AllowCodeGenerationFromStringsCallback, allow_code_gen_callback, nullptr) \
401+
V(ModifyCodeGenerationFromStringsCallback, modify_code_gen_callback, \
402+
nullptr) \
401403
V(AllowWasmCodeGenerationCallback, allow_wasm_code_gen_callback, nullptr) \
402404
V(ExtensionCallback, wasm_module_callback, &NoExtension) \
403405
V(ExtensionCallback, wasm_instance_callback, &NoExtension) \

‎src/logging/counters.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -893,6 +893,7 @@ class RuntimeCallTimer final {
893893
V(ArrayLengthSetter) \
894894
V(BoundFunctionLengthGetter) \
895895
V(BoundFunctionNameGetter) \
896+
V(CodeGenerationFromStringsCallbacks) \
896897
V(CompileAnalyse) \
897898
V(CompileBackgroundAnalyse) \
898899
V(CompileBackgroundCompileTask) \

‎src/runtime/runtime-compiler.cc‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,8 @@ RUNTIME_FUNCTION(Runtime_CompileForOnStackReplacement) {
294294
return Object();
295295
}
296296

297-
static Object CompileGlobalEval(Isolate* isolate, Handle<String> source,
297+
static Object CompileGlobalEval(Isolate* isolate,
298+
Handle<i::Object> source_object,
298299
Handle<SharedFunctionInfo> outer_info,
299300
LanguageMode language_mode,
300301
int eval_scope_position, int eval_position) {
@@ -303,9 +304,15 @@ static Object CompileGlobalEval(Isolate* isolate, Handle<String> source,
303304

304305
// Check if native context allows code generation from
305306
// strings. Throw an exception if it doesn't.
306-
if (native_context->allow_code_gen_from_strings().IsFalse(isolate) &&
307-
!Compiler::CodeGenerationFromStringsAllowed(isolate, native_context,
308-
source)) {
307+
MaybeHandle<String> source;
308+
bool unknown_object;
309+
std::tie(source, unknown_object) = Compiler::ValidateDynamicCompilationSource(
310+
isolate, native_context, source_object);
311+
// If the argument is an unhandled string time, bounce to GlobalEval.
312+
if (unknown_object) {
313+
return native_context->global_eval_fun();
314+
}
315+
if (source.is_null()) {
309316
Handle<Object> error_message =
310317
native_context->ErrorMessageForCodeGenerationFromStrings();
311318
Handle<Object> error;
@@ -321,9 +328,9 @@ static Object CompileGlobalEval(Isolate* isolate, Handle<String> source,
321328
Handle<JSFunction> compiled;
322329
ASSIGN_RETURN_ON_EXCEPTION_VALUE(
323330
isolate, compiled,
324-
Compiler::GetFunctionFromEval(source, outer_info, context, language_mode,
325-
restriction, kNoSourcePosition,
326-
eval_scope_position, eval_position),
331+
Compiler::GetFunctionFromEval(
332+
source.ToHandleChecked(), outer_info, context, language_mode,
333+
restriction, kNoSourcePosition, eval_scope_position, eval_position),
327334
ReadOnlyRoots(isolate).exception());
328335
return *compiled;
329336
}
@@ -336,11 +343,7 @@ RUNTIME_FUNCTION(Runtime_ResolvePossiblyDirectEval) {
336343

337344
// If "eval" didn't refer to the original GlobalEval, it's not a
338345
// direct call to eval.
339-
// (And even if it is, but the first argument isn't a string, just let
340-
// execution default to an indirect call to eval, which will also return
341-
// the first argument without doing anything).
342-
if (*callee != isolate->native_context()->global_eval_fun() ||
343-
!args[1].IsString()) {
346+
if (*callee != isolate->native_context()->global_eval_fun()) {
344347
return *callee;
345348
}
346349

@@ -350,7 +353,7 @@ RUNTIME_FUNCTION(Runtime_ResolvePossiblyDirectEval) {
350353
DCHECK(args[4].IsSmi());
351354
Handle<SharedFunctionInfo> outer_info(args.at<JSFunction>(2)->shared(),
352355
isolate);
353-
return CompileGlobalEval(isolate, args.at<String>(1), outer_info,
356+
return CompileGlobalEval(isolate, args.at<Object>(1), outer_info,
354357
language_mode, args.smi_at(4), args.smi_at(5));
355358
}
356359
} // namespace internal

‎test/cctest/test-api.cc‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20265,6 +20265,21 @@ bool CodeGenerationDisallowed(Local<Context> context, Local<String> source) {
2026520265
return false;
2026620266
}
2026720267

20268+
v8::MaybeLocal<String> ModifyCodeGeneration(Local<Context> context,
20269+
Local<Value> source) {
20270+
// For testing purposes, deny all odd-length strings and replace '2' with '3'
20271+
String::Utf8Value utf8(context->GetIsolate(), source);
20272+
DCHECK(utf8.length());
20273+
if (utf8.length() == 0 || utf8.length() % 2 != 0)
20274+
return v8::MaybeLocal<String>();
20275+
20276+
for (char* i = *utf8; *i != '\0'; i++) {
20277+
if (*i == '2') *i = '3';
20278+
}
20279+
return String::NewFromUtf8(context->GetIsolate(), *utf8,
20280+
v8::NewStringType::kNormal)
20281+
.ToLocalChecked();
20282+
}
2026820283

2026920284
THREADED_TEST(AllowCodeGenFromStrings) {
2027020285
LocalContext context;
@@ -20297,6 +20312,36 @@ THREADED_TEST(AllowCodeGenFromStrings) {
2029720312
CheckCodeGenerationDisallowed();
2029820313
}
2029920314

20315+
TEST(ModifyCodeGenFromStrings) {
20316+
LocalContext context;
20317+
v8::HandleScope scope(context->GetIsolate());
20318+
context->AllowCodeGenerationFromStrings(false);
20319+
context->GetIsolate()->SetModifyCodeGenerationFromStringsCallback(
20320+
&ModifyCodeGeneration);
20321+
20322+
// Test 'allowed' case in different modes (direct eval, indirect eval,
20323+
// Function constructor, Function contructor with arguments).
20324+
Local<Value> result = CompileRun("eval('42')");
20325+
CHECK_EQ(43, result->Int32Value(context.local()).FromJust());
20326+
20327+
result = CompileRun("(function(e) { return e('42'); })(eval)");
20328+
CHECK_EQ(43, result->Int32Value(context.local()).FromJust());
20329+
20330+
result = CompileRun("var f = new Function('return 42;'); f()");
20331+
CHECK_EQ(43, result->Int32Value(context.local()).FromJust());
20332+
20333+
// Test 'disallowed' cases.
20334+
TryCatch try_catch(CcTest::isolate());
20335+
result = CompileRun("eval('123')");
20336+
CHECK(result.IsEmpty());
20337+
CHECK(try_catch.HasCaught());
20338+
try_catch.Reset();
20339+
20340+
result = CompileRun("new Function('a', 'return 42;')(123)");
20341+
CHECK(result.IsEmpty());
20342+
CHECK(try_catch.HasCaught());
20343+
try_catch.Reset();
20344+
}
2030020345

2030120346
TEST(SetErrorMessageForCodeGenFromStrings) {
2030220347
LocalContext context;

0 commit comments

Comments
 (0)