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

Multi line fix #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

smtlaissezfaire
Copy link

This includes the little upgrades from the other pull request, plus: the multi-line fix.

Here's the multi-line issue. If you are outputting windows friendly text (with \r\n), stripping out \n will lead to some really odd things in unix land. Here's an example:

https://gist.github.com/943750

I ran into this issue when rails was outputting a mailer's contents to the log. It writes \r\n, not straight \n's, which was leading to all but the last line of the mailer cut off.

smtlaissezfaire and others added 7 commits April 27, 2011 00:43
(rspec requires it indirectly, as does rails, so this is only an issue
when using Hodel3000Logger directly from the command line)
prepend prefix to each line in messages rather than squelching newlines.
@noahd1
Copy link
Owner

noahd1 commented May 1, 2011

I'm tempted to pull this, as it seems how I would probably want newlines to be handled, but it's not clear to me how this might impact existing log parsers. Hodel3000 was written (not by me) with the philosophy of being syslog-parser-compatible. Are you familiar with how syslog handles newlines? I did a quick google search and it appears that syslog will replace newlines with spaces.

Maintaining compatibility with syslog seems like the right way to go, though the current behavior is 1) not exactly compatible and 2) buggy with windows newlines as you have pointed out. Thoughts?

@smtlaissezfaire
Copy link
Author

Here's what the syslog man page says:

"Newlines and other non-printable characters embedded in the message
string are printed in an alternate format. This prevents someone from
using non-printable characters to construct misleading log messages in an
output file."

http://www.manpagez.com/man/3/syslog/

I'm not exactly sure what to take out of that sentence. I guess it's to prevent control characters from user input misleading a sysop?

WDYT?

@noahd1
Copy link
Owner

noahd1 commented May 3, 2011

I think your interpretation sounds about right. My tendency is defer to syslog on this, since it's been around a lot longer than its cousin hodel. Do you want to submit a patch to fix windows control characters?

@smtlaissezfaire
Copy link
Author

Yeah, I'd have a tendency to defer to syslog as well if I understood how '\n' would mislead a sysop. I certainly understand the issue with other control characters, though.

It's interesting to note that other loggers (including the stdlib logger) don't remove newlines from a log. So presumably, if it has been a security issue, it's one that hasn't been noticed/fixed by any ruby user in the last 10+ years.

Basically - I'm pulling for my solution because it seems like it would be a real PITA for me to read a logger statement without the newlines in it. Namely, it misleads this sysop (me) when the newlines are replaced with spaces.

@noahd1
Copy link
Owner

noahd1 commented May 5, 2011

Yeah, I can't think of how it would be a security issue. Perhaps in the syslog case, where it's a bit more of the wild-wild west as far as what can get logged, the potential confusion is more of a concern. Ok, I will merge it. I suppose I should note that this does have the potential in a (very) degenerate case for your log files to get filled up with newlines.

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