Skip to content

fix: panic due casting to []rune in WritePrefixedLines#60

Merged
shenwei356 merged 1 commit intoshenwei356:masterfrom
etilite:fix-panic-immediate-lw-unicode
Aug 30, 2024
Merged

fix: panic due casting to []rune in WritePrefixedLines#60
shenwei356 merged 1 commit intoshenwei356:masterfrom
etilite:fix-panic-immediate-lw-unicode

Conversation

@etilite
Copy link
Contributor

@etilite etilite commented Aug 29, 2024

Bug

There is a panic slice bounds out of range in ImmediateLineWriter.WritePrefixedLines cause regex.FindAllStringIndex returns byte indexes and we try to match these indexes in slice of runes which is incorrect when input string has 2 byte symbols.

Way to reproduce

windows (git bash)

seq 1 2 | ./rush --immediate-output printf "один123\nдва1234\nтри123\n"

macOS

seq 1 2 | ./rush --immediate-output echo -e "один123\nдва1234\nтри123\n"

or this simple test demonstrates it

package process

import (
	"sync"
	"testing"
)

func TestImmediateLineWriter_WritePrefixedLines(t *testing.T) {
	const inputThatPanics = `один123
два1234
три123
`
	t.Run("panic occurs", func(t *testing.T) {
		lw := &ImmediateLineWriter{
			lock:    &sync.Mutex{},
			numJobs: 2,
		}

		lw.WritePrefixedLines(inputThatPanics, nil)
	})
}

Fix

By removing unnecessary casting input string to []rune: regex.FindAllStringIndex returns byte indexes so we can directly access string parts by these indexes.

@shenwei356 shenwei356 merged commit 0f80906 into shenwei356:master Aug 30, 2024
@shenwei356
Copy link
Owner

shenwei356 commented Aug 30, 2024

Thanks!

@etilite
Copy link
Contributor Author

etilite commented Aug 30, 2024

@shenwei356 Hello, do you have any plans about new release tag with this fix? It will be nice if I can have one. Thank you!

@shenwei356
Copy link
Owner

Will do it now.

@shenwei356
Copy link
Owner

done.
https://github.com/shenwei356/rush/releases/tag/v0.5.6

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