Add API to set and get the V8 cage base address#3190
Conversation
src/workerd/jsg/setup.h
Outdated
|
|
||
| // Set the location of the pointer cage base for the current isolate. This is only | ||
| // used by getJsCageBase(). | ||
| void setJsCageBase(void* cage_base); |
There was a problem hiding this comment.
Nit: prefer camelCase
src/workerd/jsg/setup.c++
Outdated
| if (!v8Initialized) return; | ||
| v8::Isolate* isolate = v8::Isolate::TryGetCurrent(); | ||
| if (isolate == nullptr) return; | ||
| // Returns null if we are not using pointer cages. |
There was a problem hiding this comment.
I'm not sure this comment makes sense -- probably copy-pasta?
src/workerd/jsg/setup.c++
Outdated
| if (!v8Initialized) return nullptr; | ||
| v8::Isolate* isolate = v8::Isolate::TryGetCurrent(); | ||
| if (isolate == nullptr) return nullptr; | ||
| // Returns null if we are not using pointer cages. |
There was a problem hiding this comment.
Does this comment refer to the line above, where we return nullptr? Or the line below? If the line below, I'm not sure it makes sense, since someone could probably call setJsCageBase() with a non-null value even if we're not using pointer cages, right?
src/workerd/jsg/setup.c++
Outdated
| v8::Isolate* isolate = v8::Isolate::TryGetCurrent(); | ||
| if (isolate == nullptr) return nullptr; | ||
| // Returns null if we are not using pointer cages. | ||
| return isolate->GetData(4); |
There was a problem hiding this comment.
Where does the magic number 4 come from? Can it be hoisted out to a constexpr variable?
| registry.template registerStructProperty<name##_JSG_NAME_DO_NOT_USE_DIRECTLY, \ | ||
| decltype(::kj::instance<Self>().name), &Self::name>() | ||
|
|
||
| enum SetDataIndex { |
There was a problem hiding this comment.
Would you mind adding a documentation/comment for each one of these values?
| return isolate->GetData(SET_DATA_CAGE_BASE); | ||
| } | ||
|
|
||
| void setJsCageBase(void* cageBase) { |
There was a problem hiding this comment.
[nit] Unless you plan to call this from lots of different call sites it seems like these methods mainly serve to silently hide subtle, presumably unexpected error cases (the early returns). Maybe it would be better to just not have these?
There was a problem hiding this comment.
It's a layering issue - Edgeworker doesn't have a lot of files that deal directly with the v8 Isolate, so I thought it made more sense to put it in jsg. The early returns are modelled on getJsStackTrace, and like that function the getJsCageBase is intended to be called during crash handling so it is written in a defensive style.
Add API to set and get the V8 cage base address
I want to use this to call madvise just before a crash so the V8 heap
for the crashing isolate is part of the core dump.