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

v1.9.1 causes panic in writer #1383

Closed
Luap99 opened this issue May 17, 2023 · 0 comments
Closed

v1.9.1 causes panic in writer #1383

Luap99 opened this issue May 17, 2023 · 0 comments

Comments

@Luap99
Copy link
Contributor

Luap99 commented May 17, 2023

Found while updating to the lasted version:

panic: bufio.Scan: too many empty tokens without progressing

goroutine 27 [running]:
bufio.(*Scanner).Scan(0xc000653f28)
        /usr/lib/golang/src/bufio/scan.go:167 +0x82c
github.com/sirupsen/logrus.(*Entry).writerScanner(0x0?, 0xc000014058, 0xc000152140)
        /home/pholzing/go/src/github.com/containers/podman/vendor/github.com/sirupsen/logrus/writer.go:86 +0x11f
created by github.com/sirupsen/logrus.(*Entry).WriterLevel
        /home/pholzing/go/src/github.com/containers/podman/vendor/github.com/sirupsen/logrus/writer.go:57 +0x3d1

This seems to be caused by #1376 which does not look correct. the split function never tells the scanner to stop. Second it even changes the behaviour because it now no longer splits at newlines.

It easy to reproduce, just apply this test to the repo here:

diff --git a/writer_test.go b/writer_test.go
index 5c34927..a6f0b3c 100644
--- a/writer_test.go
+++ b/writer_test.go
@@ -1,10 +1,15 @@
 package logrus_test
 
 import (
+       "bytes"
        "log"
        "net/http"
+       "strings"
+       "testing"
+       "time"
 
        "github.com/sirupsen/logrus"
+       "github.com/stretchr/testify/assert"
 )
 
 func ExampleLogger_Writer_httpServer() {
@@ -32,3 +37,30 @@ func ExampleLogger_Writer_stdlib() {
        // Not logrus imported under the name `log`.
        log.SetOutput(logger.Writer())
 }
+
+func TestWriterSplitNewlines(t *testing.T) {
+       buf := bytes.NewBuffer(nil)
+       logger := logrus.New()
+       logger.Formatter = &logrus.TextFormatter{
+               DisableColors:    true,
+               DisableTimestamp: true,
+       }
+       logger.SetOutput(buf)
+       writer := logger.Writer()
+
+       const logNum = 10
+
+       for i := 0; i < logNum; i++ {
+               _, err := writer.Write([]byte("bar\nfoo\n"))
+               if err != nil {
+                       assert.NoError(t, err, "write.Write failed")
+               }
+       }
+       writer.Close()
+       // Test is flaky because it writes in another goroutine,
+       // we need to make sure to wait a bit so all write are done.
+       time.Sleep(500 * time.Millisecond)
+
+       lines := strings.Split(strings.TrimRight(buf.String(), "\n"), "\n")
+       assert.Len(t, lines, logNum*2, "logger printed incorrect number of lines")
+}
Luap99 added a commit to Luap99/logrus that referenced this issue May 17, 2023
Commit 766cfec 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 te
scanner to stop. See the bufio.ScanLines function, something like:
```
if atEOF && len(data) == 0 {
	return 0, nil, nil
}
```
is need 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 sirupsen#1383

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99 added a commit to Luap99/logrus that referenced this issue May 17, 2023
Commit 766cfec 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 sirupsen#1383

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
ashmckenzie pushed a commit to ashmckenzie/logrus-Luap99-fork that referenced this issue Jun 2, 2023
Commit 766cfec 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 sirupsen#1383

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
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 a pull request may close this issue.

1 participant