displayio, tilepalettemapper, vectorio: arguments narrowed before che…

[peterbay]

Code written by Claude Code, guided and corrected by @peterbay.

The problem

Arguments that reach a uint16_t or a uint8_t field before anything checks them, so a value past the field's range wraps into a valid-looking one, and two indices that are used a step past the end of what they index.

The changes

• TileGrid narrowed four arguments before validating them. tile_width and tile_height went into uint16_t fields, where 65536 truncates to 0 — which is the "use the whole bitmap" value, so it was taken as that rather than refused. width and height did the same, giving a grid of zero tiles. All four are checked as parsed now, and width and height have to be at least one.

• default_tile was never bounded against the bitmap. It indexes the bitmap's tiles, and any value was accepted and stored.

• The tile count was multiplied in int. width * height with both uint16_t promotes to int and overflows for a large grid — 46341 by 46341 is 2147488281, past INT_MAX — and the product was then used as the size of an allocation. It is computed in uint32_t and refused if it will not fit.

• A Group's scale did not fit where the transform kept it. The scale is a uint16_t and the constructor accepts up to 32767, but absolute_transform held it in a uint8_t and the step in an int8_t. Those are widened, the products are computed in uint32_t and clamped, and the setter now has the upper bound the constructor always implied.

• The palette bounds check was off by one. colors holds exactly color_count entries, so an index equal to the count is already past the end; > let it through.

• TilePaletteMapper accepted zero colours per tile and was subscriptable before it was bound. width_in_tiles is only set when the mapper is attached to a TileGrid, and MICROPY_GC_CONSERVATIVE_CLEAR makes it deterministically zero until then, so mapper[0] divided by it. An input pixel past input_color_count also indexed the mapping row unchecked.

• vectorio.Polygon's point count did not fit its own length field. The count is a size_t in the setter, but self->len is a uint16_t holding twice it, and the validation loop counted with a uint16_t as well.

Testing

Seeed XIAO nRF52840 Sense, on two builds differing only by these changes. No display is involved: every row below is an argument that should be refused at the boundary.

	before	after
TileGrid(bmp, tile_width=65536) on a 16x16 bitmap	accepted, tile_width reads 16	ValueError: tile_width must be 0-65535
TileGrid(bmp, tile_height=65536)	accepted, reads 16	ValueError
TileGrid(bmp, width=65536)	accepted, width reads 0	ValueError: width must be 1-65535
TileGrid(bmp, width=0)	accepted, reads 0	ValueError
TileGrid(bmp, tile_width=8, tile_height=8, default_tile=99), four tiles	accepted	ValueError: default_tile must be 0-3
TileGrid(bmp, tile_width=1, tile_height=1, width=46341, height=46341)	accepted	ValueError: TileGrid length must be <= 2147483647
group.scale = 40000	accepted, reads 40000	ValueError: scale must be 1-32767
TilePaletteMapper(palette, 0)	accepted	ValueError: input_color_count must be >= 1

Four of the changes are not in that table. The widened transform, the palette off-by-one and the mapper's input-pixel bound are all on the drawing path, which needs a display to reach. And the polygon point count needs more than 32767 points to show, which is about 130 kB of coordinates before the list of tuples that carries them — more than this board has.

No new translatable strings.

[peterbay]

Testing and diagnostic script.
displayio_arguments_narrowed.py

[FoamyGuy]

scale is declared a uint16_t here:

circuitpython/shared-module/displayio/Group.h

Line 24 in 82b255f

uint16_t scale;

So, theoretically, up to 65535 would fit in this. Though I do think limiting it to something lower makes sense.

Going even lower than 32767 would be fine with me. I don't know whether it's worth using a smaller sized variable, but for visual practicality anything over a scale of a few dozen or maybe hundreds is going to exceed the "a few pixels are now larger than the displays we support" range.

[FoamyGuy]

Thanks for the improved error checking. A few requests and questions.

[FoamyGuy]

input_color_count is uint16_t (

circuitpython/shared-module/tilepalettemapper/TilePaletteMapper.h

Line 23 in 82b255f

uint16_t input_color_count;

)

We can validate the top end of the range also instead of just min here.

[FoamyGuy]

I don't think we need this check here inside of subsc()

I think the right time to check the size of the TileGrid would be when it gets bound to the TilePaletteMapper instead of any time square bracket access is used.

But I don't think we even need to do that because the width and height of TPM are set to the width and height of the TileGrid it gets bound to, and TileGrid has range enforcement on height and width added by this PR

[FoamyGuy]

Why are these changed to uint8_t? In the case of scale at least, I wonder if it would make more sense to adjust the other uses of scale in displayio to be uint8_t and then do proper bounds checking on that size instead of having this move up to uint16_t