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.

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

Jump to bottom

assert: do not read Node.js modules #18322

Closed
wants to merge 2 commits into from
Closed

assert: do not read Node.js modules #18322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74bf73b
assert: do not read Node.js modules
BridgeAR Jan 23, 2018
16f1e48
assert: fix generatedMessage
BridgeAR Jan 31, 2018
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
119 changes: 68 additions & 51 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ const {
isDeepEqual,
isDeepStrictEqual
} = require('internal/util/comparisons');
const { AssertionError, TypeError } = require('internal/errors');
const { AssertionError, TypeError, errorCache } = require('internal/errors');
const { openSync, closeSync, readSync } = require('fs');
const { parseExpressionAt } = require('internal/deps/acorn/dist/acorn');
const { inspect } = require('util');
const { EOL } = require('os');
const nativeModule = require('native_module');

const codeCache = new Map();
// Escape control characters but not \n and \t to keep the line breaks and
// indentation intact.
// eslint-disable-next-line no-control-regex
Expand Down Expand Up @@ -134,11 +134,68 @@ function getBuffer(fd, assertLine) {
return buffers;
}

function getErrMessage(call) {
const filename = call.getFileName();
const line = call.getLineNumber() - 1;
const column = call.getColumnNumber() - 1;
const identifier = `${filename}${line}${column}`;

if (errorCache.has(identifier)) {
return errorCache.get(identifier);
}

// Skip Node.js modules!
if (filename.endsWith('.js') && nativeModule.exists(filename.slice(0, -3))) {
errorCache.set(identifier, undefined);
return;
}

var fd;
try {
fd = openSync(filename, 'r', 0o666);
const buffers = getBuffer(fd, line);
const code = Buffer.concat(buffers).toString('utf8');
const nodes = parseExpressionAt(code, column);
// Node type should be "CallExpression" and some times
// "SequenceExpression".
const node = nodes.type === 'CallExpression' ? nodes : nodes.expressions[0];
const name = node.callee.name;
// Calling `ok` with .apply or .call is uncommon but we use a simple
// safeguard nevertheless.
if (name !== 'apply' && name !== 'call') {
// Only use `assert` and `assert.ok` to reference the "real API" and
// not user defined function names.
const ok = name === 'ok' ? '.ok' : '';
const args = node.arguments;
var message = code
.slice(args[0].start, args[args.length - 1].end)
.replace(escapeSequencesRegExp, escapeFn);
message = 'The expression evaluated to a falsy value:' +
`${EOL}${EOL} assert${ok}(${message})${EOL}`;
}
// Make sure to always set the cache! No matter if the message is
// undefined or not
errorCache.set(identifier, message);

return message;

} catch (e) {
// Invalidate cache to prevent trying to read this part again.
errorCache.set(identifier, undefined);
} finally {
if (fd !== undefined)
closeSync(fd);
}
}

function innerOk(args, fn) {
var [value, message] = args;

if (!value) {
let generatedMessage = false;

if (args.length === 0) {
generatedMessage = true;
message = 'No value argument passed to `assert.ok()`';
} else if (message == null) {
// Use the call as error message if possible.
Expand All @@ -156,62 +213,22 @@ function innerOk(args, fn) {
const call = err.stack[0];
Error.prepareStackTrace = tmpPrepare;

const filename = call.getFileName();
const line = call.getLineNumber() - 1;
const column = call.getColumnNumber() - 1;
const identifier = `${filename}${line}${column}`;

if (codeCache.has(identifier)) {
message = codeCache.get(identifier);
} else {
var fd;
try {
fd = openSync(filename, 'r', 0o666);
const buffers = getBuffer(fd, line);
const code = Buffer.concat(buffers).toString('utf8');
const nodes = parseExpressionAt(code, column);
// Node type should be "CallExpression" and some times
// "SequenceExpression".
const node = nodes.type === 'CallExpression' ?
nodes :
nodes.expressions[0];
// TODO: fix the "generatedMessage property"
// Since this is actually a generated message, it has to be
// determined differently from now on.

const name = node.callee.name;
// Calling `ok` with .apply or .call is uncommon but we use a simple
// safeguard nevertheless.
if (name !== 'apply' && name !== 'call') {
// Only use `assert` and `assert.ok` to reference the "real API" and
// not user defined function names.
const ok = name === 'ok' ? '.ok' : '';
const args = node.arguments;
message = code
.slice(args[0].start, args[args.length - 1].end)
.replace(escapeSequencesRegExp, escapeFn);
message = 'The expression evaluated to a falsy value:' +
`${EOL}${EOL} assert${ok}(${message})${EOL}`;
}
// Make sure to always set the cache! No matter if the message is
// undefined or not
codeCache.set(identifier, message);
} catch (e) {
// Invalidate cache to prevent trying to read this part again.
codeCache.set(identifier, undefined);
} finally {
if (fd !== undefined)
closeSync(fd);
}
}
// Make sure it would be "null" in case that is used.
message = getErrMessage(call) || message;
generatedMessage = true;
} else if (message instanceof Error) {
throw message;
}
innerFail({

const err = new AssertionError({
actual: value,
expected: true,
message,
operator: '==',
stackStartFn: fn
});
err.generatedMessage = generatedMessage;
throw err;
}
}

Expand Down
3 changes: 2 additions & 1 deletion lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,8 @@ module.exports = exports = {
URIError: makeNodeError(URIError),
AssertionError,
SystemError,
E // This is exported only to facilitate testing.
E, // This is exported only to facilitate testing.
errorCache: new Map() // This is in here only to facilitate testing.
};

// To declare an error message, use the E(sym, val) function above. The sym
Expand Down
45 changes: 43 additions & 2 deletions test/parallel/test-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,18 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

// Flags: --expose-internals

'use strict';

/* eslint-disable prefer-common-expectserror */

const common = require('../common');
const assert = require('assert');
const { EOL } = require('os');
const EventEmitter = require('events');
const { errorCache } = require('internal/errors');
const { writeFileSync, unlinkSync } = require('fs');
const a = assert;

function makeBlock(f) {
Expand Down Expand Up @@ -914,6 +919,7 @@ common.expectsError(
{
code: 'ERR_ASSERTION',
type: assert.AssertionError,
generatedMessage: true,
message: `The expression evaluated to a falsy value:${EOL}${EOL} ` +
`assert.ok(null)${EOL}`
}
Expand All @@ -923,6 +929,7 @@ common.expectsError(
{
code: 'ERR_ASSERTION',
type: assert.AssertionError,
generatedMessage: true,
message: `The expression evaluated to a falsy value:${EOL}${EOL} ` +
`assert(typeof 123 === 'string')${EOL}`
}
Expand Down Expand Up @@ -1006,7 +1013,8 @@ common.expectsError(
{
code: 'ERR_ASSERTION',
type: assert.AssertionError,
message: '0 == true'
message: '0 == true',
generatedMessage: true
}
);

Expand All @@ -1015,10 +1023,43 @@ common.expectsError(
{
code: 'ERR_ASSERTION',
type: assert.AssertionError,
message: 'test'
message: 'test',
generatedMessage: false
}
);

// Do not try to check Node.js modules.
Copy link
Member

Choose a reason for hiding this comment

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

This might deserve a separate test file since test-assert.js is already 1000+ lines long..

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 personally would like to keep this test together with the other simple assert tests. I am fine to move all of those to another file though and I agree that this file is a bit long.

{
const e = new EventEmitter();

e.on('hello', assert);

let threw = false;
try {
e.emit('hello', false);
} catch (err) {
const frames = err.stack.split('\n');
const [, filename, line, column] = frames[1].match(/\((.+):(\d+):(\d+)\)/);
Copy link
Member

Choose a reason for hiding this comment

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

This might be affected by internal changes of events and assert...can you look for the frame that contains __filename instead of using hard-coded offsets?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I am not sure I can follow. I explicitly tried to write the test in a way that it will not be affected by internal changes. It should keep on working no matter if the lines are moved or if the specific code is moved to another file. It depends on the thrown error with the information about the filename etc.

Copy link
Member

@joyeecheung joyeecheung Feb 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Uh, so this frame is guaranteed to belong to where EventEmitter.emit is defined and that should be a safe target...never mind then, sorry for the noise.

// Reset the cache to check again
errorCache.delete(`${filename}${line - 1}${column - 1}`);
const data = `${'\n'.repeat(line - 1)}${' '.repeat(column - 1)}` +
'ok(failed(badly));';
try {
writeFileSync(filename, data);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be so late to this, but can you explain why this is writing a file in project root? Is the idea to make sure that assert doesn't try to parse an events.js file when it needs a stack line or something from the internal events module? I don't completely understand what's going on here, but I'd like to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I just saw this right now. What this test makes sure is that if assert.ok is triggered inside of Node.js core code and that Node.js core file has the same name as a user file in the root directory (in this example events.js) it would have read the users file and that is clearly wrong.

assert.throws(
() => e.emit('hello', false),
{
message: 'false == true'
}
);
threw = true;
} finally {
unlinkSync(filename);
}
}
assert(threw);
}

common.expectsError(
// eslint-disable-next-line no-restricted-syntax
() => assert.throws(() => {}, 'Error message', 'message'),
Expand Down