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

`console` should be safe #831

Closed
vkurchatkin opened this Issue Feb 13, 2015 · 16 comments

Comments

Projects
None yet
8 participants
@vkurchatkin
Copy link
Member

vkurchatkin commented Feb 13, 2015

Using console is potentially unsafe:

//index.js
var fs = require('fs')

process.on('uncaughtException', function (err) {
  fs.writeFileSync('./uncaughtException.txt', err.stack)
})

setInterval(ping, 1000)

function ping () {
  console.log('ping')
}

Then:

iojs index.js &
exit

Yields Error: This socket is closed. This reminds me of old versions of IE where console property was defined only when devtools were open.

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Feb 13, 2015

This also relates to nodejs/node-v0.x-archive#6612 right?

What do you think the behaviour should be?

@vkurchatkin

This comment has been minimized.

Copy link
Member Author

vkurchatkin commented Feb 13, 2015

I think log and other methods should do nothing when stdout/stderr are not available

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Feb 14, 2015

@vkurchatkin what do most programming languages do here in this case? (C#, Java, Python, Ruby, C++ etc)

@vkurchatkin

This comment has been minimized.

Copy link
Member Author

vkurchatkin commented Feb 14, 2015

good question. I'll investigate Ruby, Python, go and Objective-C (Cocoa). In Java there are many ways to log something to stdout, but usually System.out.print methods are used. Here are some docs: http://docs.oracle.com/javase/7/docs/api/java/io/PrintStream.html. Basically, they don't throw on error, but set a flag that can be checked with checkError() method.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Feb 17, 2015

@vkurchatkin asked me to comment. It's fairly traditional for properly paranoid UNIX programs to reopen file descriptors 0-2 as /dev/null if they aren't valid file descriptors. I think it's reasonable for io.js to do the same. If opening /dev/null fails, it's probably best to abort.

@vkurchatkin

This comment has been minimized.

Copy link
Member Author

vkurchatkin commented Feb 18, 2015

Not sure how to detect when 0-2 become invalid. Also, is it really necessary to actually reopen them as /dev/null? this is only important if people use fds directly. We can just swap stdio streams implementation. In fact I think that it's fine if process.stdout.write remains unsafe, I'm talking about console specifically

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Feb 18, 2015

Also, is it really necessary to actually reopen them as /dev/null?

Yes, or at least, it's a good idea because it protects against information leaks and worse.

Example: on UNIX platforms, system calls like accept(), open(), socket(), etc. always return the lowest available file descriptor. If stdout is not a valid file descriptor, then the next accept() call uses file descriptor 1 and any logging to stdout gets routed to the socket.

@vkurchatkin

This comment has been minimized.

Copy link
Member Author

vkurchatkin commented Feb 18, 2015

any logging to stdout gets routed to the socket

I see, that's clearly a problem. I would appreciate some implementation tips or maybe an example of " properly paranoid UNIX program" to look at. Also, what about Windows?

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Feb 18, 2015

May I suggest that you review #875? :-)

I don't know about Windows. I don't think you can close stdio on that platform but you'd have to ask @piscisaureus.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Feb 18, 2015

Okay, circling back to file descriptors becoming invalid over the lifetime of the program, I see two possible causes:

  1. The controlling terminal going away because the user logs out. The write() system call will start failing with EIO or (possibly?) ENOTTY.
  2. Application bugs where the user accidentally closes the wrong file descriptor. Writes will start failing with EBADF but may race with other system calls that return new file descriptors.

The first case seems like something io.js can safely ignore. Printing a warning won't do much good anyway.

The second case arguably merits terminating the process. However, users may start filing bogus bug reports if io.js doesn't print the error. We could work around that by opening /dev/tty but what if the open() call fails? Just plain abort(), I guess.

Downright rejecting or ignoring fs.close() calls with fds <= 2 doesn't sound like a good idea because there are valid use cases for closing stdio file descriptors, like programmatically redirecting them to a file.

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Feb 18, 2015

@bnoordhuis if I understand @vkurchatkin correctly he is not suggesting ignoring writes to closed streams if they were redirected or dup2'd onto etc. He is just suggesting ignoring logs (and only from console.log) - in your response this is the first case (closing terminal).

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Mar 11, 2015

Just got bitten by this in a real app - definitely in favor of making console safe :)

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 10, 2016

I've added a proposed test for this in #5639.

It's fragile, dependent on net.js always using the presence of the _handle property to determine if the socket has been closed or not. It does that today but I imagine there's no guarantee it will continue to work that way tomorrow. But I'm not sure there's a better way to have the problem fire reliably on CI. Maybe there's some child_process sorcery I'm unaware of?

@Trott Trott referenced this issue Mar 10, 2016

Closed

test,console: known_issues test for issue 831 #5639

2 of 2 tasks complete

Trott added a commit to Trott/io.js that referenced this issue Mar 10, 2016

test,console: known_issues test for issue 831
This adds a test for the issue described in
nodejs#831.

I've added some metadata fields that I would personally find useful in
these tests.

Refs: nodejs#831

@jasnell jasnell referenced this issue Apr 30, 2016

Closed

process: add process.exitSoon() #6477

0 of 4 tasks complete
@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 6, 2016

I'm going to remove the good first contribution here because, while the issue appears simple, there are a number of complicating factors.

Fishrock123 added a commit to Fishrock123/node that referenced this issue Nov 4, 2016

process: swallow stdout/stderr error events
This is a step in making `console` safe, swallows EPIPE and other
errors by registering an empty 'error' event listener.

Also fixes writes to stdout/stderr not working after `end()` is called,
even though we already override `destroy()` so the stream is never
actually closed.

Refs: nodejs#831
Fixes: nodejs#947
Fixes: nodejs#9403

@Fishrock123 Fishrock123 referenced this issue Nov 4, 2016

Closed

process: swallow stdout/stderr error events #9470

3 of 4 tasks complete
@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 18, 2016

Node's console.log isn't a browser debug tool, its a way to write formatted output. Discarding program output silently on error isn't "safe". I can see how a browser dev would think it was a best-effort debug tool, because that is what it is over in a browser, browsers don't use console to interact with their users, but its fundamentally different in node, despite having the same name.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 18, 2016

If you really want to discard all errors, try process.stdout.on('error',function(){})

@jasnell jasnell referenced this issue Jan 27, 2017

Closed

console: do not emit error events #9744

3 of 3 tasks complete

addaleax added a commit to addaleax/node that referenced this issue Feb 11, 2017

@addaleax addaleax closed this in f18e08d Feb 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment