Rust: Converting Vec<u16> to const * u8 makes binding call go haywire

[Ryan1729]

I ran into a bug that was caused an OpenGL call to go haywire and lock up the window.

Here's a repo which more or less minimally reproduces the bug.

As far as I can tell, the problem originates in this part of the code:

let  pixel_ptr : *const u8 = match pixels {
    image::DecodingResult::U8(v) => v.as_ptr(),
    image::DecodingResult::U16(v) => v.as_ptr() as *const u8,
};

In the original program I've fixed the problem by simply not supporting the u16 case and using a Vec instead, like so:

let pixel_vec : Vec<u8> = match pixels {
    image_decoding::DecodingResult::U8(v) => v,
    image_decoding::DecodingResult::U16(_) => panic!("We don't support Deep colour yet!"),
};

Am I right in think that since the conversion is happening outside of the unsafe block, that if that conversion method is incorrect, I should be getting a compile time error?

The original program appeared to work fine on Linux, and the problem only surfaced on Windows.

rustc --version --verbose:

rustc 1.20.0 (f3d6973f4 2017-08-27)
binary: rustc
commit-hash: f3d6973f41a7d1fb83029c9c0ceaf0f5d4fd7208
commit-date: 2017-08-27
host: x86_64-pc-windows-msvc
release: 1.20.0
LLVM version: 4.0

[42triangles]

Converting and creating pointers outside an unsafe block is generally safe, since the actual unsafe thing, dereferencing those pointers, isn't happening.

[Ryan1729]

Ok, good. In that case, since the OpenGL call went haywire even though I'm
loading a 24-bit colour image, which image decodes to a Vec<u8>, this
definitely seems like a Rust bug.

On 10 September 2017 at 06:07, 42triangles [e-mail] wrote:

Converting and creating pointers outside an unsafe block is generally
safe, since the actual unsafe thing, dereferencing those pointers, isn't
happening.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/44473#issuecomment-328338475,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACCMIoHXI0PHuayVRLdWaqeVNRCpFjFvks5sg9ETgaJpZM4PSSzy
.

[42triangles]

This really isn't the fault of rust. You are deallocating the vector in your previous solution (that didn't work), because v goes out of scope within the match, and you use the data outside. And I have searched the OpenGL docs and everything until I noticed that small mistake...

You have basically two options here:

1. Prolong the lifetime of the data behind the Vec indefinitely by mem::forgetting it, or
2. Not consuming it:

let pixel_ptr: *const u8 = match pixels {
     image_decoding::DecodingResult::U8(ref v) => v.as_ptr(),
     image_decoding::DecodingResult::U16(ref v) => v.as_ptr() as _,
};

The ref part causes it to only take a reference of the stored Vec<_> here, so you don't consume it in the process of getting a pointer to its data.

You can close the issue, as this isn't a bug in Rust. You should always ask in the IRC channels #rust and #rust-beginners first ^^

[Ryan1729]

Ah. I hadn't quite realized that was the problem. It all works fine if I
borrow it instead with something like image::DecodingResult::U16(ref v).

If I don't do that and then try to use pixels again outside of the unsafe
block then I get a nice compile error. Would it be unreasonable for the
compiler to notice I'm making a pointer to freed memory in safe Rust and
immediately using it? Maybe it could require an explicit drop(v) or
putting the match in an unsafe block if you really wanted to do that for
some reason? I don't see why someone would do this on purpose, so making
that change would hopefully not break anyone's code.

On 10 September 2017 at 06:17, 42triangles [e-mail] wrote:

This really isn't the fault of rust. You are deallocating the vector in
your previous solution (that didn't work), because v goes out of scope
within the match, and you use the data outside.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/44473#issuecomment-328338918,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACCMIsb-3J5PhsFfQ-27LRX43bZzy_SNks5sg9NGgaJpZM4PSSzy
.

[42triangles]

Rust doesn't check pointers by design (that's why dereferencing them is unsafe). That's what references are for: They are checked. And if you use &v as &[u8], you'll get the compilation error.
Or &v[0] (which is the same without returned length information).

[retep998]

Converting and creating pointers outside an unsafe block is generally safe, since the actual unsafe thing, dereferencing those pointers, isn't happening.

Calling an unsafe function which dereferences the pointer is just as dangerous as dereferencing the pointer yourself.

[arielb1]

This looks like the standard CString::as_c_str crash. We could try to have a lint against such blatant misuse of raw pointers.

[arielb1]

cc @Manishearth - do you think this is something that could be done by Clippy, at least in simple cases such as this?

[Manishearth]

We already have a cstr lint I believe.

Yes, this could be a lint. File an issue with rough heuristics?