From 812d846e4784888e0218432a941939980fe9561d Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 13:30:35 -0400 Subject: [PATCH] Take libakgl 0.7.0, and refuse an over-long type or field name 0.6.0 and 0.7.0 break nothing here: 0.6.0 is three arcade-physics fixes and a physics.max_timestep property this port does not use, and 0.7.0 reports failures libakstdlib's wrappers were already catching and takes libakerror 2.0.1 so it can drop the exit-status trap its own suites needed. That is the same defect include/akbasic/error.h guards for this band, now fixed at the source rather than worked around in two places. The floor moves to 0.7.0 anyway. The soname carries MAJOR.MINOR while the major is 0, so deciding for ourselves which of libakgl's minor releases were really compatible is exactly the judgement it exists to take away. 0.7.0 is largely about a defect class worth checking for here rather than assuming past: ten unterminated strncpy calls into fixed-width name fields. Ours are clean -- every one is bounded to size - 1 and every buffer is either explicitly terminated afterwards or memset first, checked site by site. But two of them, both written last week in src/structtype.c, truncated an over-long name silently, and truncation is an error everywhere else in this interpreter. It matters more for a name than for a value: two long type names trimmed to the same 31 characters collide, and the second is then reported as redeclaring a type the program never wrote. Both are refused now, showing the name as written. The check was unreachable as first drafted, because the prescan read words into a buffer the size of the limit and so trimmed them before anything could look -- the scratch is twice the limit now, which is what makes the two cases distinguishable at all. tests/struct_types.c pins that it stays reachable. Co-Authored-By: Claude Opus 5 (1M context) --- MAINTENANCE.md | 2 +- README.md | 2 +- TODO.md | 2 +- deps/libakgl | 2 +- include/akbasic/akgl.h | 24 ++++++++++++++--------- src/structtype.c | 32 +++++++++++++++++++++++++++---- tests/struct_types.c | 43 ++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 90 insertions(+), 17 deletions(-) diff --git a/MAINTENANCE.md b/MAINTENANCE.md index ab959d5..ec69c36 100644 --- a/MAINTENANCE.md +++ b/MAINTENANCE.md @@ -166,7 +166,7 @@ a mixed build leaks pool slots or frees one twice rather than failing to link. |---|---|---|---|---| | `deps/libakerror` | 2.0.1 | `libakerror.so.2` | major only | **none** — no version macro; `include/akbasic/error.h` feature-tests `AKERR_THREAD_SAFE` and `AKERR_EXIT_STATUS_UNREPRESENTABLE` instead | | `deps/libakstdlib` | 0.2.0 | `libakstdlib.so.0.2` | **`MAJOR.MINOR` while major is 0** | `AKSL_VERSION_*`, `aksl_version()`, `AKSL_VERSION_CHECK()` | -| `deps/libakgl` | 0.5.0 | `libakgl.so.0.5` | **`MAJOR.MINOR` while major is 0** | `AKGL_VERSION*`, `akgl_version()`, `AKGL_VERSION_AT_LEAST()` | +| `deps/libakgl` | 0.7.0 | `libakgl.so.0.7` | **`MAJOR.MINOR` while major is 0** | `AKGL_VERSION*`, `akgl_version()`, `AKGL_VERSION_AT_LEAST()` | For both 0.x libraries the soname carries `MAJOR.MINOR` deliberately: 0.1 and 0.2 are *different* ABIs, and both become major-only at 1.0. Do not read `0.1 → 0.2` as a compatible diff --git a/README.md b/README.md index b0f9944..793d636 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,7 @@ nothing to install first. * [libakstdlib](https://source.starfort.tech/andrew/libakstdlib) 0.2.0 — libc wrappers that report through `libakerror`. String-to-number conversion goes straight to it, which is why `VAL("garbage")` is an error rather than a silent `0`. -* [libakgl](https://source.starfort.tech/andrew/libakgl) 0.5.0 — **optional**, only for +* [libakgl](https://source.starfort.tech/andrew/libakgl) 0.7.0 — **optional**, only for `-DAKBASIC_WITH_AKGL=ON`. Pulls in SDL3. Its soname carries `MAJOR.MINOR` while the major is 0, so rebuild rather than relink. * [basicinterpret](https://source.starfort.tech/andrew/basicinterpret) — the Go original. diff --git a/TODO.md b/TODO.md index 60a37d6..d2d48a7 100644 --- a/TODO.md +++ b/TODO.md @@ -1911,7 +1911,7 @@ Dependency baseline: |---|---|---| | `deps/libakerror` | 2.0.1 | Private ownership-enforced status registry. akbasic reserves 512–767 in `akbasic_error_register()`. Does not namespace its `coverage` target when embedded — worked around in our `CMakeLists.txt`. **2.0.0 is thread safe and an ABI break** (`libakerror.so.2`): `__akerr_last_ignored` is thread-local and `akerr_next_error()` returns an owned reference, neither of which fails to link when mismatched. **2.0.1 fixes an exit status that mattered more to this band than to any other** — see below. | | `deps/libakstdlib` | 0.2.0 | soname `libakstdlib.so.0.2`. `AKSL_VERSION_CHECK()` asserted in `tests/version_check.c`. This release fixed all six confirmed defects the port was working around — see §1.9, where the bans are now lifted. | -| `deps/libakgl` | 0.5.0 | soname `libakgl.so.0.5`. Owns status codes 256–260. Linked and tested under `-DAKBASIC_WITH_AKGL=ON`, which still defaults OFF so the core library and its whole suite build on a machine with no SDL. **0.3.0 closed every API gap this port had filed** — see §7 — so the four workarounds §3 used to list are gone. 0.4.0 was a leak-and-overread release that changed no public struct. **0.5.0 is the first that broke our source as well as our ABI**: it namespaced every exported symbol, so `akgl_render_bind2d` is `akgl_render_2d_bind`, `akgl_sprite_sheet_coords_for_frame` is `akgl_spritesheet_coords_for_frame`, the `renderer`/`camera`/`window` globals carry the prefix, and `_akgl_renderer`/`_akgl_camera` are `akgl_default_renderer`/`akgl_default_camera`. `include/akbasic/akgl.h` asserts the 0.5.0 floor. | +| `deps/libakgl` | 0.7.0 | soname `libakgl.so.0.7`. Owns status codes 256–260. Linked and tested under `-DAKBASIC_WITH_AKGL=ON`, which still defaults OFF so the core library and its whole suite build on a machine with no SDL. **0.3.0 closed every API gap this port had filed** — see §7 — so the four workarounds §3 used to list are gone. 0.4.0 was a leak-and-overread release that changed no public struct. **0.5.0 is the first that broke our source as well as our ABI**: it namespaced every exported symbol, so `akgl_render_bind2d` is `akgl_render_2d_bind`, `akgl_sprite_sheet_coords_for_frame` is `akgl_spritesheet_coords_for_frame`, the `renderer`/`camera`/`window` globals carry the prefix, and `_akgl_renderer`/`_akgl_camera` are `akgl_default_renderer`/`akgl_default_camera`. `include/akbasic/akgl.h` asserts the floor. **0.6.0 and 0.7.0 broke nothing here** — 0.6.0 is three arcade-physics fixes and a `physics.max_timestep` property this port does not use, and 0.7.0 reports failures `libakstdlib`'s wrappers were already catching and takes `libakerror` 2.0.1. The floor moved anyway, because deciding for ourselves which of libakgl's minor releases were really compatible is the judgement the soname exists to take away. | **An unhandled error in this band used to exit zero, and 512 is the worst possible base for that.** `libakerror`'s default unhandled-error handler ended in `exit(errctx->status)`, and a diff --git a/deps/libakgl b/deps/libakgl index b899f09..e4aa6a5 160000 --- a/deps/libakgl +++ b/deps/libakgl @@ -1 +1 @@ -Subproject commit b899f09fc8b54802c3f64ba1473c657bd3feaaeb +Subproject commit e4aa6a50847a12c22544acf0b3b2953f5128c6af diff --git a/include/akbasic/akgl.h b/include/akbasic/akgl.h index 6a56963..9011db9 100644 --- a/include/akbasic/akgl.h +++ b/include/akbasic/akgl.h @@ -36,10 +36,9 @@ #include /* - * **0.5.0 is a hard floor, API as well as ABI**, which is new: every release - * before it moved this line for ABI reasons alone. 0.5.0 namespaced every - * exported symbol, so the spellings this target compiles against do not exist in - * 0.4.0 and the ones 0.4.0 had do not exist here. `akgl_render_bind2d` is + * **0.5.0 was a hard floor, API as well as ABI**, and it is worth keeping the + * reason: it namespaced every exported symbol, so the spellings this target + * compiles against did not exist before it. `akgl_render_bind2d` is * `akgl_render_2d_bind`, `akgl_sprite_sheet_coords_for_frame` is * `akgl_spritesheet_coords_for_frame`, and the `renderer`, `camera` and `window` * globals carry the prefix -- with `_akgl_renderer` and `_akgl_camera` now @@ -50,20 +49,27 @@ * same name, the executable's definition preempted the library's, and every * texture load in that suite failed while the suite reported success. * - * The soname carries MAJOR.MINOR while the major is 0, so 0.4 and 0.5 are + * **0.6.0 and 0.7.0 broke nothing here**, and the floor moves anyway. 0.6.0 is + * three arcade-physics fixes and a `physics.max_timestep` property this target + * does not use; 0.7.0 reports failures libakstdlib's wrappers were already + * catching, and takes libakerror 2.0.1 so it can drop the exit-status trap its + * own suites needed -- the same defect `include/akbasic/error.h` guards for this + * band, fixed at the source rather than worked around twice. + * + * The soname carries MAJOR.MINOR while the major is 0, so 0.5, 0.6 and 0.7 are * different ABIs *by declaration* -- libakgl's versioning policy says a 0.x minor * bump may break, and the soname is built to match. The floor moves with every * one of them, because the alternative is deciding for ourselves which of * libakgl's minor releases were really compatible, and that is exactly the * judgement the soname exists to take away from us. * - * What it catches is the case the soname cannot: a build against 0.4 headers - * that happens to find a 0.5 library, or the reverse. Refused here rather than + * What it catches is the case the soname cannot: a build against 0.6 headers + * that happens to find a 0.7 library, or the reverse. Refused here rather than * at link time, because a missing symbol names a function and this names the * release. */ -#if !AKGL_VERSION_AT_LEAST(0, 5, 0) -#error "akbasic's libakgl adaptors require libakgl 0.5.0 or later" +#if !AKGL_VERSION_AT_LEAST(0, 7, 0) +#error "akbasic's libakgl adaptors require libakgl 0.7.0 or later" #endif #include diff --git a/src/structtype.c b/src/structtype.c index b47371c..e27974e 100644 --- a/src/structtype.c +++ b/src/structtype.c @@ -58,6 +58,18 @@ static const char *skip_lineno(const char *cursor) return skip_space(cursor); } +/* + * Room to read a word that is *too long* and still see that it was. + * + * Reading into a buffer the size of the limit truncates before anything can + * check, so an over-long name would arrive already trimmed to fit and be + * accepted -- which is how the length check below was unreachable when it was + * first written. Twice the limit is enough to tell the two cases apart, and a + * word longer than this is still longer than the limit, so it is refused either + * way. + */ +#define STRUCT_WORD_SCRATCH (AKBASIC_MAX_STRUCT_NAME * 2) + /** @brief Copy the next whitespace-delimited word, uppercased. Empty when the line ends. */ static const char *next_word(const char *cursor, char *dest, size_t len) { @@ -205,8 +217,8 @@ static akerr_ErrorContext *scan_names(akbasic_Runtime *obj) { PREPARE_ERROR(errctx); akbasic_StructTypeTable *table = &obj->structtypes; - char word[AKBASIC_MAX_STRUCT_NAME]; - char name[AKBASIC_MAX_STRUCT_NAME]; + char word[STRUCT_WORD_SCRATCH]; + char name[STRUCT_WORD_SCRATCH]; const char *cursor = NULL; int64_t i = 0; int open = -1; @@ -256,8 +268,15 @@ static akerr_ErrorContext *scan_names(akbasic_Runtime *obj) FAIL_ZERO_RETURN(errctx, (table->count < AKBASIC_MAX_STRUCT_TYPES), AKBASIC_ERR_BOUNDS, "More than %d TYPE declarations", AKBASIC_MAX_STRUCT_TYPES); + FAIL_ZERO_RETURN(errctx, (strlen(name) < AKBASIC_MAX_STRUCT_NAME), AKBASIC_ERR_BOUNDS, + "TYPE name \"%s\" exceeds the %d character limit", + name, AKBASIC_MAX_STRUCT_NAME - 1); open = table->count; memset(&table->types[open], 0, sizeof(table->types[open])); + /* The memset is what terminates this: bounded at size - 1, the last byte + is the zero it already wrote. Refused above rather than truncated, + because two long names truncating to the same prefix would collide + silently -- and truncation is an error everywhere else here. */ strncpy(table->types[open].name, name, sizeof(table->types[open].name) - 1); table->types[open].used = true; table->types[open].firstline = i; @@ -278,8 +297,8 @@ static akerr_ErrorContext *parse_field(akbasic_StructTypeTable *table, akbasic_S const char *line, int64_t lineno) { PREPARE_ERROR(errctx); - char fieldname[AKBASIC_MAX_STRUCT_NAME]; - char word[AKBASIC_MAX_STRUCT_NAME]; + char fieldname[STRUCT_WORD_SCRATCH]; + char word[STRUCT_WORD_SCRATCH]; const char *cursor = NULL; akbasic_StructField *field = NULL; akbasic_Type valuetype = AKBASIC_TYPE_UNDEFINED; @@ -303,8 +322,13 @@ static akerr_ErrorContext *parse_field(akbasic_StructTypeTable *table, akbasic_S } PASS(errctx, type_from_suffix(fieldname, &valuetype)); + FAIL_ZERO_RETURN(errctx, (strlen(fieldname) < AKBASIC_MAX_STRUCT_NAME), AKBASIC_ERR_BOUNDS, + "Field name \"%s\" exceeds the %d character limit", + fieldname, AKBASIC_MAX_STRUCT_NAME - 1); field = &type->fields[type->fieldcount]; memset(field, 0, sizeof(*field)); + /* Terminated by the memset above; refused rather than truncated, for the + same reason a type name is. */ strncpy(field->name, fieldname, sizeof(field->name) - 1); field->valuetype = valuetype; field->typeindex = -1; diff --git a/tests/struct_types.c b/tests/struct_types.c index 0a8add4..16c5243 100644 --- a/tests/struct_types.c +++ b/tests/struct_types.c @@ -218,6 +218,48 @@ static void test_declaration_errors_are_basic_errors(void) } } +/** + * @brief An over-long type or field name is refused, not silently trimmed. + * + * Truncation is an error everywhere else here, and it matters more for a name + * than for a value: two long names trimmed to the same 31 characters would + * collide, and the second would be reported as a redeclaration of a type the + * program never wrote. + * + * The check was unreachable when it was first written, because the prescan read + * words into a buffer the size of the limit and so trimmed them before anything + * could look. Pinned here so it stays reachable. + */ +static void test_long_names_are_refused(void) +{ + TEST_REQUIRE_OK(harness_start(NULL)); + TEST_REQUIRE_OK(akbasic_runtime_load(&HARNESS_RUNTIME, + "10 TYPE AVERYVERYVERYLONGTYPENAMETHATGOESONANDON\n" + "20 W#\n" + "30 END TYPE\n" + "40 PRINT 1\n")); + TEST_REQUIRE_OK(akbasic_runtime_start(&HARNESS_RUNTIME, AKBASIC_MODE_RUN)); + TEST_REQUIRE_OK(akbasic_runtime_run(&HARNESS_RUNTIME, 0)); + TEST_REQUIRE(strstr(HARNESS_OUTPUT, "exceeds the 31 character limit") != NULL, + "an over-long type name should be refused, got \"%s\"", HARNESS_OUTPUT); + /* The whole name, not the trimmed one -- that is what makes it actionable. */ + TEST_REQUIRE(strstr(HARNESS_OUTPUT, "GOESONANDON") != NULL, + "the refusal should show the name as written, got \"%s\"", HARNESS_OUTPUT); + harness_stop(); + + TEST_REQUIRE_OK(harness_start(NULL)); + TEST_REQUIRE_OK(akbasic_runtime_load(&HARNESS_RUNTIME, + "10 TYPE OKNAME\n" + "20 AVERYVERYVERYLONGFIELDNAMETHATGOESON#\n" + "30 END TYPE\n" + "40 PRINT 1\n")); + TEST_REQUIRE_OK(akbasic_runtime_start(&HARNESS_RUNTIME, AKBASIC_MODE_RUN)); + TEST_REQUIRE_OK(akbasic_runtime_run(&HARNESS_RUNTIME, 0)); + TEST_REQUIRE(strstr(HARNESS_OUTPUT, "exceeds the 31 character limit") != NULL, + "an over-long field name should be refused, got \"%s\"", HARNESS_OUTPUT); + harness_stop(); +} + int main(void) { TEST_REQUIRE_OK(akbasic_error_register()); @@ -228,6 +270,7 @@ int main(void) test_missing_field_lists_the_others(); test_self_by_value_refused(); test_declaration_errors_are_basic_errors(); + test_long_names_are_refused(); return akbasic_test_failures; }