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

Return an error if the buffer is too large. #354

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

NullHypothesis
Copy link
Contributor

@NullHypothesis NullHypothesis commented Oct 9, 2024

So far, we would cast a uint64 to a int32 but that may result in an integer overflow. At this point, GoBytes does not seem to support buffers this large, so the next best thing to do is return an error if the given buffer is too large.


According to the test below, this check works correctly. I'd rather not add this (or a similar) test because it relies on allocating a 2 GiB buffer.

func TestBufferOverflow(t *testing.T) {
        context, err := NewContext(nil)
        require.NoError(t, err)
        buffer, err := NewBuffer(context)
        require.NoError(t, err)

        require.NoError(t, buffer.SetBuffer(make([]byte, math.MaxInt32+10)))
        buffer.data = nil
        c, err := buffer.dataCopy()
        t.Logf("buf len: %d", len(c))
        t.Log(err)
}

@NullHypothesis NullHypothesis requested a review from a team as a code owner October 9, 2024 14:25
So far, we would cast a uint64 to a int32 but that may result in an
integer overflow if the uint64 cannot fit into the int32.  Since GoBytes
does not support buffer sizes of type uint64, the next best thing to do
is return an error if the given buffer is too large.
Copy link
Contributor

@shaunrd0 shaunrd0 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@ypatia ypatia left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! It will help uncover the places were we ask for serializing more than 2GB of data, and then we'll have to fix those.

@NullHypothesis NullHypothesis merged commit 9e2c19f into master Oct 10, 2024
10 checks passed
@NullHypothesis NullHypothesis deleted the phw/prevent-int-overflow branch October 10, 2024 13:10
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.

3 participants