gleam
gleam copied to clipboard
servo
Verify the texture buffer for tex_image_2d() call.
r? @glennw @kvark
We might hit the crash when we pass an empty slice to tex_image_2d().
This change is 
I don't understand. Why are we calling it with an empty slice? If we are to verify the slice, we need to ensure proper length, not just check for emptiness of it.
I don't understand. Why are we calling it with an empty slice? If we are to verify the slice, we need to ensure proper length, not just check for emptiness of it.
@kvark This idea comes from: https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_bindings/src/bindings.rs#107 If we pass a null(), then return an empty slice.
The tex_image_2d() already could accept null() and create a empty texture. I think the "empty slice" should have the same semantic. That's why I only convert the empty slice to null() here.
@kvark How about the second part? Is the reason still not strong enough? The api doesn't restrict to pass a non-empty slice only. So, we should do something(update the api document or do something else). Otherwise, we will hit the crash.
The tex_image_2d() already could accept null() and create a empty texture. I think the "empty slice" should have the same semantic. That's why I only convert the empty slice to null() here.
The tex_image_2d() already could accept null() and create a empty texture. I think the "empty slice" should have the same semantic. That's why I only convert the empty slice to null() here.
I consider that being answered as well. Basically, it's just not sound to mix up the semantics of Some(&[]) and None. That's why we have Option as the parameter in the first place.
The api doesn't restrict to pass a non-empty slice only.
The API will also crash if the slice is non-zero but less then the requested data size. Your PR doesn't handle that range of cases, and I see an empty slice just being a sub-case of it.
So, we should do something(update the api document or do something else). Otherwise, we will hit the crash.
We should verify that the slice contains at least the required number of elements in order to prevent the crash. Notice the comment:
// FIXME: Does not verify buffer size -- unsafe!
The tex_image_2d() already could accept null() and create a empty texture. I think the "empty slice" should have the same semantic. That's why I only convert the empty slice to null() here.
It accepts None, not null. We clearly define the difference in semantics: passing Some(thing) means providing data, passing None means just allocating.
I don't see how this can be done exhaustively, given this also involves various UNPACK_* flags which can be set through glPixelStorei and we don't keep track of those in that method. IMO this method should just be unsafe.
:umbrella: The latest upstream changes (presumably #181) made this pull request unmergeable. Please resolve the merge conflicts.
