[extensions] Replace SafeBuiltins v8::Extension with Pure C++ V8 API [chromium/src : main]
Jochen Eisinger (Gerrit)
Jochen Eisinger voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jochen Eisinger
| Commit-Queue | +1 |
1 comment
Related details
- Camillo Bruni
- Devlin Cronin
- Kentaro Hara
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 15 comments![]( "Open in Gerrit")
Thanks, Jochen! This is cool!
void Dispatcher::OnRenderThreadStarted(content::RenderThread* thread) {}Seems like we can get rid of this method now?
const std::unordered_map<std::string, v8::Global<v8::Function>>&we use this type a fair amount. Can we alias it with `using`?
if (!data->Get(context, 0).ToLocal(&target_val) ||
!target_val->IsFunction() || !data->Get(context, 1).ToLocal(&recv)) {
return;
}these are the passed in function and prototype from the safe builtin, passed as a data -- can they ever fail? Or should these be ToChecked()? Ditto below
for (const auto& item : saved) {this can potentially throw v8 exceptions if the prototype were tampered with. For one, those probably *shouldn't* be surfaced to the calling script, but also, I think v8 might not allow calling some methods with pending exceptions?
Should we insert a TryCatch here?
item.proto->DefineProperty(context, to_json_key, desc);I think defining a property like this will override any previous writable, enumerable, configurable attributes that may have existed. Should we cache those and restore them properly?
struct SavedToJSON {
v8::Local<v8::Object> proto;
bool had_own_property = false;
v8::Local<v8::Value> original_descriptor;
};
struct ToJSONRestorer {
v8::Local<v8::Context> context;
v8::Local<v8::String> to_json_key;
std::vector<SavedToJSON> saved;
~ToJSONRestorer() {
v8::Isolate* isolate = v8::Isolate::GetCurrent();
for (const auto& item : saved) {
std::ignore = item.proto->Delete(context, to_json_key);
if (item.had_own_property && !item.original_descriptor.IsEmpty() &&
item.original_descriptor->IsObject()) {
v8::Local<v8::Object> desc_obj =
item.original_descriptor.As<v8::Object>();
v8::Local<v8::Value> get_fn, set_fn, val_val;
bool has_get =
desc_obj
->Get(context, v8_helpers::ToV8StringUnsafe(isolate, "get"))
.ToLocal(&get_fn) &&
!get_fn->IsUndefined();
bool has_set =
desc_obj
->Get(context, v8_helpers::ToV8StringUnsafe(isolate, "set"))
.ToLocal(&set_fn) &&
!set_fn->IsUndefined();
if (has_get || has_set) {
v8::PropertyDescriptor desc(
get_fn.IsEmpty() ? v8::Undefined(isolate).As<v8::Value>()
: get_fn,
set_fn.IsEmpty() ? v8::Undefined(isolate).As<v8::Value>()
: set_fn);
std::ignore =
item.proto->DefineProperty(context, to_json_key, desc);
} else if (desc_obj
->Get(context,
v8_helpers::ToV8StringUnsafe(isolate, "value"))
.ToLocal(&val_val)) {
v8::PropertyDescriptor desc(val_val);
std::ignore =
item.proto->DefineProperty(context, to_json_key, desc);
}
}
}
}
} restorer{context, v8_helpers::ToV8StringUnsafe(isolate, "toJSON")};please add a comment for what this code is doing to help readability
for (uint32_t i = 0; i < count; ++i) {this loop as well, please add a brief comment
std::ignore =this ignore seems a bit dangerous.
As above, I think we should at least catch any errors that come here. But more than that, it means we'll use the overridden toJSON method (which we also know is *not* the same as the original). Should we just abort in that case perhaps?
const std::vector<const char*>& proto_methods,nit: would it make sense for these to be span<>s to avoid allocation of a full array?
ctor_val->IsFunction()) {Throughout this function: this operates on the untainted context, right? Are there times fetching these types of builtins could fail? If so, when?
if (safe_array_.IsEmpty()) {
v8::Isolate* isolate = context_->isolate();
v8::HandleScope handle_scope(isolate);
v8::Local<v8::Context> v8_context = context_->v8_context();
v8::Context::Scope context_scope(v8_context);
v8::Local<v8::Function> ctor;
if (!untainted_->array.ctor.IsEmpty()) {
ctor = untainted_->array.ctor.Get(isolate);
}
v8::Local<v8::Object> safe =
CreateSafeBuiltin(ctor, untainted_->array.proto_methods,
untainted_->array.static_methods);
safe_array_.Reset(isolate, safe);
}
return safe_array_.Get(context_->isolate());this seems to be very heavily duplicated across most of the other Get() methods for function, object, etc. Can we extract the common bits out to a helper method?
"Array.prototype.push = function() { throw new Error('clobbered'); };\n"nit: I think string literals would help readability here (avoiding all the \n and new quotes)
TEST_F(SafeBuiltinsUnittest, TestAllSafeBuiltinsExposed) {I'm not entirely opposed to these, but they seem very verbose and excessive, and could just become change-detector tests. Is there value in exercising each of the individual bits here explicitly, or should we just test a few?
raw_ptr<v8::Isolate> isolate_;add a note that this needs to go above safe_builtins_
Related details
- Camillo Bruni
- Jochen Eisinger
- Kentaro Hara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |