Validate display list items - #19
Conversation
bettio
left a comment
There was a problem hiding this comment.
So just a few comments about this PR.
- Would you mind to point Claude at https://github.com/atomvm/AtomVM/blob/main/C_CODING_STYLE.md ?
So we can make the code consistent with the rest of AtomVM, especially when declaring functions and so on. Concretely, in the new code:
- Parameter order is target, inputs, outputs, options, environment (AVMCCS-A001).
get_bgcolor_element()andget_rgba8888_image()takeContext *ctxbefore their output parameter, andlog_invalid_item()has the context in the middle of its inputs. - The empty line before
returnin bodies longer than three lines (AVMCCS-F011) is applied in some helpers and missing in others, for instanceget_rgba8888_image(),get_clamped_span()andinit_item(). - The new public function and the two public macros in
display_items.hneed at least a@brief(AVMCCS-D001, AVMCCS-D021). - Optional: the guide prefers a result enum over returning the status as a string (AVMCCS-A003). Fine to keep the strings for file-local helpers if you prefer the brevity.
- I think there might be problems with the
atom_table_ensure_atomstub. In general I suggest checking that all stubs match the release-0.7 AtomVM branch. We can discard compatibility with release-0.6: we are phasing it out and it doesn't make sense anymore to support it.
It is the only stub that mismatches. 0.7 declares it as:
enum AtomTableEnsureAtomResult atom_table_ensure_atom(struct AtomTable *table,
const uint8_t *atom_data, size_t atom_len, enum AtomTableCopyOpt opts, atom_index_t *result);while the stub has the 0.6 form, so tests/items fails to compile against 0.7 headers. The same file uses TERM_BOXED_VALUE_TAG, deprecated in 0.7 in favour of TERM_PRIMARY_BOXED. With those two changes all 388 checks pass against 0.7 under ASan and UBSan.
Dropping 0.6 has two consequences worth doing together:
.github/workflows/esp32-build.yamlstill checks out AtomVMrelease-0.6, so CI keeps building against the line being phased out and would reject 0.7-only APIs.- Once 0.6 is out,
get_int_element()can useterm_is_int64()andterm_to_int64()fromterm.hinstead of the hand-written boxed-size check, which is exactly the body ofterm_is_int64().
Optional: the test builds terms by hand (int_term, boxed_int, tuple); term_from_int(), term_alloc_tuple() and term_make_boxed_int64() would remove that coupling to the term encoding.
- Make sure to use
LIKELY(),UNLIKELY()andIS_NULL_PTR()macros from AtomVM for dealing with branch optimization. You should useUNLIKELY()this way:
if (UNLIKELY(is_error)) {
// error handling
}In the parser every validation failure is such a branch: the arity checks, the !get_*_element() chains, the image tuple and binary checks, the source-offset check, !ok || text == NULL, !loaded_font, res != EPD_DRAW_SUCCESS, and the reason != NULL branches in init_item() and display_items_init_list(). !surface.buffer and handle != NULL are IS_NULL_PTR() cases. The clamping conditions are normal flow and should stay as they are.
-
An empty string yields a zero-size rect and a zero-byte allocation, which returns NULL on ESP-IDF, so the item is logged as "out of memory" on every update. Main already logged a failure there every update, so it is not a regression, but the reason is misleading. Suggest treating a non-positive rect as an empty item with no log: a check on
rect.width <= 0 || rect.height <= 0right afterepd_get_string_rect(), freeing the text and returning success with the zeroed item. It also removes amalloc(0), which the style guide asks to avoid. -
The parser file now includes the default atoms header (
defaultatoms.h) without using it. -
Let's fix an old existing issue: an empty display list on the ESP32 drivers allocates zero bytes, gets NULL and skips the update with an error line.
The five do_update() functions share the same lines and this PR already touches all of them for the display_items_init_list() switch. Minimal fix: allocate only when len > 0 and keep items NULL otherwise; display_items_delete() already tolerates that. Better fix, if you are willing: a display_items_new_list() that does the length, the allocation and the parsing in one place, which is also where a non-list or an improper list should be rejected, since the drivers ignore the proper flag today.
release-0.6 is being phased out. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Tom Hoenderdos <tomhoenderdos@gmail.com>
b0bfe1e to
50cd685
Compare
The parser trusted every display list item: it read tuple elements without checking the arity, converted terms with term_to_int() without checking that they were integers, and used image binaries without checking their size. A malformed item from Erlang code could read outside a tuple or past the end of an image binary, and a scaled_cropped_image with a zero scale divided by zero in the renderers. Items are now checked before they are drawn. An item with a wrong arity, a value of the wrong type, an out of range value or an unknown command becomes an invalid item that draws nothing, and the rest of the display list is drawn as usual. In detail: - image and scaled_cropped_image need an rgba8888 image tuple with a width and height from 1 to 32767 and a binary holding at least Width * Height * 4 bytes. Their coordinates, sizes, source offsets and scales must be within +-32767, the scales at least 1 and the source offset inside the image. The drawn width and height of a scaled_cropped_image are reduced to what is left of the image right of and below the source offset. - rect and text accept any integer position and size, which is clamped to +-32767 without changing which pixels are drawn, so existing lists that draw oversized rects keep working. - A text font must be an atom. - Integers may be boxed, so values above 2^27 are read the same way on 32-bit and 64-bit builds. Ranges are checked on 64 bits before narrowing to int, so out of range values can't wrap into range. Colors use the low 24 bits of any integer. Each invalid item is logged to stderr on one line with its position, command and arity, at most three per update, so that a list rebuilt on every frame doesn't flood a slow serial console. The drivers now get their items from the new display_items_new_list(), which counts, allocates and parses the list in one place. An empty display list no longer allocates zero bytes, which returned NULL on ESP-IDF and skipped the update with an error, and a display list that is not a proper list is rejected instead of being parsed up to its tail. Text in a ufont font that renders to an empty rect becomes an empty item instead of failing a zero-byte allocation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Tom Hoenderdos <tomhoenderdos@gmail.com>
Add tests/items, a host test that builds display_items.c and the DCS
LCD renderer against AtomVM's headers, with stubs for the few
libAtomVM functions they call and for the ESP-IDF SPI header, so the
parser can be exercised without a device.
The test builds display list items from real AtomVM terms and checks
that malformed items become invalid items with every field zeroed,
that valid items carry the expected fields, that rect and text
positions are clamped as documented, and that invalid items are
logged in the documented format. It then renders images and
scaled_cropped_images under occluding rects and compares every pixel
with a reference computed from the source bytes.
It runs under ASan and UBSan by default, with every image binary in
an exactly sized allocation, so any read past an image aborts the
test. Allocations made by the parser are counted and must all be
released by display_items_delete().
Build and run with:
cmake -S tests/items -B build/items \
-DLIBATOMVM_INCLUDE_PATH=<AtomVM>/src/libAtomVM
cmake --build build/items && build/items/test_items
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Tom Hoenderdos <tomhoenderdos@gmail.com>
9d9f79d to
2be76b1
Compare
Based on comments on #18
Summary:
The display list parser trusted every item: it read tuple elements without checking the arity, converted terms with
term_to_int()without checking they were integers, and used image binaries without checking their size. A malformed item could read outside a tuple or past the end of an image binary, and ascaled_cropped_imagewith a zero scale divided by zero in the renderers.Items are now checked before they are drawn. An item with a wrong arity, wrong type, out of range value or unknown command becomes an invalid item that draws nothing; the rest of the list draws as usual. Invalid items are logged to stderr (at most three per update). Validation rules are documented in
docs/primitives.md.The second commit adds
tests/items, a host test that buildsdisplay_items.cand the DCS LCD renderer against AtomVM headers with stubs, and runs under ASan/UBSan:🤖 Generated with Claude Code