-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Comments
This also relates to nodejs/node-v0.x-archive#6612 right? What do you think the behaviour should be? |
I think |
@vkurchatkin what do most programming languages do here in this case? (C#, Java, Python, Ruby, C++ etc) |
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 |
@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. |
Not sure how to detect when 0-2 become invalid. Also, is it really necessary to actually reopen them as |
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. |
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? |
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. |
Okay, circling back to file descriptors becoming invalid over the lifetime of the program, I see two possible causes:
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 |
@bnoordhuis if I understand @vkurchatkin correctly he is not suggesting ignoring writes to closed streams if they were redirected or |
Just got bitten by this in a real app - definitely in favor of making console safe :) |
I've added a proposed test for this in #5639. It's fragile, dependent on |
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
I'm going to remove the good first contribution here because, while the issue appears simple, there are a number of complicating factors. |
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
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. |
If you really want to discard all errors, try |
Fixes: nodejs#831 Fixes: nodejs#947 Ref: nodejs#9470
Using
console
is potentially unsafe:Then:
Yields
Error: This socket is closed.
This reminds me of old versions of IE whereconsole
property was defined only when devtools were open.The text was updated successfully, but these errors were encountered: