Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: log warning on process.binding('natives') #2741

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,15 @@ inline uint64_t Environment::timer_base() const {
return timer_base_;
}

inline int Environment::bootstrap_script_id() const {
CHECK(!internal_script_ids_.empty());
return internal_script_ids_[0];
}

inline std::vector<int>* Environment::internal_script_ids() {
return &internal_script_ids_;
}

inline bool Environment::using_domains() const {
return using_domains_;
}
Expand Down
5 changes: 5 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "v8.h"

#include <stdint.h>
#include <vector>

// Caveat emptor: we're going slightly crazy with macros here but the end
// hopefully justifies the means. We have a lot of per-context properties
Expand Down Expand Up @@ -451,6 +452,9 @@ class Environment {
inline node_ares_task_list* cares_task_list();
inline IsolateData* isolate_data() const;

inline int bootstrap_script_id() const;
inline std::vector<int>* internal_script_ids();

inline bool using_domains() const;
inline void set_using_domains(bool value);

Expand Down Expand Up @@ -578,6 +582,7 @@ class Environment {
uint32_t* heap_space_statistics_buffer_ = nullptr;

char* http_parser_buffer_;
std::vector<int> internal_script_ids_;

#define V(PropertyName, TypeName) \
v8::Persistent<TypeName> PropertyName ## _;
Expand Down
89 changes: 56 additions & 33 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
#include <string.h>
#include <sys/types.h>

#include <algorithm>
#include <string>
#include <vector>

Expand Down Expand Up @@ -123,6 +124,7 @@ using v8::PromiseRejectMessage;
using v8::PropertyCallbackInfo;
using v8::ScriptOrigin;
using v8::SealHandleScope;
using v8::StackTrace;
using v8::String;
using v8::TryCatch;
using v8::Uint32Array;
Expand Down Expand Up @@ -1659,35 +1661,6 @@ static void ReportException(Environment* env, const TryCatch& try_catch) {
}


// Executes a str within the current v8 context.
static Local<Value> ExecuteString(Environment* env,
Local<String> source,
Local<String> filename) {
EscapableHandleScope scope(env->isolate());
TryCatch try_catch(env->isolate());

// try_catch must be nonverbose to disable FatalException() handler,
// we will handle exceptions ourself.
try_catch.SetVerbose(false);

ScriptOrigin origin(filename);
MaybeLocal<v8::Script> script =
v8::Script::Compile(env->context(), source, &origin);
if (script.IsEmpty()) {
ReportException(env, try_catch);
exit(3);
}

Local<Value> result = script.ToLocalChecked()->Run();
if (result.IsEmpty()) {
ReportException(env, try_catch);
exit(4);
}

return scope.Escape(result);
}


static void GetActiveRequests(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down Expand Up @@ -2543,6 +2516,21 @@ void ClearFatalExceptionHandlers(Environment* env) {
}


int GetCallerScriptId(v8::Isolate* isolate) {
auto options = StackTrace::kScriptId;
auto stack_trace = StackTrace::CurrentStackTrace(isolate, 1, options);
if (stack_trace->GetFrameCount() > 0)
return stack_trace->GetFrame(0)->GetScriptId();
return Message::kNoScriptIdInfo;
}


inline bool IsInternalScriptId(Environment* env, int script_id) {
auto ids = env->internal_script_ids();
return ids->end() != std::find(ids->begin(), ids->end(), script_id);
}


static void Binding(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down Expand Up @@ -2581,9 +2569,26 @@ static void Binding(const FunctionCallbackInfo<Value>& args) {
DefineConstants(env->isolate(), exports);
cache->Set(module, exports);
} else if (!strcmp(*module_v, "natives")) {
auto caller_script_id = GetCallerScriptId(env->isolate());
if (!IsInternalScriptId(env, caller_script_id)) {
// graceful-fs < 4 evals process.binding('natives').fs, which prohibits
// using internal modules in that module. Encourage people to upgrade.
Copy link
Member

@ChALkeR ChALkeR Aug 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which prohibits using internal modules in that module.

This is not accurate any more, they evaded that, and it now reevalutes internal modules with more dark magic, internal API usage, and relying on implementation details.

auto pid = getpid();
fprintf(stderr,
"(node:%d) process.binding('natives') is deprecated.\n", pid);
fprintf(stderr,
"(node:%d) If you use graceful-fs < 4, please update.\n", pid);
Copy link
Member

@ChALkeR ChALkeR Aug 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do recent versions of graceful-fs@3 still trigger this? It was updated just recently.
Update: they do, I verified that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question: do we need to specifically note graceful-fs here? It's not the only package that uses process.binding('natives'), and that could lead people away from the actual issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flashback: #2741 (comment)

Copy link
Member

@ChALkeR ChALkeR Aug 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis yes, but see the list at #2741 (comment), that wasn't considered back then.

Also, graceful-fs@2 and graceful-fs@3 usage significantly decreased in past months.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I could check if the caller is graceful-fs and customize the error based on that but meh. I have to be selective with my time in the next few weeks so I'm probably not going to expend too much effort on this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't have the time to work on it, I am willing to take it over.

auto stack_trace = StackTrace::CurrentStackTrace(env->isolate(), 12);
for (int i = 0, n = stack_trace->GetFrameCount(); i < n; i += 1) {
Local<v8::StackFrame> frame = stack_trace->GetFrame(i);
node::Utf8Value name(env->isolate(), frame->GetScriptName());
fprintf(stderr,
"(node:%d) at %s:%d\n", pid, *name, frame->GetLineNumber());
}
fflush(stderr);
}
exports = Object::New(env->isolate());
DefineJavaScript(env, exports);
cache->Set(module, exports);
} else {
char errmsg[1024];
snprintf(errmsg,
Expand Down Expand Up @@ -3377,14 +3382,32 @@ void LoadEnvironment(Environment* env) {
// 'internal_bootstrap_node_native' is the string containing that source code.
Local<String> script_name = FIXED_ONE_BYTE_STRING(env->isolate(),
"bootstrap_node.js");
Local<Value> f_value = ExecuteString(env, MainSource(env), script_name);
ScriptOrigin origin(script_name);
Local<v8::Script> script;
auto maybe_script =
v8::Script::Compile(env->context(), MainSource(env), &origin);
if (!maybe_script.ToLocal(&script)) {
ReportException(env, try_catch);
exit(3);
}

auto internal_script_ids = env->internal_script_ids();
CHECK(internal_script_ids->empty());
internal_script_ids->push_back(script->GetUnboundScript()->GetId());

Local<Value> result = script->Run();
if (result.IsEmpty()) {
ReportException(env, try_catch);
exit(4);
}

if (try_catch.HasCaught()) {
ReportException(env, try_catch);
exit(10);
}
// The bootstrap_node.js file returns a function 'f'
CHECK(f_value->IsFunction());
Local<Function> f = Local<Function>::Cast(f_value);
CHECK(result->IsFunction());
Local<Function> f = Local<Function>::Cast(result);

// Now we call 'f' with the 'process' variable that we've built up with
// all our bindings. Inside bootstrap_node.js we'll take care of
Expand Down
20 changes: 12 additions & 8 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -515,20 +515,18 @@ class ContextifyScript : public BaseObject {
else if (produce_cached_data)
compile_options = ScriptCompiler::kProduceCodeCache;

MaybeLocal<UnboundScript> v8_script = ScriptCompiler::CompileUnboundScript(
env->isolate(),
&source,
compile_options);

if (v8_script.IsEmpty()) {
Local<UnboundScript> script;
auto maybe_script =
ScriptCompiler::CompileUnboundScript(env->isolate(), &source,
compile_options);
if (!maybe_script.ToLocal(&script)) {
if (display_errors) {
DecorateErrorStack(env, try_catch);
}
try_catch.ReThrow();
return;
}
contextify_script->script_.Reset(env->isolate(),
v8_script.ToLocalChecked());
contextify_script->script_.Reset(env->isolate(), script);

if (compile_options == ScriptCompiler::kConsumeCodeCache) {
args.This()->Set(
Expand All @@ -548,6 +546,12 @@ class ContextifyScript : public BaseObject {
env->cached_data_produced_string(),
Boolean::New(env->isolate(), cached_data_produced));
}

auto caller_script_id = GetCallerScriptId(env->isolate());
if (caller_script_id == env->bootstrap_script_id()) {
// Called by NativeModule.require().
env->internal_script_ids()->push_back(script->GetId());
}
}


Expand Down
4 changes: 4 additions & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ void SetupProcessObject(Environment* env,
int exec_argc,
const char* const* exec_argv);

// Returns the script id of the top-most JS frame or v8::Message::kNoScriptId
// if no such frame exists or if the frame cannot be mapped to a script.
int GetCallerScriptId(v8::Isolate* isolate);

enum Endianness {
kLittleEndian, // _Not_ LITTLE_ENDIAN, clashes with endian.h.
kBigEndian
Expand Down