subframe: turn panics into errors #10
Conversation
I believe panics should be reserved to internal invariant violations or irrecoverable errors, not invalid user inputs. Also, they are triggered by go-fuzz, removing them helps testing the package.
|
Hi Patrick, Thanks for spearheading this! I was very fascinated by go-fuzz when reading about it, and it provides a great tool for improving the robustness of packages. As you may have noticed from the panic messages, they all refer to specific parts of the FLAC spec which have not yet been implemented. The main reason is that I simply have not found any FLAC files which uses wasted-bits-per-sample, or are encoded with Rice parameter escape code. The negative shift may be a real issue (I cannot remember if those may occur in valid FLAC files) and should therefore return an error message. If you were to stumble on regular FLAC files (i.e. music files generated by specific FLAC encoding parameters) which triggers any of these cases, I would be very interested in receiving a copy! Once again, thanks for taking the initiative to run go-fuzz on the FLAC package. Do you by any chance have this code in a GitHub repo? It would be interesting to learn about how to hook up go-fuzz. Cheers /u |
subframe: turn panics into errors
|
go-fuzz is a very interesting tool, which I randomly run on third-party packages to see what it can find and how fast. I have pushed my fuzzing branch here: https://github.com/pmezard/flac-1/tree/fuzz The two interesting bits are:
To use it you have to do something like: This thing works well, and find problems quite fast. For instance, the last one before testing your package : That said, running it on image/audio/video codecs often triggers issues with memory allocations which are not necessarily problems with the package. It could either panic upon overallocation (in eaburns case), or make the system swap like crazy (with your package). |
|
Thanks a lot! I got a crash after only two minues, when running
I know, when I first read about it this felt like an impressively effective idea to guide the fuzzing. The combination of genetic algorithms for input generation and code coverage to track what has been tested, and which branches to take. |
|
@mewmew, I can give it a try! |
|
Nice!!! :) |
|
@karlek, make sure to run on the latest version of the code.
|
…3 release notes. Updates #10. (better late than never)
|
I finally got around fuzzing our own library. As mentioned by @pmezard only out of memory panics gets triggered. These can be filtered out by checking the size before allocation. By returning an error for abnormally large allocations, we can fuzz relevant parts of the library. After 8 hours of fuzzing on diff --git a/meta/picture.go b/meta/picture.go
index c0d7951..ff59e59 100644
--- a/meta/picture.go
+++ b/meta/picture.go
@@ -2,6 +2,7 @@ package meta
import (
"encoding/binary"
+ "errors"
"io"
)
@@ -66,6 +67,10 @@ func (block *Block) parsePicture() error {
return unexpected(err)
}
+ // 67108864 == 2**26 ~= 67 MiB
+ if x > 67108864 {
+ return errors.New("picture data too large")
+ }
// (MIME type length) bytes: MIME.
buf, err := readBytes(block.lr, int(x))
if err != nil {
@@ -78,6 +83,10 @@ func (block *Block) parsePicture() error {
return unexpected(err)
}
+ // 67108864 == 2**26 ~= 67 MiB
+ if x > 67108864 {
+ return errors.New("picture data too large")
+ }
// (description length) bytes: Desc.
buf, err = readBytes(block.lr, int(x))
if err != nil {
@@ -113,6 +122,10 @@ func (block *Block) parsePicture() error {
return nil
}
+ // 67108864 == 2**26 ~= 67 MiB
+ if x > 67108864 {
+ return errors.New("picture data too large")
+ }
// (data length) bytes: Data.
pic.Data = make([]byte, x)
_, err = io.ReadFull(block.lr, pic.Data)
diff --git a/meta/seektable.go b/meta/seektable.go
index c0b96ce..2a08b30 100644
--- a/meta/seektable.go
+++ b/meta/seektable.go
@@ -22,6 +22,9 @@ func (block *Block) parseSeekTable() error {
if n < 1 {
return errors.New("meta.Block.parseSeekTable: at least one seek point is required")
}
+ if n > 1000000 {
+ return errors.New("seektable too large")
+ }
table := &SeekTable{Points: make([]SeekPoint, n)}
block.Body = table
var prev uint64
diff --git a/meta/vorbiscomment.go b/meta/vorbiscomment.go
index 78935e3..f7d0e45 100644
--- a/meta/vorbiscomment.go
+++ b/meta/vorbiscomment.go
@@ -2,6 +2,7 @@ package meta
import (
"encoding/binary"
+ "errors"
"fmt"
"strings"
)
@@ -25,6 +26,10 @@ func (block *Block) parseVorbisComment() (err error) {
return unexpected(err)
}
+ // 67108864 == 2**26 ~= 67 MiB
+ if x > 67108864 {
+ return errors.New("vorbis comment data too large")
+ }
// (vendor length) bits: Vendor.
buf, err := readBytes(block.lr, int(x))
if err != nil {
@@ -49,6 +54,10 @@ func (block *Block) parseVorbisComment() (err error) {
return unexpected(err)
}
+ // 67108864 == 2**26 ~= 67 MiB
+ if x > 67108864 {
+ return errors.New("vorbis comment data too large")
+ }
// (vector length): vector.
buf, err = readBytes(block.lr, int(x))
if err != nil { |
Hello,
I have run go-fuzz on the flac package and it eventually triggered panics by supplying invalid user inputs. I believe panics should be reserved to internal invariant violations or irrecoverable errors, not invalid user inputs, so this change turns them into errors.