Skip to content

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

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.

Sign up for GitHub

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

Closed
bnoordhuis wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f51b943
src: log warning on process.binding('natives')
bnoordhuis Aug 25, 2016
35a62c5
squash! alternative take without $natives
bnoordhuis Aug 25, 2016
dc8e0e6
squash! do not print internal stack frames
bnoordhuis Aug 26, 2016
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
Prev Previous commit
Next Next commit
squash! alternative take without $natives
  • Loading branch information
@bnoordhuis
bnoordhuis committed Aug 25, 2016
commit 35a62c5272c6c2c9b7ade23a8ee3680de1235c0f
2 changes: 1 addition & 1 deletion lib/_debugger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ Client.prototype._addHandle = function(desc) {
};


const natives = process.binding('$natives');
const natives = process.binding('natives');


Client.prototype._addScript = function(desc) {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@
this.loading = false;
}

NativeModule._source = process.binding('$natives');
NativeModule._source = process.binding('natives');
NativeModule._cache = {};

NativeModule.require = function(id) {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/v8_prof_processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const scriptFiles = [
var script = '';

scriptFiles.forEach(function(s) {
script += process.binding('$natives')[s] + '\n';
script += process.binding('natives')[s] + '\n';
});

var tickArguments = [];
Expand Down
9 changes: 9 additions & 0 deletions src/env-inl.h
View file
Open in desktop
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
View file
Open in desktop
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
78 changes: 42 additions & 36 deletions src/node.cc
View file
Open in desktop
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 @@ -2580,16 +2568,17 @@ static void Binding(const FunctionCallbackInfo<Value>& args) {
exports = Object::New(env->isolate());
DefineConstants(env->isolate(), exports);
cache->Set(module, exports);
} else if (!strcmp(*module_v, "$natives") || !strcmp(*module_v, "natives")) {
if (!strcmp(*module_v, "natives")) {
} 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.

@ChALkeR ChALkeR Aug 25, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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);

@ChALkeR ChALkeR Aug 25, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@ChALkeR ChALkeR Aug 25, 2016

Copy link
Copy Markdown
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.

@bnoordhuis bnoordhuis Aug 25, 2016

Copy link
Copy Markdown
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)

@ChALkeR ChALkeR Aug 25, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@bnoordhuis bnoordhuis Aug 25, 2016

Copy link
Copy Markdown
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.

@jasnell jasnell Aug 25, 2016

Copy link
Copy Markdown
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 = v8::StackTrace::CurrentStackTrace(env->isolate(), 12);
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());
Expand All @@ -2600,7 +2589,6 @@ static void Binding(const FunctionCallbackInfo<Value>& args) {
}
exports = Object::New(env->isolate());
DefineJavaScript(env, exports);
cache->Set(module, exports);
} else {
char errmsg[1024];
snprintf(errmsg,
Expand Down Expand Up @@ -3394,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
View file
Open in desktop
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
View file
Open in desktop
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