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

console should be safe #831

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

console should be safe #831

vkurchatkin opened this issue Feb 13, 2015 · 16 comments
Labels
console Issues and PRs related to the console subsystem.

Comments

@vkurchatkin
Copy link
Contributor

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
Copy link
Member

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

What do you think the behaviour should be?

@vkurchatkin
Copy link
Contributor Author

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

@benjamingr
Copy link
Member

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

@vkurchatkin
Copy link
Contributor Author

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:https://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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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

@Trott
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 added a commit to Trott/io.js that referenced this issue Mar 10, 2016
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
@Fishrock123 Fishrock123 added the good first issue Issues that are suitable for first-time contributors. label Aug 3, 2016
@jasnell
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.

@jasnell jasnell removed the good first issue Issues that are suitable for first-time contributors. label Aug 6, 2016
Fishrock123 added a commit to Fishrock123/node that referenced this issue Nov 4, 2016
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
@sam-github
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants