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

Avoid out of bounds when calculating b.URI[startPos:] #69

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

pic4xiu
Copy link
Contributor

@pic4xiu pic4xiu commented Jul 5, 2023

Processing illegal base64 content may cause the program to panic, For example, the following gltf file:

{"buffers": [
    {
      "uri": "data:application/octet-stream;base64",
      "byteLength": 1
    }
  ]
}

When our program calculates the index, the split character is counted by default, and 1 is automatically added, resulting in an out-of-bounds problem in the program.

func (b *Buffer) marshalData() ([]byte, error) {
	if !b.IsEmbeddedResource() {
		return nil, nil
	}
	startPos := len(mimetypeApplicationOctet) + 1//startPos is 37
	//But our b.URI is only 36 in length in this processing.
	sl, err := base64.StdEncoding.DecodeString(b.URI[startPos:])
	if len(sl) == 0 || err != nil {
		return nil, err
	}
	return sl, nil
}

what happened

❯ go run main.go
panic: runtime error: slice bounds out of range [37:36]

goroutine 1 [running]:
github.com/qmuntal/gltf.(*Buffer).marshalData(0x1400012c1e0)
        /Users/housihan/go/pkg/mod/github.com/qmuntal/gltf@v0.24.0/gltf.go:136 +0xdc

Processing illegal base64 content may cause the program to panic
@qmuntal
Copy link
Owner

qmuntal commented Jul 5, 2023

Thanks for reporting and fixing this issue @pic4xiu.

The fix seems OK, I'm just missing a test case.

@pic4xiu
Copy link
Contributor Author

pic4xiu commented Jul 6, 2023

Maybe no one would write like this, I discovered it by accidental testing. But it does cause the program to panic and cause trouble for users.

@qmuntal
Copy link
Owner

qmuntal commented Jul 18, 2023

Maybe no one would write like this, I discovered it by accidental testing. But it does cause the program to panic and cause trouble for users.

I'm still missing a test case that covers the new code. Can you submit it @pic4xiu? Thanks!

@pic4xiu
Copy link
Contributor Author

pic4xiu commented Jul 18, 2023

Hello, please take a look to see if this added test case meets the standard?

@qmuntal
Copy link
Owner

qmuntal commented Jul 18, 2023

Hello, please take a look to see if this added test case meets the standard?

I does. Merging 😄

@qmuntal qmuntal merged commit f894395 into qmuntal:master Jul 18, 2023
7 checks passed
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.

2 participants