Larger canvas at SlotPool::create_buffer

[l4l]

For given buffer, canvas = SlotPool::create_buffer(..), canvas might be bigger than the one returned from buffer.canvas(). This happens because of buffer alignment at the allocation:

client-toolkit/src/shm/slot.rs

Line 257 in 4cf0def

len = (len + 63) & !63;

for the Buffer::canvas it explicitly limits the canvas by height*stride as expected:

client-toolkit/src/shm/slot.rs

Lines 425 to 430 in 4cf0def

	let len = (self.height as usize) * (self.stride as usize);
	if self.slot.inner.active_buffers.load(Ordering::Relaxed) != 0 {
	return None;
	}
	if self.slot.inner.free_list.as_ptr() == Arc::as_ptr(&pool.free_list) {
	Some(&mut pool.inner.mmap()[self.slot.inner.offset..][..len])

while the Slot::raw_data_mut inside create_buffer has only a bound of the allocated slot:

client-toolkit/src/shm/slot.rs

Line 279 in 4cf0def

&mut self.inner.mmap()[slot.inner.offset..][..slot.inner.len]

This implementation is fine if documented. Although I would rather prefer these two to be the same without exposing inner stuff. Also, is SlotPool::raw_data_mut really sound considering aliasing?

[ids1024]

Also, is SlotPool::raw_data_mut really sound considering aliasing?

You mean because the server may also be accessing the same memory? If the server isn't mutating the memory, this isn't unsound in the client (I suppose this API was designed on the assumption the server would only be reading buffers, but it may need some changes for how the new https://wayland.app/protocols/ext-image-copy-capture-v1 protocol uses buffers). On the server-side, this can cause unsoundness if the server creates an & reference into the memory. But the server probably shouldn't depend on correct (or even non-malicious) clients for soundness, so it should avoid relying on this.

[chris-morgan]

I was just writing an example that used raquote like the existing examples. raquote::DrawTarget::from_backing rather reasonably does assert_eq!((width*height) as usize, buf.as_ref().len());. So it panicked because it was ten bytes too long. This seems a fairly big deal to me; it’s a clear violation of expectations, and it bit me at just the time I was making what should have been a harmless refactor, from using .canvas() to using the originally-returned slice.

---

On the safety of raw data access which has also come up here: in my example I want to add to the frame rather than redrawing from scratch, so I was looking into double buffering and copying damage areas between front and back as necessary, and I was wondering why there wasn’t a way of accessing the contents of an active buffer, immutably—because I have been lead to believe that’s safe.

Excerpt from a wayland-devel thread from June 2020 (with minor grammar/typo correction):

You can play tricks like copy back some regions from a previous frame instead of re-rendering them (as it's a read from, not a write to that buffer, it's safe). But beware that readbacks may have issues especially if regions are not mapped as non-cachable because they are being scanned out/displayed for example.

And in the next message, confirmation and clarification of the confusing “issues” bit so that it sounds like it shouldn’t apply here:

Yes, copying from a busy buffer is safe.

[…] the caching thing applies to hardware buffers (some of which can be written to by CPU). But a usual desktop application or a toolkit should not use hardware buffers for software rendering