Skip to content

displayio, tilepalettemapper, vectorio: arguments narrowed before che… #11382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
peterbay wants to merge 2 commits into
base: main
Choose a base branch
from
Open

displayio, tilepalettemapper, vectorio: arguments narrowed before che… #11382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91783e6
displayio, tilepalettemapper, vectorio: arguments narrowed before che…
peterbay Sep 14, 2026
d36f069
Merge branch 'main' into displayio-arguments-narrowed-before-checked
tannewt Sep 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shared-bindings/displayio/Group.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ MP_DEFINE_CONST_FUN_OBJ_1(displayio_group_get_scale_obj, displayio_group_obj_get
static mp_obj_t displayio_group_obj_set_scale(mp_obj_t self_in, mp_obj_t scale_obj) {
displayio_group_t *self = native_group(self_in);

mp_int_t scale = mp_arg_validate_int_min(mp_obj_get_int(scale_obj), 1, MP_QSTR_scale);
mp_int_t scale = mp_arg_validate_int_range(mp_obj_get_int(scale_obj), 1, 32767, MP_QSTR_scale);

@FoamyGuy FoamyGuy Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


common_hal_displayio_group_set_scale(self, scale);
return mp_const_none;
Expand Down
17 changes: 16 additions & 1 deletion shared-bindings/displayio/TileGrid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ static mp_obj_t displayio_tilegrid_make_new(const mp_obj_type_t *type, size_t n_
}
mp_obj_t pixel_shader = args[ARG_pixel_shader].u_obj;
displayio_tilegrid_validate_pixel_shader(pixel_shader);
mp_arg_validate_int_range(args[ARG_tile_width].u_int, 0, 0xffff, MP_QSTR_tile_width);
mp_arg_validate_int_range(args[ARG_tile_height].u_int, 0, 0xffff, MP_QSTR_tile_height);
uint16_t tile_width = args[ARG_tile_width].u_int;
if (tile_width == 0) {
tile_width = bitmap_width;
Expand All @@ -118,19 +120,32 @@ static mp_obj_t displayio_tilegrid_make_new(const mp_obj_type_t *type, size_t n_
if (tile_height == 0) {
tile_height = bitmap_height;
}
mp_arg_validate_int_min(tile_width, 1, MP_QSTR_tile_width);
mp_arg_validate_int_min(tile_height, 1, MP_QSTR_tile_height);
if (bitmap_width % tile_width != 0) {
mp_raise_ValueError(MP_ERROR_TEXT("Tile width must exactly divide bitmap width"));
}
if (bitmap_height % tile_height != 0) {
mp_raise_ValueError(MP_ERROR_TEXT("Tile height must exactly divide bitmap height"));
}

mp_arg_validate_int_range(args[ARG_width].u_int, 1, 0xffff, MP_QSTR_width);
mp_arg_validate_int_range(args[ARG_height].u_int, 1, 0xffff, MP_QSTR_height);

uint16_t bitmap_width_in_tiles = bitmap_width / tile_width;
uint16_t bitmap_height_in_tiles = bitmap_height / tile_height;
uint32_t tiles_in_bitmap = (uint32_t)bitmap_width_in_tiles * bitmap_height_in_tiles;
mp_arg_validate_length_min(tiles_in_bitmap, 1, MP_QSTR_bitmap);

mp_arg_validate_int_range(args[ARG_default_tile].u_int, 0,
(mp_int_t)tiles_in_bitmap - 1, MP_QSTR_default_tile);

int16_t x = args[ARG_x].u_int;
int16_t y = args[ARG_y].u_int;

displayio_tilegrid_t *self = mp_obj_malloc(displayio_tilegrid_t, &displayio_tilegrid_type);
common_hal_displayio_tilegrid_construct(self, bitmap,
bitmap_width / tile_width, bitmap_height / tile_height,
bitmap_width_in_tiles, bitmap_height_in_tiles,
pixel_shader, args[ARG_width].u_int, args[ARG_height].u_int,
tile_width, tile_height, x, y, args[ARG_default_tile].u_int);

Expand Down
7 changes: 6 additions & 1 deletion shared-bindings/tilepalettemapper/TilePaletteMapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ static mp_obj_t tilepalettemapper_tilepalettemapper_make_new(const mp_obj_type_t
mp_raise_TypeError_varg(MP_ERROR_TEXT("unsupported %q type"), MP_QSTR_pixel_shader);
}

mp_arg_validate_int_min(args[ARG_input_color_count].u_int, 1, MP_QSTR_input_color_count);

@FoamyGuy FoamyGuy Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tilepalettemapper_tilepalettemapper_t *self = mp_obj_malloc(tilepalettemapper_tilepalettemapper_t, &tilepalettemapper_tilepalettemapper_type);
common_hal_tilepalettemapper_tilepalettemapper_construct(self, pixel_shader, args[ARG_input_color_count].u_int);

Expand Down Expand Up @@ -128,9 +130,12 @@ static mp_obj_t tilepalettemapper_subscr(mp_obj_t self_in, mp_obj_t index_obj, m
} else {
uint16_t x = 0;
uint16_t y = 0;
uint16_t width = common_hal_tilepalettemapper_tilepalettemapper_get_width(self);
if (width == 0) {
mp_raise_IndexError(MP_ERROR_TEXT("Tile index out of bounds"));

@FoamyGuy FoamyGuy Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if (mp_obj_is_small_int(index_obj)) {
mp_int_t i = MP_OBJ_SMALL_INT_VALUE(index_obj);
uint16_t width = common_hal_tilepalettemapper_tilepalettemapper_get_width(self);
x = i % width;
y = i / width;
} else {
Expand Down
8 changes: 5 additions & 3 deletions shared-module/displayio/Group.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ void displayio_group_update_transform(displayio_group_t *self,
self->absolute_transform.mirror_x = parent_transform->mirror_x;
self->absolute_transform.mirror_y = parent_transform->mirror_y;

self->absolute_transform.scale = parent_transform->scale * self->scale;
uint32_t combined = (uint32_t)parent_transform->scale * self->scale;
self->absolute_transform.scale = combined > UINT16_MAX ? UINT16_MAX : (uint16_t)combined;
}
_update_child_transforms(self);
}
Expand All @@ -197,10 +198,11 @@ void common_hal_displayio_group_set_scale(displayio_group_t *self, uint32_t scal
return;
}
check_readonly(self);
uint8_t parent_scale = self->absolute_transform.scale / self->scale;
uint16_t parent_scale = self->absolute_transform.scale / self->scale;
self->absolute_transform.dx = self->absolute_transform.dx / self->scale * scale;
self->absolute_transform.dy = self->absolute_transform.dy / self->scale * scale;
self->absolute_transform.scale = parent_scale * scale;
uint32_t combined = (uint32_t)parent_scale * scale;
self->absolute_transform.scale = combined > UINT16_MAX ? UINT16_MAX : (uint16_t)combined;
self->scale = scale;
_update_child_transforms(self);
}
Expand Down
2 changes: 1 addition & 1 deletion shared-module/displayio/Palette.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ uint32_t common_hal_displayio_palette_get_color(displayio_palette_t *self, uint3

void displayio_palette_get_color(displayio_palette_t *self, const _displayio_colorspace_t *colorspace, const displayio_input_pixel_t *input_pixel, displayio_output_pixel_t *output_color) {
uint32_t palette_index = input_pixel->pixel;
if (palette_index > self->color_count || self->colors[palette_index].transparent) {
if (palette_index >= self->color_count || self->colors[palette_index].transparent) {
output_color->opaque = false;
return;
}
Expand Down
4 changes: 3 additions & 1 deletion shared-module/displayio/TileGrid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ void common_hal_displayio_tilegrid_construct(displayio_tilegrid_t *self, mp_obj_
mp_obj_t pixel_shader, uint16_t width, uint16_t height,
uint16_t tile_width, uint16_t tile_height, uint16_t x, uint16_t y, uint16_t default_tile) {

uint32_t total_tiles = width * height;
uint32_t total_tiles = (uint32_t)width * (uint32_t)height;
mp_arg_validate_length_max(total_tiles, SIZE_MAX / sizeof(uint16_t),
MP_QSTR_TileGrid);
self->bitmap_width_in_tiles = bitmap_width_in_tiles;
self->tiles_in_bitmap = bitmap_width_in_tiles * bitmap_height_in_tiles;

Expand Down
6 changes: 3 additions & 3 deletions shared-module/displayio/area.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ struct _displayio_area_t {
typedef struct {
uint16_t x;
uint16_t y;
int8_t dx;
int8_t dy;
uint8_t scale;
int16_t dx;
int16_t dy;
uint16_t scale;

@FoamyGuy FoamyGuy Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

uint16_t width;
uint16_t height;
bool mirror_x;
Expand Down
4 changes: 4 additions & 0 deletions shared-module/tilepalettemapper/TilePaletteMapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ void tilepalettemapper_tilepalettemapper_get_color(tilepalettemapper_tilepalette
}
return;
}
if (input_pixel->pixel >= self->input_color_count) {
output_color->opaque = false;
return;
}
uint16_t tile_index = y_tile_index * self->width_in_tiles + x_tile_index;
uint32_t mapped_index = self->tile_mappings[tile_index][input_pixel->pixel];
displayio_input_pixel_t tmp_pixel;
Expand Down
3 changes: 2 additions & 1 deletion shared-module/vectorio/Polygon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ static void _clobber_points_list(vectorio_polygon_t *self, mp_obj_t points_tuple
if (len < 3) {
mp_raise_TypeError(MP_ERROR_TEXT("Polygon needs at least 3 points"));
}
mp_arg_validate_length_range(len, 3, UINT16_MAX / 2, MP_QSTR_points);

int16_t *points_list = gc_realloc(self->points_list, 2 * len * sizeof(uint16_t), true);
VECTORIO_POLYGON_DEBUG("realloc(%p, %d) -> %p", self->points_list, 2 * len * sizeof(uint16_t), points_list);
Expand All @@ -39,7 +40,7 @@ static void _clobber_points_list(vectorio_polygon_t *self, mp_obj_t points_tuple
self->points_list = NULL;
self->len = 0;

for (uint16_t i = 0; i < len; ++i) {
for (size_t i = 0; i < len; ++i) {
size_t tuple_len = 0;
mp_obj_t *tuple_items;
mp_arg_validate_type(items[i], &mp_type_tuple, MP_QSTR_point);
Expand Down
Loading