xpm: Fix various parser panics #7
Conversation
gnoack
force-pushed
the
picons2
branch
2 times, most recently
from
8bda740
to
89f34d3
Compare
Fuzzing support was only introduced in Go 1.18.
The existing code returned nil colors on errors, which leads to crashes.
go.mod
Outdated
| @@ -1,8 +1,14 @@ | |||
| module github.com/fyne-io/image | |||
|
|
|||
| go 1.14 | |||
| go 1.18 | |||
There was a problem hiding this comment.
The upcoming version of Fyne is targeting 1.19 as baseline. You might as well bump it once more.
There was a problem hiding this comment.
Makes sense, will add a commit for that which you can squash with 5bdced5
xpm/xpm.go
Outdated
| if len(data) < charSize { | ||
| return | ||
| return "", nil, fmt.Errorf("invalid format: missing color specification") |
There was a problem hiding this comment.
You are not using any formatting. Consider using errors.New() instead. You might also want to bring it out to a global error variable given that the same error is used in multiple places.
There was a problem hiding this comment.
I introduced an ErrInvalidFormat and return fmt.Errorf("%w: ...") in the error cases, so that callers can tell it apart.
xpm/xpm.go
Outdated
| return | ||
| } | ||
| if ncolors <= 0 { | ||
| err = fmt.Errorf("invalid format: ncolors <= 0: missing color palette") |
There was a problem hiding this comment.
Using fmt.Errorf("%w: ...", ErrInvalidFormat) (see comment above)
xpm/xpm.go
Outdated
| if err != nil { | ||
| return | ||
| } | ||
| j, err = strconv.Atoi(parts[3]) | ||
| if cpp <= 0 { | ||
| err = fmt.Errorf("invalid format: characters per pixel <= 0") |
xpm/xpm.go
Outdated
| func parsePixels(row string, charSize int, pixRow int, colors map[string]color.Color, img *image.NRGBA) { | ||
| func parsePixels(row string, charSize int, pixRow int, colors map[string]color.Color, img *image.NRGBA) error { | ||
| if len(row) < charSize*(img.Stride/4) { | ||
| return fmt.Errorf("invalid format: missing pixel data") |
There was a problem hiding this comment.
Using fmt.Errorf("%w: ...", ErrInvalidFormat) (see comment above)
xpm/xpm.go
Outdated
| off := pixRow * img.Stride | ||
| if len(img.Pix) < off+img.Stride { | ||
| return fmt.Errorf("invalid format: too much pixel data") |
There was a problem hiding this comment.
Using fmt.Errorf("%w: ...", ErrInvalidFormat) (see comment above)
| // parser. To run, use: | ||
| // | ||
| // go test -fuzz=FuzzParsePanic | ||
| func FuzzParsePanic(f *testing.F) { |
There was a problem hiding this comment.
I think it would be ok to run fuzz automatically everyday in a github action. I usually pick 4am as that's when the activity on fyne repository are at their lowest. If you do not mind adding a new action, I think that will be useful to run this fuzzing regularly.
There was a problem hiding this comment.
Fuzzing runs indefinitely if you let it, and I'm not sure whether Github actions are the best fit there. (TBH, I'm not terribly familiar with Github actions either.) I suspect that these considerations would inflate the scope of this change too much. I'd rather leave it up to you to configure this, if that's OK with you. :)
Fixes fyne-io#7 (comment) (Should be a fixup on commit 5bdced5.)
We return fmt.Errorf("%w: ...") in the error cases,
so callers can tell the error apart using `errors.Is()` and
`errors.As()`.
Thanks for the review. I added some commits on top to address these; you might want to squash the go.mod change onto 5bdced5 before merging. Please take another look. |
|
Thanks for working on this :) |
This series of patches adds subtests to test input files against golden PNGs, a fuzzer, and then various changes to fix the detected panics in the parser.