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

fix index out of range panic #362

Closed
wants to merge 1 commit into from
Closed

Conversation

AdamKorcz
Copy link

@AdamKorcz AdamKorcz commented Jun 29, 2023

What this PR does:

Adds a size check, as peekByte() will otherwise panic with an index out of range.

PoC:

package main

import (
        "testing"

        hessian "github.com/apache/dubbo-go-hessian2"
)

func TestPoC(t *testing.T) {
    body = []byte{0x48}
    decoder := hessian.NewDecoder(body)
    _, _ = decoder.Decode()
}

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Signed-off-by: AdamKorcz <adam@adalogics.com>
@tiltwind
Copy link
Contributor

@AdamKorcz if the buffer is empty, which means it's invalid. I wonder when will it happens, can u provide a demo or unit test about this issue?

@AdamKorcz
Copy link
Author

AdamKorcz commented Jun 29, 2023

@AdamKorcz if the buffer is empty, which means it's invalid. I wonder when will it happens, can u provide a demo or unit test about this issue?

Added a reproducer in the PR message.

@tiltwind
Copy link
Contributor

I think the serialization data body = []byte{0x48} is illegal, panic is expected. And there are lots of calling peekByte().
Suggest add recover check for calling decoder.Decode().

@AdamKorcz
Copy link
Author

Is the decoder only meant to accept valid bytes?

Why is it better to panic instead of returning an error, considering that decoder.Decode() returns an error?

@codecov-commenter
Copy link

Codecov Report

Merging #362 (761def6) into master (f0fbe40) will increase coverage by 0.05%.
The diff coverage is 0.00%.

❗ Current head 761def6 differs from pull request most recent head bf588b6. Consider uploading reports for the commit bf588b6 to get more accurate results

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage   68.98%   69.03%   +0.05%     
==========================================
  Files          28       28              
  Lines        3140     3142       +2     
==========================================
+ Hits         2166     2169       +3     
+ Misses        751      749       -2     
- Partials      223      224       +1     
Impacted Files Coverage Δ
map.go 58.97% <0.00%> (-0.62%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tiltwind
Copy link
Contributor

tiltwind commented Jul 2, 2023

Calling readByte() may be better than peekByte() when needs to check buffer length.
I think adding logic for checking buffer length in many places will make the code bloated.

@wongoo
Copy link
Contributor

wongoo commented Nov 1, 2023

close for not active

@wongoo wongoo closed this Nov 1, 2023
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.

None yet

4 participants