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

A fix to invalid commands causing node-telnet to crash irrevocably. #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dazhazit
Copy link

I've been waiting for a few months for a fix to this so I did it myself. OK, try/catch could have been used but it seemed a cleaner solution to put in guard code where the crash is happening and just ignore the command code or option code entirely.

@dazhazit
Copy link
Author

One unit test fail on one version of node and we have a pull failure?
No offence but the unit tests would never have passed on v0.8.28 with even the original master as AFAIK stream.end() does not accept a function in that version. So I guess it's time to bugfix the unit tests too?
http://nodejs.org/docs/v0.8.28/api/stream.html#stream_stream_end_string_encoding

@georgefrick
Copy link

I assume this is a case such as sending command EL will crash the host? It's better to add the handling for these then to swallow the error.

  1. add EOF, SUSP, and ABORT, as 236, 237, 238. then:

;['eof','susp','abort','ec','el'].forEach(function (command) {
var code = COMMANDS[command.toUpperCase()]
COMMAND_IMPLS[code] = function (bufs, i, event) {
// Needs to be converted to a NOP, we don't want to act on it.
event.buf = bufs.splice(0, i).toBuffer()
event.data = Buffer([ 241 ])
return event
}
})

Really, additional linemode support should be added; but this code will prevent crashes.
I also agree about the unit tests.
Due to lack of reply, a fork is probably in order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants