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

[Golang] Add bounds-checked decoding functions #8244

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ricardonunez-io
Copy link

"Safe"-prepended "Get" decoding functions in the Go package include bounds checking to avoid panics for situations where safety from untrusted inputs is critical and performance isn't as necessary.

Does not modify existing functions, simply adds 15 new functions for each type of decoding available in go/encode.go.

Does not include safety measures for decoding strings.

"Safe"-prepended "Get" decoding functions include bounds checking to avoid panics for situations where safety from untrusted inputs is critical and performance isn't as necessary. Does not modify existing functions, simply adds 15 new functions for each type of decoding available in `go/encode.go`. Does not include safety measures for decoding strings.
Correct buffer length check from 4 to 8
@ricardonunez-io ricardonunez-io changed the title GOLANG: Add bounds-checked decoding functions [Golang] Add bounds-checked decoding functions Feb 29, 2024
@jdemeyer
Copy link
Contributor

How is one supposed to use these functions? Ideally these would be used in safe variants of the generated code.

Also, there is clearly some functional overlap with #8030

// SafeGetBool first bounds checks the byte slice to ensure a length of 1
// then decodes a little-endian bool from the byte slice
func SafeGetBool(buf []byte) (bool, error) {
if len(buf) != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking the length exactly, this should probably be if len(buf) < 1 (same in other functions)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jeroen! Thanks for the review.

Instead of checking the length exactly, this should probably be if len(buf) < 1 (same in other functions)

Wouldn't this cause this function to incorrectly pass the bounds check if buf is an empty slice and/or a slice of length n - 1 when checking for length n?

How is one supposed to use these functions? Ideally these would be used in safe variants of the generated code.

Yes, you're correct, these would have to be used in the generated code as safe options through a compiler flag, I missed that, I'll work on adding that to the Go code generator.

Also, there is clearly some functional overlap with #8030

There is definitely overlap in its utility, but verifier is more comprehensive in its implementation in verifying each table, buffer offsets, table depth/complexity, etc. and thus incurs more overhead.

These functions are to safely retrieve scalar values with as minimal overhead as possible, given that the original functions already force bounds checks but panic instead of throw.

However, let me know if you think the project would see further utility in these safe scalar retrieval functions, and I'll work on getting codegen working through an --include-safe flag.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants