Skip to content

Commit

Permalink
src: enhance feature access CHECKs during bootstrap
Browse files Browse the repository at this point in the history
This adds `CHECK`s verifying that bootstrapping has finished
before environment variables are accessed or handles/requests
are created. The latter complements a pair of existent checks,
but fails earlier and thus gives information about the call
site, effectively addressing the TODO comment there.

PR-URL: #30452
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
addaleax authored and BridgeAR committed Nov 19, 2019
1 parent fba2f9a commit 592d51c
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 1 deletion.
1 change: 1 addition & 0 deletions src/handle_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ HandleWrap::HandleWrap(Environment* env,
handle_(handle) {
handle_->data = this;
HandleScope scope(env->isolate());
CHECK(env->has_run_bootstrapping_code());
env->handle_wrap_queue()->PushBack(this);
}

Expand Down
3 changes: 2 additions & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,8 @@ MaybeLocal<Value> Environment::RunBootstrapping() {

// Make sure that no request or handle is created during bootstrap -
// if necessary those should be done in pre-execution.
// TODO(joyeecheung): print handles/requests before aborting
// Usually, doing so would trigger the checks present in the ReqWrap and
// HandleWrap classes, so this is only a consistency check.
CHECK(req_wrap_queue()->IsEmpty());
CHECK(handle_wrap_queue()->IsEmpty());

Expand Down
5 changes: 5 additions & 0 deletions src/node_env_var.cc
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ Maybe<bool> KVStore::AssignFromObject(Local<Context> context,
static void EnvGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
CHECK(env->has_run_bootstrapping_code());
if (property->IsSymbol()) {
return info.GetReturnValue().SetUndefined();
}
Expand All @@ -287,6 +288,7 @@ static void EnvSetter(Local<Name> property,
Local<Value> value,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
CHECK(env->has_run_bootstrapping_code());
// calling env->EmitProcessEnvWarning() sets a variable indicating that
// warnings have been emitted. It should be called last after other
// conditions leading to a warning have been met.
Expand Down Expand Up @@ -320,6 +322,7 @@ static void EnvSetter(Local<Name> property,
static void EnvQuery(Local<Name> property,
const PropertyCallbackInfo<Integer>& info) {
Environment* env = Environment::GetCurrent(info);
CHECK(env->has_run_bootstrapping_code());
if (property->IsString()) {
int32_t rc = env->env_vars()->Query(env->isolate(), property.As<String>());
if (rc != -1) info.GetReturnValue().Set(rc);
Expand All @@ -329,6 +332,7 @@ static void EnvQuery(Local<Name> property,
static void EnvDeleter(Local<Name> property,
const PropertyCallbackInfo<Boolean>& info) {
Environment* env = Environment::GetCurrent(info);
CHECK(env->has_run_bootstrapping_code());
if (property->IsString()) {
env->env_vars()->Delete(env->isolate(), property.As<String>());
}
Expand All @@ -340,6 +344,7 @@ static void EnvDeleter(Local<Name> property,

static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
Environment* env = Environment::GetCurrent(info);
CHECK(env->has_run_bootstrapping_code());

info.GetReturnValue().Set(
env->env_vars()->Enumerate(env->isolate()));
Expand Down
1 change: 1 addition & 0 deletions src/req_wrap-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace node {

ReqWrapBase::ReqWrapBase(Environment* env) {
CHECK(env->has_run_bootstrapping_code());
env->req_wrap_queue()->PushBack(this);
}

Expand Down

0 comments on commit 592d51c

Please sign in to comment.