UB: Bitmap::get_color_of_pattern returns a dangling reference

[parasyte]

playdate/api/gfx/src/bitmap/bitmap.rs

Lines 302 to 313 in 30fc871

	/// Returns 8 x 8 pattern using this bitmap.
	/// `x, y` indicates the top left corner of the 8 x 8 pattern.
	pub fn get_color_of_pattern<'t>(&'t self, x: c_int, y: c_int) -> &mut LCDPattern {
	let mut color: LCDColor = 0;
	let f = self.1.set_color_to_pattern();
	unsafe { f(&mut color, self.0, x, y) }
	assert!(color.is_pattern());
	let ptr = color as *mut c_void as *mut u8 as *mut LCDPattern;
	// TODO: check if ptr.is_null() ...
	unsafe { ptr.as_mut() }.unwrap()
	}

The &mut LCDPattern returned is not dependent on the lifetime of &self, in fact it isn't dependent on any lifetime at all. The pointer is constructed from an integer. Running this code in MIRI (after stripping the sys API parts) reports:

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
     Running `/playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/playground`
warning: integer-to-pointer cast
  --> src/main.rs:37:13
   |
37 |         let ptr = color as *mut c_void as *mut u8 as *mut LCDPattern;
   |                   ^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast
   |
   = help: This program is using integer-to-pointer casts or (equivalently) `ptr::from_exposed_addr`,
   = help: which means that Miri might miss pointer bugs in this program.
   = help: See https://doc.rust-lang.org/nightly/std/ptr/fn.from_exposed_addr.html for more details on that operation.
   = help: To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead.
   = help: You can then pass the `-Zmiri-strict-provenance` flag to Miri, to ensure you are not relying on `from_exposed_addr` semantics.
   = help: Alternatively, the `-Zmiri-permissive-provenance` flag disables this warning.
   = note: BACKTRACE:
   = note: inside `Bitmap::get_color_of_pattern` at src/main.rs:37:13: 37:33
note: inside `main`
  --> src/main.rs:45:14
   |
45 |     let color = bitmap.get_color_of_pattern();
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: Undefined Behavior: dereferencing pointer failed: 0x2a[noalloc] is a dangling pointer (it has no provenance)
   --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mut_ptr.rs:686:57
    |
686 |         if self.is_null() { None } else { unsafe { Some(&mut *self) } }
    |                                                         ^^^^^^^^^^ dereferencing pointer failed: 0x2a[noalloc] is a dangling pointer (it has no provenance)
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `std::ptr::mut_ptr::<impl *mut [u8; 16]>::as_mut::<'_>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mut_ptr.rs:686:57: 686:67
note: inside `Bitmap::get_color_of_pattern`
   --> src/main.rs:39:12
    |
39  |         unsafe { ptr.as_mut() }.unwrap()
    |                  ^^^^^^^^^^^^
note: inside `main`
   --> src/main.rs:45:14
    |
45  |     let color = bitmap.get_color_of_pattern();
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The integer-pointer cast warning suggests that the pointer may have been intended to be created from a reference to color, but this also would not work! That would return a reference to a stack allocation. Short of Bitmap owning some state that this method references, there is no way to return a loan like this.

Another issue is the size disparity between LCDColor and LCDPattern. Even if you did cast an LCDColor reference to an LCDPattern reference, that would result in out-of-bounds accesses.

Probably what was intended is returning an owned LCDColor, which doesn't involve pointer casting shenanigans.

[boozook]

Thanks for the find and research!

From headers in the SDK:

typedef uint8_t LCDPattern[16]; // 8x8 pattern: 8 rows image data, 8 rows mask
typedef uintptr_t LCDColor; // LCDSolidColor or pointer to LCDPattern

1. I'm not sure what's really wrong and what to do 🤷🏻‍♂️. It because I do not see any errors (just assert seems unnecessary and null-check needed), but I must cast int to ptr because of 2. (see next point)
2. Dangling reference (without provenance) is "by-design" as it invented in the SDK. And that's probably ok - just check that int > 3 and so it's a pointer. Yes, pretty unsafe. I'm not sure what we can do with it.

---

Ok, we can wrap LCDPattern into newtype or enum with { PtrToLCDPattern, LCDPattern }. And so... 🤷🏻‍♂️

[boozook]

Ha! There is all wrong, seems to I've got it finally!

[boozook]

@parasyte,
Probably when I wrote this I was tired and sleepy and didn't realize what I was doing.

Is that right solution?

[parasyte]

LOL! Yeah, I like that solution. AFAICT, Bitmap::set_color_to_pattern already does the right thing.