sirupsen / logrus

Structured, pluggable logging for Go.
MIT License
24.59k stars 2.27k forks source link

fix panic in Writer #1384

Closed Luap99 closed 1 year ago

Luap99 commented 1 year ago

Commit 766cfece introduced this bug by defining an incorrect split function. First it breaks the old behavior because it never splits at newlines now. Second, it causes a panic because it never tells the scanner to stop. See the bufio.ScanLines function, something like:

if atEOF && len(data) == 0 {
    return 0, nil, nil
}

is needed to do that.

This commit fixes it by restoring the old behavior and calling bufio.ScanLines but also keep the 64KB check in place to avoid buffering for to long.

Two tests are added to ensure it is working as expected.

Fixes #1383

sirupsen commented 1 year ago

@ashmckenzie @Luap99 I've decided to merge this behaviour and it has been released as v1.9.3. Thank you both for your efforts!

It is the least invasive change, and I'm committed to maintaining as strong backwards compatibility as possible due to the # of users of logrus. Let me know if you find any behaviours with this version

ashmckenzie commented 1 year ago

Thanks @sirupsen and @Luap99, am very pleased to see this merged 🙂:+1::bow:

kke commented 1 year ago

Great! But as noted here https://github.com/k0sproject/k0s/pull/3182#issuecomment-1578691745 - the downside is it's not unicode safe

sirupsen commented 1 year ago

@kke yeah I believe breaking codepoints is acceptable to make this change as minimal as possible. You shouldn't be logging > 64 KiB lines, because as also noted in the comment—it's difficult to ascertain where things came from due to how the log lines are broken up (see https://github.com/sirupsen/logrus/pull/1384#discussion_r1199764806). If you absolutely need to log huge lines like this, you should choose something more performant than logrus :)

kke commented 1 year ago

Yeah, for us the problem comes from an external program which is launched through os/exec and is given logrus a writer as cmd.Stdout, one of the external tools dumped a certificate or whatever in debug mode and made the writer crash.