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

repl: wrong repl stack trace column number in strict mode #5416

Closed
wants to merge 1 commit into from

Conversation

princejwesley
Copy link
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

repl

Description of change

This PR addresses the below problem.

Problem

On REPL strict mode, 'use strict'; void 0; is added as prefix in order to prevent
"use strict" as the result value for let/const statements. It causes wrong column number in
stack trace.

Code for REPL with strict mode
Desktop 🙈  cat strict-repl.js
'use strict';
let net = require('net');
let repl = require('repl')

repl.start({
  replMode: repl.REPL_MODE_STRICT
});

net.createServer(function(socket) {
  repl.start("node via TCP socket> ", socket);
}).listen(5001);
Output
Desktop 🙈  node strict-repl.js
> x = 1
ReferenceError: x is not defined
    at repl:1:25 // <- column number should be 3
    at REPLServer.defaultEval (repl.js:264:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:427:12)
    at emitOne (events.js:90:13)
    at REPLServer.emit (events.js:182:7)
    at REPLServer.Interface._onLine (readline.js:211:10)
    at REPLServer.Interface._line (readline.js:550:8)
    at REPLServer.Interface._ttyWrite (readline.js:827:14)
>
Behavior

Length of 'use strict'; void 0; (22 chars) is added up to column number. ❌

Output (after fix)
node 🙈  git:(upstream  repl-linenumber) ./node ~/Desktop/strict-repl.js
> x = 1
ReferenceError: x is not defined
    at repl:1:3
    at REPLServer.defaultEval (repl.js:264:27)
    at bound (domain.js:281:14)
    at REPLServer.runBound [as eval] (domain.js:294:12)
    at REPLServer.<anonymous> (repl.js:431:12)
    at emitOne (events.js:91:13)
    at REPLServer.emit (events.js:183:7)
    at REPLServer.Interface._onLine (readline.js:216:10)
    at REPLServer.Interface._line (readline.js:555:8)
    at REPLServer.Interface._ttyWrite (readline.js:832:14)
>

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Feb 24, 2016
@@ -289,6 +289,10 @@ function REPLServer(prompt,
debug('domain error');
const top = replMap.get(self);
internalUtil.decorateErrorStack(e);
if (e.stack && self.replMode === exports.REPL_MODE_STRICT) {
e.stack = e.stack.replace(/(\s+at\s+repl:)(\d+)/,
(_, pre, line) => pre + (parseInt(line) - 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need the parseInt() here, "3" - 1 will be faster, and the regex is going to guarantee an integer anyway.

Edit: also the parenthesis around the 2nd math seem to be extraneous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fishrock123 parseInt is removed. parenthesis around subtraction is required because arithmetic operators are left associative and pre is a string.

@Fishrock123
Copy link
Contributor

LGTM once CI is back online so that we can run the tests

@cjihrig
Copy link
Contributor

cjihrig commented Feb 24, 2016

LGTM

On strict mode, "'use strict'; void 0; " is added as prefix
in order to prevent "use strict" as the result value
for let/const statements. It causes wrong column number in
stack trace.
@silverwind
Copy link
Contributor

@princejwesley
Copy link
Contributor Author

@silverwind Green build?

@silverwind
Copy link
Contributor

Yes!

@silverwind
Copy link
Contributor

Thanks! Landed in 40d57b7.

@silverwind silverwind closed this Feb 29, 2016
silverwind pushed a commit that referenced this pull request Feb 29, 2016
On strict mode, "'use strict'; void 0; " is added as prefix
in order to prevent "use strict" as the result value
for let/const statements. It causes wrong column number in
stack trace.

PR-URL: #5416
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

Marked for LTS watch... still need to verify that this bug is relevant for v4, however. /cc @nodejs/lts

Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
On strict mode, "'use strict'; void 0; " is added as prefix
in order to prevent "use strict" as the result value
for let/const statements. It causes wrong column number in
stack trace.

PR-URL: #5416
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@MylesBorins
Copy link
Contributor

one more ping to @nodejs/lts

@MylesBorins
Copy link
Contributor

I've gone through and manually followed the reproduction steps and saw this error on v4.

After backporting the change things are fixed.

As such I'm moving this to staging.

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
On strict mode, "'use strict'; void 0; " is added as prefix
in order to prevent "use strict" as the result value
for let/const statements. It causes wrong column number in
stack trace.

PR-URL: #5416
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
On strict mode, "'use strict'; void 0; " is added as prefix
in order to prevent "use strict" as the result value
for let/const statements. It causes wrong column number in
stack trace.

PR-URL: #5416
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
This was referenced Mar 30, 2016
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
Notable Changes

* https:
  - Under certain conditions ssl sockets may have been causing a memory
  leak when keepalive is enabled. This is no longer the case.
    - (Alexander Penev) #5713

* lib:
  - The way that we were internally passing arguments was causing a
  potential leak. By copying the arguments into an array we can avoid this
    - (Nathan Woltman) #4361

* repl:
  - Previously if you were using the repl in strict mode the column number
  would be wrong in a stack trace. This is no longer an issue.
    - (Prince J Wesley) #5416

PR-URL: #5961
MylesBorins pushed a commit that referenced this pull request Mar 31, 2016
Notable Changes

* https:
  - Under certain conditions ssl sockets may have been causing a memory
  leak when keepalive is enabled. This is no longer the case.
    - (Alexander Penev) #5713

* lib:
  - The way that we were internally passing arguments was causing a
  potential leak. By copying the arguments into an array we can avoid this
    - (Nathan Woltman) #4361

* npm:
  - Upgrade to v2.15.1. (Forrest L Norvell)

* repl:
  - Previously if you were using the repl in strict mode the column number
  would be wrong in a stack trace. This is no longer an issue.
    - (Prince J Wesley) #5416

PR-URL: #5961
MylesBorins pushed a commit that referenced this pull request Mar 31, 2016
Notable Changes

* https:
  - Under certain conditions ssl sockets may have been causing a memory
  leak when keepalive is enabled. This is no longer the case.
    - (Alexander Penev) #5713

* lib:
  - The way that we were internally passing arguments was causing a
  potential leak. By copying the arguments into an array we can avoid this
    - (Nathan Woltman) #4361

* npm:
  - Upgrade to v2.15.1. Fixes a security flaw in the use of authentication
  tokens in HTTP requests that would allow an attacker to set up a server
  that could collect tokens from users of the command-line interface.
  Authentication tokens have previously been sent with every request made
  by the CLI for logged-in users, regardless of the destination of the
  request. This update fixes this by only including those tokens for
  requests made against the registry or registries used for the current
  install. (Forrest L Norvell)

* repl:
  - Previously if you were using the repl in strict mode the column number
  would be wrong in a stack trace. This is no longer an issue.
    - (Prince J Wesley) #5416

PR-URL: #5961
MylesBorins pushed a commit that referenced this pull request Apr 1, 2016
Notable Changes

* https:
  - Under certain conditions ssl sockets may have been causing a memory
  leak when keepalive is enabled. This is no longer the case.
    - (Alexander Penev) #5713

* lib:
  - The way that we were internally passing arguments was causing a
  potential leak. By copying the arguments into an array we can avoid this
    - (Nathan Woltman) #4361

* npm:
  - Upgrade to v2.15.1. Fixes a security flaw in the use of authentication
  tokens in HTTP requests that would allow an attacker to set up a server
  that could collect tokens from users of the command-line interface.
  Authentication tokens have previously been sent with every request made
  by the CLI for logged-in users, regardless of the destination of the
  request. This update fixes this by only including those tokens for
  requests made against the registry or registries used for the current
  install. (Forrest L Norvell)

* repl:
  - Previously if you were using the repl in strict mode the column number
  would be wrong in a stack trace. This is no longer an issue.
    - (Prince J Wesley) #5416

PR-URL: #5961
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants