Integrating go-fuzz... #77
Conversation
|
Looking now! 👁 |
worksheets.go
Outdated
| @@ -93,6 +93,8 @@ func NewDefinitions(reader io.Reader, opts ...Options) (*Definitions, error) { | |||
| // Any bad index? | |||
| if field.index == 0 { | |||
| return nil, fmt.Errorf("%s.%s: index cannot be zero", def.name, field.name) | |||
| } else if field.index > 65536 { | |||
There was a problem hiding this comment.
I think most code-readers will understand the significance of 65536, but it might be more literate to express this as math.MaxUint16 + 1, so your intent is clear. Same thing for other instances of this.
parser_test.go
Outdated
| `bool`: &BoolType{}, | ||
| `number[5]`: &NumberType{5}, | ||
| `number[32]`: &NumberType{32}, | ||
| `[]bool`: &SliceType{&BoolType{}}, |
There was a problem hiding this comment.
Do we have simple (non fuzz) tests for other types of slices? If not, might be nice to add those while we're in here. (Not directly related to your PR, but occurred to me from looking through the tests recently.)
There was a problem hiding this comment.
We don't! We could definitely beef up TestParser_parseType to have more examples like []number[4] or [][]foo, etc. Will add to my TODO list #73.
parser.go
Outdated
| panic(err) | ||
| } | ||
| } | ||
| if scale > 32 { |
There was a problem hiding this comment.
I'd be +1 on farming this (32) out to a constant somewhere, so it's easy to reference consistently.
There was a problem hiding this comment.
Yes, we definitely should. We also need to apply this consistently so that round down 33 errors out with the same message around scale being to large. I'll address as part of this change, might as well.
parser.go
Outdated
| if err != nil { | ||
| // unexpected since sIndex should conform to pIndex | ||
| panic(err) | ||
| index := 65536 + 1 |
There was a problem hiding this comment.
(Same note here about 65536.)
parser.go
Outdated
| if len(sScale) <= len("32") { | ||
| scale, err = strconv.Atoi(sScale) | ||
| if err != nil { | ||
| // unexpected since sIndex should conform to pIndex |
There was a problem hiding this comment.
Should this comment be about scales instead of indexes? Looks like maybe just bad copypasta from above. :-)
There was a problem hiding this comment.
yes, fixing
and fixing two small bugs: limiting to no more than 65536 fields, and no more than a scale of 32. (We can lift the scale limitation when we implement arbitrary precision... though having some limit makes sense.)
pascallouisperez
force-pushed
the
go-fuzz
branch
from
f072703
to
b50f52e
Compare
…suring rounding expressions are properly limited, and parsed
and fixing two small bugs: limiting to no more than 65536 fields, and no more than a scale of 32.
(We can lift the scale limitation when we implement arbitrary precision... though having some limit makes sense.)