Verify the texture buffer for tex_image_2d() call.

[JerryShih]

r? @glennw @kvark

We might hit the crash when we pass an empty slice to tex_image_2d().

---

This change is 

[emilio]

What about:

let ptr = match opt_data {
    Some(data) => {
        if data.is_empty() { ptr::null() } else { data.as_ptr() }
    }
    None => ptr::null(),
};

And just a single call after that?

[jdm]

let ptr = match opt_data {
    Some(data) if !data.is_empty() => data.as_ptr(),
    _ => ptr::null(),
};

[emilio]

Looks good, with those.

[emilio]

Also, this is still true (we don't verify that there is enough data on the buffer for the given format, width and height), so please add these comments back.

[JerryShih]

r? @emilio @jdm
update for review comment.

[kvark]

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.

[JerryShih]

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]

@JerryShih just because the Gecko binding code is incorrect, doesn't mean that gleam should accommodate to it. Gecko should pass None instead of Option<&[]>.

[JerryShih]

@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.

[kvark]

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.

[nox]

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.

[bors-servo]

☔ The latest upstream changes (presumably #181) made this pull request unmergeable. Please resolve the merge conflicts.