diff --git a/Makefile b/Makefile index ff2c0db8..6e65f219 100644 --- a/Makefile +++ b/Makefile @@ -104,7 +104,7 @@ amy-message: $(OBJECTS) src/amy-message.o # Plain C tests for things the audio-rendering suite can't reach -- e.g. clock # rollovers 50 days out, which you can only hit by fast-forwarding the counters. -CTESTS = tests/test_clock_wrap +CTESTS = tests/test_clock_wrap tests/test_sequencer_bounds # Static pattern rules, so these win over the generic %.o: %.c above (which # would compile without -Isrc and fail to find amy.h). diff --git a/src/sequencer.c b/src/sequencer.c index 46a20a09..80bb98c5 100644 --- a/src/sequencer.c +++ b/src/sequencer.c @@ -182,7 +182,15 @@ uint8_t sequencer_add_event(amy_event *e) { // if the tag already exists - if there's tick/period, overwrite, if there's no tick / period, we should remove the entry //fprintf(stderr, "sequencer_add_event: e->instrument %d e->note %.0f e->vel %.2f tick %d period %d tag %d\n", e->instrument, e->midi_note, e->velocity, e->sequence[SEQUENCE_TICK], e->sequence[SEQUENCE_PERIOD], e->sequence[SEQUENCE_TAG]); int32_t tag = e->sequence[SEQUENCE_TAG]; - if (tag > max_sequences) { + // `sequences` is max_sequences entries, so the last valid tag is + // max_sequences - 1. This read `>` and let tag == max_sequences + // through to write one sequence_info_t past the end of the + // allocation -- the message the check prints has always said "greater + // than or eq", which is what it was meant to be doing. A negative tag + // is rejected here too: SEQUENCE_TAG arrives as an unsigned field but + // lands in an int32_t, so a value past INT32_MAX reads as negative and + // would index backwards out of the array. + if (tag < 0 || tag >= max_sequences) { fprintf(stderr, "sequencer tag %" PRIi32" (with tick %" PRIu32", period %" PRIu32") is greater than or eq max_sequences %" PRIi32"\n", tag, e->sequence[SEQUENCE_TICK], e->sequence[SEQUENCE_PERIOD], max_sequences); // ignore diff --git a/tests/test_sequencer_bounds.c b/tests/test_sequencer_bounds.c new file mode 100644 index 00000000..f8146901 --- /dev/null +++ b/tests/test_sequencer_bounds.c @@ -0,0 +1,102 @@ +// Regression test for the sequencer tag bounds check. +// +// `sequences` is a malloc'd array of max_sequences entries, so the last +// valid tag is max_sequences - 1. sequencer_add_event() guarded with +// +// if (tag > max_sequences) +// +// which let tag == max_sequences through and wrote a whole +// sequence_info_t (a pointer and two uint32_t) one element past the end +// of the allocation. The message it prints on rejection has always said +// "greater than or eq", so the intent was never in doubt. +// +// Not reachable from the audio-rendering suite, which never sends a tag +// near the ceiling -- and not reachable from a well-behaved host either, +// since an allocator that hands out tags below the limit can't produce +// one. It takes exactly one hand-written message: +// +// amy.send(sequence="0,16,256") # with max_sequencer_tags 256 +// +// Build/run with `make ctest`. + +#include +#include +#include +#include "amy.h" +#include "sequencer.h" + +static int failures = 0; + +#define CHECK(cond, fmt, ...) do { \ + if (cond) { printf(" ok " fmt "\n", ##__VA_ARGS__); } \ + else { printf(" FAIL " fmt "\n", ##__VA_ARGS__); failures++; } \ +} while (0) + +#define MAX_TAGS 64 + +// A schedulable event: a period is what makes sequencer_add_event keep it +// rather than returning 0 for "nothing to schedule here". +static uint8_t add_at_tag(int32_t tag) { + amy_event e = amy_default_event(); + e.osc = 0; + e.wave = SINE; + e.velocity = 1.0f; + e.midi_note = 60; + e.sequence[SEQUENCE_TICK] = 0; + e.sequence[SEQUENCE_PERIOD] = 16; + e.sequence[SEQUENCE_TAG] = (uint32_t)tag; + return sequencer_add_event(&e); +} + +static void test_tag_bounds(void) { + printf("sequencer tag bounds (max_sequences = %d)\n", MAX_TAGS); + + CHECK(add_at_tag(0) == 1, "tag 0 is accepted"); + CHECK(add_at_tag(MAX_TAGS - 1) == 1, + "tag max-1 (%d) is accepted -- the last valid slot", MAX_TAGS - 1); + + // The bug: this one used to be written, one element past the end. + CHECK(add_at_tag(MAX_TAGS) == 0, + "tag max (%d) is REJECTED, not written past the end", MAX_TAGS); + CHECK(add_at_tag(MAX_TAGS + 1) == 0, "tag max+1 is rejected"); + CHECK(add_at_tag(1000000) == 0, "a far-out tag is rejected"); + + // SEQUENCE_TAG is an unsigned field read into an int32_t, so anything + // past INT32_MAX arrives negative and would index backwards. + CHECK(add_at_tag((int32_t)0x80000000u) == 0, + "a tag that reads negative is rejected"); +} + +// ...and it still refuses with every legitimate slot occupied, which is +// the state a host that fills its allocation is actually in. +static void test_full_table(void) { + printf("a full table still refuses the one past the end\n"); + + sequencer_reset(); + int filled = 0; + for (int32_t tag = 0; tag < MAX_TAGS; ++tag) + filled += add_at_tag(tag); + CHECK(filled == MAX_TAGS, "every one of the %d valid tags took an event", + MAX_TAGS); + CHECK(add_at_tag(MAX_TAGS) == 0, "tag %d is still rejected", MAX_TAGS); +} + +// examples.c calls this; the platform normally provides it. +void delay_ms(uint32_t ms) { (void)ms; } + +int main(void) { + amy_config_t c = amy_default_config(); + c.features.startup_bleep = 0; + c.max_sequencer_tags = MAX_TAGS; + amy_start(c); + + test_tag_bounds(); + test_full_table(); + + if (failures) { + printf("\n%d check(s) FAILED\n", failures); + return 1; + } + printf("\nall sequencer bounds checks passed\n"); + return 0; +}