-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Fix: Cannot read properties of null for setRawMode #41335
Conversation
@@ -72,7 +72,7 @@ ObjectSetPrototypeOf(ReadStream, net.Socket); | |||
|
|||
ReadStream.prototype.setRawMode = function(flag) { | |||
flag = !!flag; | |||
const err = this._handle.setRawMode(flag); | |||
const err = this._handle && this._handle.setRawMode(flag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be covered by a test. Also, is it correct that this function does not fail if this._handle == null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review
I think it's safe as long as this._handle would be null if this function is called when using the terminal.
also, I found this check in readline interface:
node/lib/internal/readline/interface.js
Lines 349 to 350 in f81c627
if (typeof this.input.setRawMode === 'function') { | |
this.input.setRawMode(mode); |
I have covered this issue by a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tniessen What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const err = this._handle && this._handle.setRawMode(flag); | |
const err = this._handle?.setRawMode(flag); |
const r = require('repl').start(); | ||
|
||
// Should not throw. | ||
r.write('process.stdin.push(null)\n'); | ||
r.write('.exit\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test already passes with current version of Node.js, that means it's failing to replicate the issue at hand.
node/doc/guides/collaborator-guide.md
Lines 204 to 205 in 406e6d8
All fixes must have a test case which demonstrates the defect. The test should | |
fail before the change, and pass after the change. |
Superseded by #43803. |
Fixes #41330