Skip to content

WindowMaskCellFormat::from(u16) always returns the 1x1 variant (bit-shift bug, width byte lost) #49

Description

@MichaelB-ai91

Summary

impl From<u16> for WindowMaskCellFormat in src/object_pool/object_attributes.rs never recovers the width byte, so every WindowMask read back via ObjectPool::from_iop/reader.rs reports WindowMaskCellFormat::CF1x1, regardless of what was actually written to the .iop file.

Where

src/object_pool/object_attributes.rs, line 98-101 (main branch):

impl From<u16> for WindowMaskCellFormat {
    fn from(value: u16) -> Self {
        WindowMaskCellFormat::from_size((value << 8) as u8, value as u8)
    }
}

from_size(width: u8, height: u8) (line 61) falls back to CF1x1 for any (width, height) pair it doesn't recognize (line 76: _ => WindowMaskCellFormat::CF1x1). Two things go wrong in the From<u16> impl above:

  1. (value << 8) as u8 is 0 for every possible u16 input -- the high byte is shifted out of the 16-bit value before the cast down to u8 truncates it.
  2. Even ignoring that, the two bytes are read back in the wrong order. write_window_mask (src/object_pool/writer.rs, line 62-63) writes cell_format.size().x (width) first, then .y (height), as two separate bytes. read_window_mask (src/object_pool/reader.rs, line 1026) reads them back with Self::read_u16(data)?, whose read_u16 (reader.rs, line 236) does u16::from_le_bytes([a, b]) -- little-endian, so the first byte read (width) ends up as the low byte of value, and the second (height) as the high byte. So value as u8 is actually the width, and (value >> 8) as u8 would be the height -- but the code passes (value << 8) as u8 (always 0) as the width argument and value as u8 (the real width) as the height argument. from_size is therefore always called with width = 0, which matches nothing and falls back to CF1x1.

Reproduction

use ag_iso_stack::object_pool::object_attributes::WindowMaskCellFormat;

// on-wire bytes read_u16 would see for a written CF2x1 (width=2, height=1):
// first byte (width) = 2, second byte (height) = 1 -> le_bytes([2, 1]) = 0x0102
let on_wire: u16 = u16::from_le_bytes([2, 1]);
assert_eq!(WindowMaskCellFormat::from(on_wire), WindowMaskCellFormat::CF2x1); // fails, is CF1x1

More concretely: build any WindowMask object with cell_format: WindowMaskCellFormat::CF2x1, serialize it with ObjectPool::as_iop(), then parse the resulting bytes back with ObjectPool::from_iop(). The round-tripped WindowMask.cell_format comes back as CF1x1.

Impact

Only affects reading a .iop pool back into this library's object model (e.g. round-trip tests, or tools that inspect an existing pool). The bytes written to the .iop file itself are unaffected and ISO 11783-6-correct -- a real Virtual Terminal parses the two raw bytes directly, not through this From<u16> impl, so VT rendering is not affected. Found while writing an external tool that builds a .iop pool with ag-iso-stack and verifies it by reading it back with the same library.

Suggested fix

impl From<u16> for WindowMaskCellFormat {
    fn from(value: u16) -> Self {
        WindowMaskCellFormat::from_size(value as u8, (value >> 8) as u8)
    }
}

i.e. swap which byte gets shifted, and swap the argument order to match from_size(width, height) and the little-endian byte order read_u16 already uses.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions