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

discover native modules from process.moduleLoadList ... #34

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
*.un~
/node_modules
**/temp/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't add these here, especially not ones that are environment-specific (like .idea). For those you should use your global machine .gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of the test, I create the temp folder inside the integration folder. I'm good with removing .idea, but not so much with removing /temp/. I'm good with a more specific path though. See https://github.com/robrich/node-sandboxed-module/blob/master/test/integration/test-all-native-modules.js#L14

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, you should remove any files you create during a test; tests need to clean up after themselves.

.idea
26 changes: 26 additions & 0 deletions lib/native_modules.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//jshint node:true
Copy link
Collaborator

Choose a reason for hiding this comment

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

As much as I am a fan of JSHint and 'use strict', we should stick with this project's style and omit them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm good with that ... kind of. I'm 1/2 tempted to make the whole project pass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not in this commit, at least.

'use strict';

// ordinarily I'd use lodash, but let's use native to avoid the dependency
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this editorial comment in the source code; in the pull request is fine.


var moduleLoadList = process.moduleLoadList;
// an array of 'NativeModule assert'
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the code below, it sounds like a more descriptive comment would be:

// Looks like ['NativeModule assert', 'NativeModule cluster', ...]

or similar. I got confused with the existing comment.


var natives = moduleLoadList.map(function (item) {
// split by space
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for comments that duplicate source code.

return item.split(' ');
}).filter(function (item) {
// filter out invalid entries (if any)
// filter out non 'NativeModule' like 'Binding'
// filter out hidden modules (start with _)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm why would we want to filter out hidden modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think they worked. A quick test shows I was incorrect.

return item && item.length === 2 && item[0] === 'NativeModule' && item[1][0] !== '_';
}).map(function (item) {
// grab the name
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for comments that duplicate source code.

return item[1];
});

if (natives.indexOf('repl') === -1) {
natives.push('repl'); // No idea why repl doesn't show up in process.moduleLoadList
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation in this file is inconsistent with the rest of the code, which uses two spaces.

}

module.exports = natives;
5 changes: 1 addition & 4 deletions lib/sandboxed_module.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ var parent = module.parent;
var globalOptions = {};
var registeredBuiltInSourceTransformers = ['coffee']

var builtinlibs = ['assert', 'buffer', 'child_process', 'cluster',
'crypto', 'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'https','net',
'os', 'path', 'punycode', 'querystring', 'readline', 'stream',
'string_decoder','timers', 'tls', 'tty', 'url', 'util', 'vm', 'zlib', 'smalloc'];
var builtinlibs = require('./native_modules');

module.exports = SandboxedModule;
function SandboxedModule() {
Expand Down
50 changes: 50 additions & 0 deletions test/integration/test-all-native-modules.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
//jshint node:true
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file also needs to conform to the style of the rest of the codebase.

'use strict';

// Assert that we can load all the native modules
// FRAGILE: this tests that all the native modules we know about work
// FRAGILE: it doesn't test that the native_modules list is exhaustive

var assert = require('assert');
var fs = require('fs');
var path = require('path');
var SandboxedModule = require('../..');
var nativeModules = require('../../lib/native_modules');
Copy link
Collaborator

Choose a reason for hiding this comment

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

After thinking about this test for a while, I am not sure it benefits much to test every single native module. Especially since you're relying on the code that enumerates them inside the test itself.

It might be best to just create a single fixture that does require(require('../lib/native_modules')[0]), or maybe does a few of those, and then test that single fixture, in a similar fashion to the rest of the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debated just picking my favorites, but it's this very scenario where our favorite (path) wasn't the problem. Flexing them all runs nearly instantaneously, so I don't see the harm in it. I debated writing one file that required them all, then running SandboxedModule.require() on that file, but then it'd be much less discoverable which module did bad things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be OK based on the error message from your original bug. And more importantly, it would allow us to check in the fixture, instead of generating it at runtime, which makes the test much easier to understand.

You could still use the list from lib/native_modules, just do

require('../../lib/native_modules').forEach(require);

in the fixture.


var temp = path.join(__dirname, 'temp');

fs.mkdir(temp, function (err) {
if (err && err.code === 'EEXIST') {
err = null; // ignore "directory already exists"
}
if (err) {
throw err;
}

// are there any modules?
assert.ok(nativeModules.length > 5, 'error discovering native modules');

nativeModules.forEach(function (modName) {
var filename = path.join(temp, 'fixture_'+modName+'.js');
var requireName = './temp/fixture_'+modName;
var content = 'module.exports.'+modName+' = require(\''+modName+'\');';

// write a file that depends on this module
fs.writeFile(filename, content, function (err) {
if (err) {
throw err;
}

// load that file
var requireModule = SandboxedModule.require(requireName);
var nestDependency = requireModule[modName];
var directDependency = require(modName);
// if we didn't die, we can load this module
assert.ok(requireModule, modName + ' is blank'); // assert not blank
if (modName !== 'module' && modName !== 'repl') { // no idea why these two are different
assert.strictEqual(requireModule[modName], require(modName), modName + ' is different'); // assert we get the same result as just requiring it
}

});
});
});