Bug in LCDPattern

[jhbruhn]

There seems to be a bug in the LCDPattern implementation. When using the draw_line method (and possibly others), the Pattern is drawn with random data (changes for example on button inputs) instead of the selected pattern (in my case a checkerboard for grayscale).

This was only tested on the simulator though, as I don't have real hardware yet!

Here is an example. The leftmost line should have every second pixel on, but it's actually even more (and changes in more complex programs). The second line is supposed to be solid.

#![no_std]

extern crate alloc;

use {
    alloc::boxed::Box,
    anyhow::Error,
    crankstart::{
        crankstart_game,
        graphics::{Graphics, LCDColor, LCDSolidColor},
        system::System,
        Game, Playdate,
    },
    crankstart_sys::{LCDPattern},
    euclid::{point2},
};

const GRAY_PATTERN: LCDPattern = [0x55, 0xAA, 0x55, 0xAA, 0x55, 0xAA, 0x55, 0xAA, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff];

struct State {
}

impl State {
    pub fn new(_playdate: &Playdate) -> Result<Box<Self>, Error> {
        crankstart::display::Display::get().set_refresh_rate(20.0)?;
        Ok(Box::new(Self {
        }))
    }
}

impl Game for State {
    fn update(&mut self, _playdate: &mut Playdate) -> Result<(), Error> {
        let graphics = Graphics::get();
        graphics.clear(LCDColor::Solid(LCDSolidColor::kColorWhite))?;
        graphics.draw_line(point2(20, 0), point2(20, 200), 1, LCDColor::Pattern(GRAY_PATTERN));
        graphics.draw_line(point2(40, 200), point2(40, 0), 1, LCDColor::Solid(LCDSolidColor::kColorBlack));

        System::get().draw_fps(0, 0)?;

        Ok(())
    }
}

crankstart_game!(State);

[boozook]

I suppose that pattern was dropped. Inner implementation of color is

• usize 0..=3 is color
• usize >4 is interpreted as pointer to array 8*8

So there's pointer to const array, temp value on stack. I'll test, but could you try make it static or store anywhere?

There's almost same implementation but with lifetime and ownership.

[parasyte]

Yikes! 😳

Yes, this is a use-after-free. LCDColor::Pattern(GRAY_PATTERN) is constructed on the stack and dropped after from returns: https://github.com/pd-rs/crankstart/blob/5b289e55598af9b90ceda3e24dff354815171ea2/src/graphics.rs#L36-L46

The implementation in @boozook's crate (owned variant) has the same problem for the same reason. The return value from from() points at dropped memory.

---

From what I can tell, the PatternRef variant in the linked crate is sound.

[boozook]

Agreed. This is next in my queue.

[boozook]

Any suggestions? I have no good idea yet, just allow to store only &'static pattern in the LCDColor::Pattern (e.g.). Also maybe allow something with !Unpin 🤷🏻‍♂️

I'll look at it tomorrow. Here is deep night now.

[boozook]

Also there is problem with bitmap.set_color_to_pattern.

1. create color
2. create filled bitmap
3. bitmap.set_color_to_pattern(&mut color) (or something like that)
4. change or drop bitmap Color, as we know, is ptr points to array allocated somewhere. And it isn't changed at step 4! Seems to it's our own array from now. So when it should be feed? What padding, what preserved capacity?

---

Update: No, if try to free it => SIGSEGV in sim. Not tested on the device.

[parasyte]

I believe any lifetime will work. It just might be a bit of an ergonomics issue for users. I tested this yesterday with MIRI and it seemed ok (apart from the int-pointer casts, which raised some warnings).

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=dac65cfbabee99da198f4f99c2d838a3

(edit: this is not in reference to bitmap.set_color_to_pattern)

[parasyte]

Do I take it that the assignment means you want (one of us) to submit a PR for this issue?

[boozook]

It with be great. Of course, no pressure, just say no if you don't want to. The assignment is just a hint to attract attention.