From 8d0f311c22d3b53cc64e50081916238907752b0a Mon Sep 17 00:00:00 2001 From: Enver Haase Date: Thu, 3 Sep 2026 20:54:47 +0200 Subject: [PATCH 1/7] Check what a G64 says about its tracks before believing it GcrImage::load() took each track's length straight from the image, masked it to 14 bits and stored it. That mask admits 16383, while GCRIMAGE_MAXTRACKLEN is 7928, and nothing compared the length against the space left in the buffer behind the track's own offset. insert_disk() then programs that length into the drive engine's parameter RAM, and floppy_mem.vhd both reads and writes memory bounded by it, so a crafted image could point the hardware about 8 KB past the end of the allocation. Three smaller holes on the same path: the offset test admitted offset == GCRIMAGE_MAXSIZE, so tr[0] and tr[1] were read one and two bytes past the buffer; the offset was never compared against how much the file actually delivered, so a length could be taken from a part of the buffer no read had filled; and a track declared zero bytes long reached insert_disk()'s division by track_length. A track whose metadata does not survive these checks is now left as invalidate() left it -- absent -- so add_blank_tracks() gives it a blank track and the mount goes on. add_blank_tracks() in the same file already bound-checks this way, so the missing check in load() was an omission rather than a choice. The checks live in a new header of free functions over plain integers rather than in load() itself, because none of the drive headers can be compiled on a build host and this is the part worth testing there. Reported in GideonZ/1541ultimate#823, problems 2 and 4. --- software/drive/disk_image.cc | 36 +++--- software/drive/gcr_track_bounds.h | 60 ++++++++++ software/drive/tests/Makefile | 15 +++ .../drive/tests/gcr_track_bounds_test.cpp | 108 ++++++++++++++++++ 4 files changed, 206 insertions(+), 13 deletions(-) create mode 100644 software/drive/gcr_track_bounds.h create mode 100644 software/drive/tests/Makefile create mode 100644 software/drive/tests/gcr_track_bounds_test.cpp diff --git a/software/drive/disk_image.cc b/software/drive/disk_image.cc index 05fad24c8..f20aeccef 100644 --- a/software/drive/disk_image.cc +++ b/software/drive/disk_image.cc @@ -20,6 +20,7 @@ extern "C" { #include "user_file_interaction.h" #include "blockdev_file.h" #include "endianness.h" +#include "gcr_track_bounds.h" #define HARDWARE_ENCODING 1 @@ -709,23 +710,32 @@ bool GcrImage :: load(File *f) // track offsets start at 0x000c for(int i=0;i GCRIMAGE_MAXSIZE) { + if(offset >= GCRIMAGE_MAXSIZE) { printf("Error. Track pointer outside GCR memory range.\n"); return false; } + // Offset zero means the track is absent. A non-zero one still has to + // point at two bytes the file actually delivered, since reading the + // length word is itself an access into the image buffer. + if(!gcr_track_header_is_readable(offset, bytes_read)) { + continue; + } tr = gcr_data + offset; - if(offset) { - w = tr[0] | (uint16_t(tr[1]) << 8); - tracks[i].track_address = tr + 2; - tracks[i].track_length = (int)(w & 0x3FFF); - tracks[i].track_used = true; - tracks[i].in_image_file = true; - tracks[i].track_is_mfm = (w & 0x8000); -// printf("Set track %d.%d to 0x%6x / 0x%4x.\n", (i>>1)+1, (i&1)?5:0, tracks[i].track_address, w); - if (i >= GCRIMAGE_FIRSTTRACKSIDE1) { - double_sided = true; - } - } + w = tr[0] | (uint16_t(tr[1]) << 8); + int length = gcr_validated_track_length(w, offset, GCRIMAGE_MAXSIZE, GCRIMAGE_MAXTRACKLEN); + if(!length) { + printf("Track %d: declared length %d does not fit the image. Track skipped.\n", + i, (int)(w & 0x3FFF)); + continue; // invalidate() left this entry unused; leave it that way + } + tracks[i].track_address = tr + 2; + tracks[i].track_length = length; + tracks[i].track_used = true; + tracks[i].in_image_file = true; + tracks[i].track_is_mfm = (w & 0x8000); + if (i >= GCRIMAGE_FIRSTTRACKSIDE1) { + double_sided = true; + } } // Track speed zone info starts after the track offsets uint8_t reported_tracks = gcr_data[9]; diff --git a/software/drive/gcr_track_bounds.h b/software/drive/gcr_track_bounds.h new file mode 100644 index 000000000..a48c1ed98 --- /dev/null +++ b/software/drive/gcr_track_bounds.h @@ -0,0 +1,60 @@ +/* + * gcr_track_bounds.h -- what a G64 says about its tracks, checked. + * + * The track lengths in a G64 file are not decoration: insert_disk() programs + * them into the drive engine's parameter RAM, and floppy_mem.vhd both reads + * and writes memory bounded by them. So a length taken from the file decides + * how far the hardware reaches into a buffer, and a track that is not in the + * image at all still has to be given a parameter pair the engine can survive. + * + * These are free functions over plain integers, deliberately: none of the + * drive headers can be compiled on a build host, and this is the part that is + * worth testing there. See software/drive/tests/. + */ +#ifndef DRIVE_GCR_TRACK_BOUNDS_H +#define DRIVE_GCR_TRACK_BOUNDS_H + +#include + +/* A track table entry points at a two-byte length word followed by the track + * data. Reading that word is itself an access into the image buffer, so it has + * to lie inside the part of the buffer the file actually filled. Offset zero + * means the track is absent. */ +static inline bool gcr_track_header_is_readable(uint32_t offset, uint32_t bytes_read) +{ + if (offset == 0 || offset > bytes_read) { + return false; + } + return (bytes_read - offset) >= 2; +} + +/* Returns the track length to use, or 0 when the track cannot be used and has + * to be left marked absent. + * + * declared the 16-bit word from the image, flag bits included + * offset byte offset of that word within the buffer + * capacity size of the buffer + * max_length longest track the format allows + * + * The masked field holds 14 bits, so it admits lengths more than twice the + * longest legitimate track, and a track near the end of the buffer can declare + * a length that runs off it. Both are rejected here rather than programmed. + */ +static inline int gcr_validated_track_length(uint16_t declared, uint32_t offset, + uint32_t capacity, int max_length) +{ + int length = (int)(declared & 0x3FFF); + + if (length <= 0 || length > max_length) { + return 0; /* zero would also divide by zero in insert_disk() */ + } + if (offset > capacity || (capacity - offset) < 2) { + return 0; + } + if ((uint32_t)length > (capacity - offset - 2)) { + return 0; + } + return length; +} + +#endif /* DRIVE_GCR_TRACK_BOUNDS_H */ diff --git a/software/drive/tests/Makefile b/software/drive/tests/Makefile new file mode 100644 index 000000000..e9669a892 --- /dev/null +++ b/software/drive/tests/Makefile @@ -0,0 +1,15 @@ +CXX ?= g++ +CXXFLAGS := -std=c++14 -g -Wall -Wextra -I. -I.. -I../../io/usb/tests +OUTPUT_DIR := output +GCR_TRACK_BOUNDS_TEST := $(OUTPUT_DIR)/gcrTrackBoundsTest + +.PHONY: all gcr-track-bounds clean + +all: gcr-track-bounds + +gcr-track-bounds: + mkdir -p $(OUTPUT_DIR) + $(CXX) $(CXXFLAGS) gcr_track_bounds_test.cpp ../../io/usb/tests/host_test_main.cpp -lpthread -o $(GCR_TRACK_BOUNDS_TEST) && $(GCR_TRACK_BOUNDS_TEST) + +clean: + rm -rf $(OUTPUT_DIR) diff --git a/software/drive/tests/gcr_track_bounds_test.cpp b/software/drive/tests/gcr_track_bounds_test.cpp new file mode 100644 index 000000000..fdcd7c38e --- /dev/null +++ b/software/drive/tests/gcr_track_bounds_test.cpp @@ -0,0 +1,108 @@ +// Regression tests for GideonZ/1541ultimate#823: a G64's track metadata used to +// program the drive engine's memory bounds without ever being checked against +// the buffer that holds the image. +// +// Two of the four problems in that issue are pinned down here; the parameter +// RAM side follows in its own change. +// +// 2. track length never validated against the buffer -> ValidatedLength +// 4. divide by zero on a zero-length track -> ValidatedLength + +#include "../../io/usb/tests/host_test/host_test.h" +#include "../gcr_track_bounds.h" + +namespace { + +// The real constants, so the numbers here are the ones the firmware uses. +const uint32_t kMaxSize = (0x1EF8 * 80) + (12 + (168 * 10)); // GCRIMAGE_MAXSIZE +const int kMaxTrackLen = 0x1EF8; // GCRIMAGE_MAXTRACKLEN + +} // namespace + +// ---------------------------------------------------------------- problem 2 -- + +TEST(ValidatedLength, AcceptsALegitimateTrack) +{ + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 0x2000, kMaxSize, kMaxTrackLen), 0x1E0C); +} + +TEST(ValidatedLength, KeepsTheLongestLegalTrack) +{ + EXPECT_EQ(gcr_validated_track_length(kMaxTrackLen, 0x2000, kMaxSize, kMaxTrackLen), kMaxTrackLen); +} + +TEST(ValidatedLength, IgnoresTheFlagBitsAboveTheLength) +{ + // Bit 15 is the MFM marker; it must not be read as part of the length. + EXPECT_EQ(gcr_validated_track_length(0x8000 | 0x1E0C, 0x2000, kMaxSize, kMaxTrackLen), 0x1E0C); +} + +TEST(ValidatedLength, RejectsALengthAboveTheFormatMaximum) +{ + // 14 bits admit 16383, more than twice the longest real track. Before the + // fix this was programmed as-is and the engine read and wrote past the + // allocation. + EXPECT_EQ(gcr_validated_track_length(0x3FFF, 0x2000, kMaxSize, kMaxTrackLen), 0); +} + +TEST(ValidatedLength, RejectsATrackRunningOffTheEndOfTheBuffer) +{ + // Legal length, but placed so that it does not fit behind its own header. + const uint32_t offset = kMaxSize - 100; + EXPECT_EQ(gcr_validated_track_length(0x1E0C, offset, kMaxSize, kMaxTrackLen), 0); +} + +TEST(ValidatedLength, AcceptsATrackEndingExactlyAtTheBufferEnd) +{ + const uint32_t offset = kMaxSize - 0x1E0C - 2; + EXPECT_EQ(gcr_validated_track_length(0x1E0C, offset, kMaxSize, kMaxTrackLen), 0x1E0C); +} + +TEST(ValidatedLength, RejectsATrackOneByteTooLongForTheBuffer) +{ + const uint32_t offset = kMaxSize - 0x1E0C - 1; + EXPECT_EQ(gcr_validated_track_length(0x1E0C, offset, kMaxSize, kMaxTrackLen), 0); +} + +TEST(ValidatedLength, SurvivesAnOffsetPastTheBuffer) +{ + // The caller rejects these first, but the arithmetic here must not wrap. + EXPECT_EQ(gcr_validated_track_length(0x1E0C, kMaxSize + 1, kMaxSize, kMaxTrackLen), 0); + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 0xFFFFFFFFu, kMaxSize, kMaxTrackLen), 0); + EXPECT_EQ(gcr_validated_track_length(0x1E0C, kMaxSize - 1, kMaxSize, kMaxTrackLen), 0); +} + +// ---------------------------------------------------------------- problem 4 -- + +TEST(ValidatedLength, RejectsAZeroLengthTrack) +{ + // insert_disk() divides the rotation speed by the track length, so a track + // declared zero bytes long used to reach a division by zero. + EXPECT_EQ(gcr_validated_track_length(0x0000, 0x2000, kMaxSize, kMaxTrackLen), 0); + EXPECT_EQ(gcr_validated_track_length(0x8000, 0x2000, kMaxSize, kMaxTrackLen), 0); +} + +// ------------------------------------------------------- the header itself -- + +TEST(HeaderReadable, RejectsTheAbsentTrackMarker) +{ + EXPECT_FALSE(gcr_track_header_is_readable(0, 4096)); +} + +TEST(HeaderReadable, AcceptsAHeaderWhollyInsideWhatWasRead) +{ + EXPECT_TRUE(gcr_track_header_is_readable(4094, 4096)); +} + +TEST(HeaderReadable, RejectsAHeaderStraddlingTheEndOfWhatWasRead) +{ + // One byte of the length word is in the file, the other is stale buffer. + EXPECT_FALSE(gcr_track_header_is_readable(4095, 4096)); + EXPECT_FALSE(gcr_track_header_is_readable(4096, 4096)); +} + +TEST(HeaderReadable, RejectsAPointerPastWhatWasRead) +{ + EXPECT_FALSE(gcr_track_header_is_readable(8192, 4096)); + EXPECT_FALSE(gcr_track_header_is_readable(0xFFFFFFFFu, 4096)); +} From 3475e9eea3dde2d4ebdd730fccbdc20b3e7ad270 Mon Sep 17 00:00:00 2001 From: Enver Haase Date: Thu, 3 Sep 2026 21:05:09 +0200 Subject: [PATCH 2/7] Never program the drive from a track parameter nobody wrote insert_disk() declared bit_time and track_len without initialisers, assigned them only inside the branch taken for a track that is present, and read them in the branch taken for one that is not. Two consequences. The first is reachable with nothing but a malformed file. GcrImage::load() returns false on a bad signature after invalidate() has zeroed the whole track table, but mount_g64() ignored what it returned and inserted the image anyway. Mounting any .g64 whose first eight bytes are not GCR-1541 or GCR-1571 therefore programmed all 84 parameter slots with dummy_track -- 7692 bytes, no slack -- paired with whatever those two stack slots happened to hold. A stale length above that points the drive engine, which reads and writes there, past the end of the heap block. The second is on every image. add_blank_tracks() fills only even indices, so every odd one takes the absent branch and was handed dummy_track together with the preceding track's length. That is safe today by exactly zero margin: the largest entry in track_lengths[] equals GCRIMAGE_DUMMYTRACKLEN. Both loops now build their parameter pair through one function, which gives an absent track the dummy address with the same short, safe length init() and remove_disk() already program, and which cannot see a neighbour at all. A zero or negative length is treated as absent there too, so the division has no way to reach a zero divisor even if a length slips past the checks at load time. mount_g64() now acts on load()'s answer: the file is closed and the drive is left empty, which is the state remove_disk() has already put it in. Reported in GideonZ/1541ultimate#823, problems 1 and 3. --- .github/workflows/build.yml | 6 ++ software/drive/c1541.cc | 52 ++++++------- software/drive/gcr_track_bounds.h | 30 ++++++++ .../drive/tests/gcr_track_bounds_test.cpp | 75 ++++++++++++++++++- 4 files changed, 135 insertions(+), 28 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 534962da0..1f1d9b6c6 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -105,6 +105,12 @@ jobs: -v /opt/build-tools:/mnt/build-tools:ro \ -v $GITHUB_WORKSPACE:/mnt/project:rw my_docker_image /bin/bash -c "cd /mnt/project && make -B -C target/pc/linux/test_http_target test-integer-widths" + - name: Test G64 track bounds + run: | + docker run --rm \ + -v /opt/build-tools:/mnt/build-tools:ro \ + -v $GITHUB_WORKSPACE:/mnt/project:rw my_docker_image /bin/bash -c "cd /mnt/project && make -C software/drive/tests" + - name: Test UCI Palette Commands run: | docker run --rm \ diff --git a/software/drive/c1541.cc b/software/drive/c1541.cc index 84cdb03ae..0e5bffb9a 100644 --- a/software/drive/c1541.cc +++ b/software/drive/c1541.cc @@ -3,6 +3,7 @@ #include #include #include +#include "gcr_track_bounds.h" #include "itu.h" #include "dump_hex.h" @@ -461,40 +462,30 @@ void C1541 :: insert_disk(bool protect, GcrImage *image) uint32_t rotation_speed = (CLOCK_FREQ / 20); // 2 (half clocks) * 1/8 (bytes) * clocks per track. 300 RPM = 5 RPS. (5 * 8 / 2) = 20 // side 0 - uint32_t bit_time; - uint32_t track_len; volatile uint32_t *param = (volatile uint32_t *)®isters[C1541_PARAM_RAM]; for(int i=0; i < GCRIMAGE_FIRSTTRACKSIDE1; i++) { GcrTrack *tr = &image->tracks[i]; - if (tr->track_address) { - bit_time = rotation_speed / tr->track_length; - track_len = tr->track_length; - // printf("Side0: %2d %08x %08x %d\n", i, tr->track_address, track_len, bit_time); - *(param++) = (uint32_t)tr->track_address; - *(param++) = (track_len-1) | (bit_time << 16); - } else { - // printf("Side0: %2d %08x %08x %d\n", i, tr->track_address, track_len, bit_time); - *(param++) = (uint32_t)dummy_track; - *(param++) = (track_len-1) | (bit_time << 16); - } + uint32_t track_addr, track_param; + gcr_track_parameters((uint32_t)tr->track_address, tr->track_length, + (uint32_t)dummy_track, GCRIMAGE_DUMMYTRACKLEN, + rotation_speed, &track_addr, &track_param); + // printf("Side0: %2d %08x %08x\n", i, track_addr, track_param); + *(param++) = track_addr; + *(param++) = track_param; registers[C1541_DIRTYFLAGS + i/2] = 0; - } + } // Side 1 param = (volatile uint32_t *)®isters[C1541_PARAM_RAM + 0x400]; // side 1 for(int i=GCRIMAGE_FIRSTTRACKSIDE1; i < GCRIMAGE_MAXHDRTRACKS; i++) { GcrTrack *tr = &image->tracks[i]; - if (tr->track_address) { - bit_time = rotation_speed / tr->track_length; - track_len = tr->track_length; - // printf("Side1: %2d %08x %08x %d\n", i, tr->track_address, tr->track_length, bit_time); - *(param++) = (uint32_t)tr->track_address; - *(param++) = (track_len-1) | (bit_time << 16); - } else { - // printf("Side1: %2d %08x %08x %d\n", i, tr->track_address, tr->track_length, bit_time); - *(param++) = (uint32_t)dummy_track; - *(param++) = (track_len-1) | (bit_time << 16); - } + uint32_t track_addr, track_param; + gcr_track_parameters((uint32_t)tr->track_address, tr->track_length, + (uint32_t)dummy_track, GCRIMAGE_DUMMYTRACKLEN, + rotation_speed, &track_addr, &track_param); + // printf("Side1: %2d %08x %08x\n", i, track_addr, track_param); + *(param++) = track_addr; + *(param++) = track_param; registers[C1541_DIRTYFLAGS + i/2] = 0; } @@ -615,7 +606,16 @@ void C1541 :: mount_g64(bool protect, File *file) return; } printf("Loading..."); - gcr_image->load(file); + if (!gcr_image->load(file)) { + // load() has already invalidated the image, so its track table holds + // nothing that may be programmed. remove_disk() ran above, so an empty + // drive is the state the machine is already in. + printf("Failed. Not a G64 image, or its track table does not fit.\n"); + fm->fclose(mount_file); + mount_file = NULL; + drive_reset(0); // do not reset, but restore the freeze + return; + } printf("Inserting..."); insert_disk(protect, gcr_image); printf("Done\n"); diff --git a/software/drive/gcr_track_bounds.h b/software/drive/gcr_track_bounds.h index a48c1ed98..45f00f1d3 100644 --- a/software/drive/gcr_track_bounds.h +++ b/software/drive/gcr_track_bounds.h @@ -16,6 +16,11 @@ #include +/* The low half of a parameter word is the last valid offset in the track, so + * this is a 257-byte empty track. init() and remove_disk() already program it + * for a drive with nothing in it. */ +#define GCR_EMPTY_TRACK_PARAM 0x100 + /* A track table entry points at a two-byte length word followed by the track * data. Reading that word is itself an access into the image buffer, so it has * to lie inside the part of the buffer the file actually filled. Offset zero @@ -57,4 +62,29 @@ static inline int gcr_validated_track_length(uint16_t declared, uint32_t offset, return length; } +/* Builds one entry of the drive's track parameter RAM: the address the engine + * works at, and a word carrying the last valid offset in the low half and the + * bit time in the high half. + * + * A track that is not in the image gets the dummy track together with the same + * short, safe length init() and remove_disk() use -- not whatever length the + * previous track happened to leave in a local. + */ +static inline void gcr_track_parameters(uint32_t track_address, int track_length, + uint32_t dummy_address, int dummy_length, + uint32_t rotation_speed, + uint32_t *out_address, uint32_t *out_param) +{ + if (track_address != 0 && track_length > 0) { + uint32_t bit_time = rotation_speed / (uint32_t)track_length; + *out_address = track_address; + *out_param = (uint32_t)(track_length - 1) | (bit_time << 16); + return; + } + + uint32_t bit_time = (dummy_length > 0) ? (rotation_speed / (uint32_t)dummy_length) : 0; + *out_address = dummy_address; + *out_param = GCR_EMPTY_TRACK_PARAM | (bit_time << 16); +} + #endif /* DRIVE_GCR_TRACK_BOUNDS_H */ diff --git a/software/drive/tests/gcr_track_bounds_test.cpp b/software/drive/tests/gcr_track_bounds_test.cpp index fdcd7c38e..3b90c95e3 100644 --- a/software/drive/tests/gcr_track_bounds_test.cpp +++ b/software/drive/tests/gcr_track_bounds_test.cpp @@ -2,10 +2,11 @@ // program the drive engine's memory bounds without ever being checked against // the buffer that holds the image. // -// Two of the four problems in that issue are pinned down here; the parameter -// RAM side follows in its own change. +// The four problems in that issue, and where each is pinned down below: // +// 1. parameter RAM programmed from uninitialised locals -> TrackParameters // 2. track length never validated against the buffer -> ValidatedLength +// 3. half-tracks inherit the previous track's length -> TrackParameters // 4. divide by zero on a zero-length track -> ValidatedLength #include "../../io/usb/tests/host_test/host_test.h" @@ -16,6 +17,8 @@ namespace { // The real constants, so the numbers here are the ones the firmware uses. const uint32_t kMaxSize = (0x1EF8 * 80) + (12 + (168 * 10)); // GCRIMAGE_MAXSIZE const int kMaxTrackLen = 0x1EF8; // GCRIMAGE_MAXTRACKLEN +const int kDummyLen = 0x1E0C; // GCRIMAGE_DUMMYTRACKLEN +const uint32_t kRotationSpeed = 50000000 / 20; // CLOCK_FREQ / 20 on the U64 } // namespace @@ -106,3 +109,71 @@ TEST(HeaderReadable, RejectsAPointerPastWhatWasRead) EXPECT_FALSE(gcr_track_header_is_readable(8192, 4096)); EXPECT_FALSE(gcr_track_header_is_readable(0xFFFFFFFFu, 4096)); } + +// ------------------------------------------------------------- problems 1+3 -- + +TEST(TrackParameters, ProgramsARealTrackFromItsOwnLength) +{ + uint32_t address = 0, param = 0; + gcr_track_parameters(0x00800000, 0x1E0C, 0x00900000, kDummyLen, kRotationSpeed, + &address, ¶m); + EXPECT_EQ(address, 0x00800000u); + EXPECT_EQ(param & 0xFFFFu, 0x1E0Bu); // last valid offset + EXPECT_EQ(param >> 16, kRotationSpeed / 0x1E0C); // bit time +} + +TEST(TrackParameters, GivesAnAbsentTrackTheDummyAndASafeLength) +{ + // Problem 1: before the fix the else branch read two locals that the if + // branch had never written, so the first track of an image whose tracks + // are all absent programmed dummy_track with whatever was on the stack. + uint32_t address = 0, param = 0; + gcr_track_parameters(0, 0, 0x00900000, kDummyLen, kRotationSpeed, &address, ¶m); + EXPECT_EQ(address, 0x00900000u); + EXPECT_EQ(param & 0xFFFFu, (uint32_t)GCR_EMPTY_TRACK_PARAM); + EXPECT_EQ(param >> 16, kRotationSpeed / (uint32_t)kDummyLen); +} + +TEST(TrackParameters, DoesNotCarryTheNeighbourLengthIntoAnAbsentTrack) +{ + // Problem 3: add_blank_tracks() fills only even indices, so every odd one + // takes the absent path on every image. It used to be handed the dummy + // track's address together with the length left behind by the preceding + // track, a pairing that is safe only because the longest entry in + // track_lengths[] happens to equal GCRIMAGE_DUMMYTRACKLEN exactly. + // + // What has to hold is that the absent case does not depend on what came + // before it. So: program a long real track, then an absent one, and + // require the same answer as an absent track programmed on its own. + uint32_t alone_address = 0, alone_param = 0; + gcr_track_parameters(0, 0, 0x00900000, kDummyLen, kRotationSpeed, + &alone_address, &alone_param); + + uint32_t address = 0, param = 0; + gcr_track_parameters(0x00800000, kMaxTrackLen, 0x00900000, kDummyLen, kRotationSpeed, + &address, ¶m); + gcr_track_parameters(0, 0, 0x00900000, kDummyLen, kRotationSpeed, &address, ¶m); + + EXPECT_EQ(address, alone_address); + EXPECT_EQ(param, alone_param); + EXPECT_TRUE((int)((param & 0xFFFFu) + 1) <= kDummyLen); +} + +TEST(TrackParameters, TreatsAZeroLengthTrackAsAbsent) +{ + // Problem 4 again, one layer down: an address with a zero length must not + // reach the division either. + uint32_t address = 0, param = 0; + gcr_track_parameters(0x00800000, 0, 0x00900000, kDummyLen, kRotationSpeed, + &address, ¶m); + EXPECT_EQ(address, 0x00900000u); + EXPECT_EQ(param & 0xFFFFu, (uint32_t)GCR_EMPTY_TRACK_PARAM); +} + +TEST(TrackParameters, TreatsANegativeLengthAsAbsent) +{ + uint32_t address = 0, param = 0; + gcr_track_parameters(0x00800000, -1, 0x00900000, kDummyLen, kRotationSpeed, + &address, ¶m); + EXPECT_EQ(address, 0x00900000u); +} From b1e604097b9207c2e44292093e395ac2dcbfe800 Mon Sep 17 00:00:00 2001 From: Enver Haase Date: Wed, 16 Sep 2026 03:21:13 +0200 Subject: [PATCH 3/7] Bound a G64 track by what the file delivered, not by the buffer The first version of this fix compared a declared track length against GCRIMAGE_MAXSIZE, so a track only had to fit the allocation. It did not have to be in the file. A 14 byte GCR-1541 image whose track 0 points at offset 12 and declares 0x1E0C passed both the header check and the length check, and then gcr_data+14 through gcr_data+7705 were programmed into the drive engine -- bytes no read ever wrote, holding whatever the previous mount left behind, since the buffer is zeroed at allocation and not between images. gcr_validated_track_length() now takes bytes_read instead of the capacity and requires offset + 2 + length <= bytes_read, written as subtractions so nothing can wrap, and load() passes what f->read() reported. Since bytes_read never exceeds GCRIMAGE_MAXSIZE, this is strictly the stronger bound and the buffer cases the earlier tests pin down still hold. Five cases added under TruncatedImage, including the reported 14 byte file. Against this commit the suite is 23 green; against a header restored to the capacity bound, 4 of the 5 fail -- the fifth is the positive boundary, a track ending exactly where the file does, which must stay accepted either way. Reported by @chrisgleissner in review of GideonZ/1541ultimate#840. --- software/drive/disk_image.cc | 4 +- software/drive/gcr_track_bounds.h | 20 ++++--- .../drive/tests/gcr_track_bounds_test.cpp | 52 +++++++++++++++++++ 3 files changed, 67 insertions(+), 9 deletions(-) diff --git a/software/drive/disk_image.cc b/software/drive/disk_image.cc index f20aeccef..d56230984 100644 --- a/software/drive/disk_image.cc +++ b/software/drive/disk_image.cc @@ -722,9 +722,9 @@ bool GcrImage :: load(File *f) } tr = gcr_data + offset; w = tr[0] | (uint16_t(tr[1]) << 8); - int length = gcr_validated_track_length(w, offset, GCRIMAGE_MAXSIZE, GCRIMAGE_MAXTRACKLEN); + int length = gcr_validated_track_length(w, offset, bytes_read, GCRIMAGE_MAXTRACKLEN); if(!length) { - printf("Track %d: declared length %d does not fit the image. Track skipped.\n", + printf("Track %d: declared length %d was not delivered by the file. Track skipped.\n", i, (int)(w & 0x3FFF)); continue; // invalidate() left this entry unused; leave it that way } diff --git a/software/drive/gcr_track_bounds.h b/software/drive/gcr_track_bounds.h index 45f00f1d3..c1bc06e7c 100644 --- a/software/drive/gcr_track_bounds.h +++ b/software/drive/gcr_track_bounds.h @@ -38,25 +38,31 @@ static inline bool gcr_track_header_is_readable(uint32_t offset, uint32_t bytes_ * * declared the 16-bit word from the image, flag bits included * offset byte offset of that word within the buffer - * capacity size of the buffer + * bytes_read how much of the buffer the file actually filled * max_length longest track the format allows * - * The masked field holds 14 bits, so it admits lengths more than twice the - * longest legitimate track, and a track near the end of the buffer can declare - * a length that runs off it. Both are rejected here rather than programmed. + * The bound is what was read, not how large the buffer is. A truncated image + * can declare a track whose header is present but whose data never arrived, + * and the buffer behind it holds whatever the previous mount left there. So + * the whole track, header word included, has to lie inside what was read: + * offset + 2 + length <= bytes_read, written as subtractions so that no sum + * can wrap. + * + * The masked field holds 14 bits, so it also admits lengths more than twice + * the longest legitimate track. Rejected here rather than programmed. */ static inline int gcr_validated_track_length(uint16_t declared, uint32_t offset, - uint32_t capacity, int max_length) + uint32_t bytes_read, int max_length) { int length = (int)(declared & 0x3FFF); if (length <= 0 || length > max_length) { return 0; /* zero would also divide by zero in insert_disk() */ } - if (offset > capacity || (capacity - offset) < 2) { + if (offset > bytes_read || (bytes_read - offset) < 2) { return 0; } - if ((uint32_t)length > (capacity - offset - 2)) { + if ((uint32_t)length > (bytes_read - offset - 2)) { return 0; } return length; diff --git a/software/drive/tests/gcr_track_bounds_test.cpp b/software/drive/tests/gcr_track_bounds_test.cpp index 3b90c95e3..bedafbccf 100644 --- a/software/drive/tests/gcr_track_bounds_test.cpp +++ b/software/drive/tests/gcr_track_bounds_test.cpp @@ -8,6 +8,10 @@ // 2. track length never validated against the buffer -> ValidatedLength // 3. half-tracks inherit the previous track's length -> TrackParameters // 4. divide by zero on a zero-length track -> ValidatedLength +// +// And what checking the buffer alone still let through, found in review of the +// first fix: a truncated file whose track header arrived but whose track data +// did not -> TruncatedImage. #include "../../io/usb/tests/host_test/host_test.h" #include "../gcr_track_bounds.h" @@ -23,6 +27,11 @@ const uint32_t kRotationSpeed = 50000000 / 20; // CLOCK_FREQ } // namespace // ---------------------------------------------------------------- problem 2 -- +// +// The third argument is how much of the buffer the file filled. Passing +// kMaxSize here models an image that filled it completely, which is what the +// buffer-overrun cases below are about; the truncated cases further down pass +// the smaller number a short file leaves behind. TEST(ValidatedLength, AcceptsALegitimateTrack) { @@ -75,6 +84,49 @@ TEST(ValidatedLength, SurvivesAnOffsetPastTheBuffer) EXPECT_EQ(gcr_validated_track_length(0x1E0C, kMaxSize - 1, kMaxSize, kMaxTrackLen), 0); } +// ------------------------------------------------------------ truncation -- +// +// Reported by @chrisgleissner against the first version of this fix, which +// bounded the track by the size of the buffer rather than by what the file +// delivered. The buffer is not zeroed between mounts, so what lies past the +// end of a short file is the previous image. + +TEST(TruncatedImage, RejectsATrackWhoseDataNeverArrived) +{ + // The reported case, exactly: a 14-byte GCR-1541 file. Track 0 points at + // offset 12, the two length bytes are there, and they declare a full + // 0x1E0C track. Against the buffer this passes; against the file it must + // not, or gcr_data+14 .. gcr_data+7705 go to the drive unread. + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 12, 14, kMaxTrackLen), 0); +} + +TEST(TruncatedImage, RejectsATrackCutShortByOneByte) +{ + const uint32_t bytes_read = 12 + 2 + 0x1E0C - 1; + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 12, bytes_read, kMaxTrackLen), 0); +} + +TEST(TruncatedImage, AcceptsATrackEndingExactlyWhereTheFileDoes) +{ + const uint32_t bytes_read = 12 + 2 + 0x1E0C; + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 12, bytes_read, kMaxTrackLen), 0x1E0C); +} + +TEST(TruncatedImage, DoesNotCareHowLargeTheBufferIs) +{ + // Same track, same short file. The only difference between these two calls + // used to be the answer: the buffer was the bound, so the whole image was + // judged by memory that had nothing to do with it. + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 12, kMaxSize, kMaxTrackLen), 0x1E0C); + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 12, 14, kMaxTrackLen), 0); +} + +TEST(TruncatedImage, SurvivesAnEmptyRead) +{ + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 12, 0, kMaxTrackLen), 0); + EXPECT_EQ(gcr_validated_track_length(0x1E0C, 0, 0, kMaxTrackLen), 0); +} + // ---------------------------------------------------------------- problem 4 -- TEST(ValidatedLength, RejectsAZeroLengthTrack) From ad56e8e835c1338adc964e6b1f4463d95c612a18 Mon Sep 17 00:00:00 2001 From: Enver Haase Date: Fri, 18 Sep 2026 19:16:04 +0200 Subject: [PATCH 4/7] Do not read an MFM header out of a track too short to hold one Length and MFM marker share one word in a G64, so 0x8001 -- one byte of track, marked MFM -- is a legal thing to put in an image, and the bound added in the previous commit passes it: the one declared byte really was delivered. map_gcr_image_to_mfm() then believed the marker and read the whole 162 byte metadata area out of that one byte. Placed at the end of the buffer, as in the reported case -- a file of exactly GCRIMAGE_MAXSIZE with track 0 pointing at GCRIMAGE_MAXSIZE - 3 and a sector count of 32 in the final byte -- the sector loop runs past the allocation. The space behind the area was taken as a subtraction into reservedSpace, a uint32_t that gates every track write, where 1 - 162 cannot come out negative. MFM_TRACK_HEADER_SIZE was also unparenthesised, so x - MFM_TRACK_HEADER_SIZE expanded to x - 2 + 160. Every mapped track reserved 320 bytes more than it had, and the underflow above never happened because the subtraction was not one. The macro's three other uses -- an addition, a comparison, and - 2 -- come out right either way, which is why this stayed hidden. The static_assert beside it fails if the brackets are lost again; comparing the macro against 162 would not, since 2 + 160 equals 162. disk_image.h now includes mfmdisk.h, which defines the WD_MAX_SECTORS_PER_TRACK the macro has always used. A track shorter than the metadata area is left as init() zeroed it: no sectors, no reserved space, so UpdateTrack() admits no write for it. Six cases added under MfmHeader, including the reported end of buffer file. Against this commit the suite is 29 green; with gcr_mfm_reserved_space() restored to a bare subtraction the short track reserves 4294967135 bytes, and with the header check restored to believing the marker the reported case is mapped. Reported by @chrisgleissner in review of GideonZ/1541ultimate#840. --- software/drive/c1541.cc | 9 ++- software/drive/disk_image.h | 10 ++- software/drive/gcr_track_bounds.h | 35 +++++++++ .../drive/tests/gcr_track_bounds_test.cpp | 72 +++++++++++++++++++ 4 files changed, 124 insertions(+), 2 deletions(-) diff --git a/software/drive/c1541.cc b/software/drive/c1541.cc index 0e5bffb9a..74f752015 100644 --- a/software/drive/c1541.cc +++ b/software/drive/c1541.cc @@ -518,7 +518,14 @@ void C1541 :: map_gcr_image_to_mfm(void) if (!gcr || !mtr) { continue; } - mtr->reservedSpace = gtr->track_length - MFM_TRACK_HEADER_SIZE; + // A track shorter than the metadata area cannot carry one, whatever + // its marker says, and there is no space behind an area that does + // not fit. Leave the entry as init() zeroed it: no sectors, no + // reserved space, so UpdateTrack() admits no write for this track. + if (!gcr_track_can_hold_mfm_header(gtr->track_length, MFM_TRACK_HEADER_SIZE)) { + continue; + } + mtr->reservedSpace = gcr_mfm_reserved_space(gtr->track_length, MFM_TRACK_HEADER_SIZE); mtr->offsetInFile = (int)gcr - (int)gcr_image->gcr_data; mtr->offsetInFile += MFM_TRACK_HEADER_SIZE; diff --git a/software/drive/disk_image.h b/software/drive/disk_image.h index 8111b9bd4..19adf4f84 100644 --- a/software/drive/disk_image.h +++ b/software/drive/disk_image.h @@ -10,6 +10,7 @@ #include "menu.h" #include "filemanager.h" #include "subsys.h" +#include "mfmdisk.h" // WD_MAX_SECTORS_PER_TRACK, used by MFM_TRACK_HEADER_SIZE below #define GCR_DECODER_GCR_IN (*(volatile uint8_t *)(GCR_CODER_BASE + 0x00)) #define GCR_DECODER_BIN_OUT0 (*(volatile uint8_t *)(GCR_CODER_BASE + 0x00)) @@ -48,7 +49,14 @@ #define C1541_MIN_D71_SIZE (2*C1541_MAX_D64_35_NO_ERRORS) #define C1541_D71_SIZE_WITH_ERRORS (C1541_MIN_D71_SIZE + 1366) -#define MFM_TRACK_HEADER_SIZE 2 + (WD_MAX_SECTORS_PER_TRACK * 5) +/* Parenthesised because it is subtracted: without the outer brackets + * "x - MFM_TRACK_HEADER_SIZE" expanded to "x - 2 + 160", which is x + 158 + * rather than x - 162, and map_gcr_image_to_mfm() reserved 320 bytes more + * than the track had. The static_assert below fails if they are lost again; + * comparing the macro against 162 would not, since 2 + 160 equals 162. */ +#define MFM_TRACK_HEADER_SIZE (2 + (WD_MAX_SECTORS_PER_TRACK * 5)) +static_assert(0 - MFM_TRACK_HEADER_SIZE == -162, + "MFM_TRACK_HEADER_SIZE must survive being subtracted"); class BinImage; diff --git a/software/drive/gcr_track_bounds.h b/software/drive/gcr_track_bounds.h index c1bc06e7c..2a6a62fa5 100644 --- a/software/drive/gcr_track_bounds.h +++ b/software/drive/gcr_track_bounds.h @@ -68,6 +68,41 @@ static inline int gcr_validated_track_length(uint16_t declared, uint32_t offset, return length; } +/* An MFM track carries a metadata area at its front: a sector count, a version + * byte, and five bytes for each of up to WD_MAX_SECTORS_PER_TRACK sectors. + * map_gcr_image_to_mfm() reads that area whole for a track marked MFM, and + * reserves what lies behind it for sector data. Neither exists unless the + * track is at least as long as the area itself. + * + * Length and MFM marker share one word in a G64, so 0x8001 -- a one-byte track + * marked MFM -- is a legal thing to put in an image. The declared extent then + * checks out: one byte was delivered. The mapping used to believe the marker + * anyway, read the full metadata area from that one byte, and take the space + * behind it as a subtraction in an unsigned field, where it cannot come out + * negative. + * + * track_length the validated length of the track + * header_size bytes of metadata at the front of an MFM track + */ +static inline bool gcr_track_can_hold_mfm_header(int track_length, int header_size) +{ + if (header_size <= 0 || track_length <= 0) { + return false; + } + return track_length >= header_size; +} + +/* Bytes behind the metadata area, or 0 for a track with no room for one. The + * caller keeps this in an unsigned field that gates every track write, so it + * must never be reached by underflow. */ +static inline uint32_t gcr_mfm_reserved_space(int track_length, int header_size) +{ + if (!gcr_track_can_hold_mfm_header(track_length, header_size)) { + return 0; + } + return (uint32_t)(track_length - header_size); +} + /* Builds one entry of the drive's track parameter RAM: the address the engine * works at, and a word carrying the last valid offset in the low half and the * bit time in the high half. diff --git a/software/drive/tests/gcr_track_bounds_test.cpp b/software/drive/tests/gcr_track_bounds_test.cpp index bedafbccf..3b0432e1f 100644 --- a/software/drive/tests/gcr_track_bounds_test.cpp +++ b/software/drive/tests/gcr_track_bounds_test.cpp @@ -12,6 +12,10 @@ // And what checking the buffer alone still let through, found in review of the // first fix: a truncated file whose track header arrived but whose track data // did not -> TruncatedImage. +// +// And what checking the declared extent still let through, found in review of +// the second fix: a track short enough that the MFM mapping's own, larger +// assumption about it does not hold -> MfmHeader. #include "../../io/usb/tests/host_test/host_test.h" #include "../gcr_track_bounds.h" @@ -23,6 +27,7 @@ const uint32_t kMaxSize = (0x1EF8 * 80) + (12 + (168 * 10)); // GCRIMAGE_M const int kMaxTrackLen = 0x1EF8; // GCRIMAGE_MAXTRACKLEN const int kDummyLen = 0x1E0C; // GCRIMAGE_DUMMYTRACKLEN const uint32_t kRotationSpeed = 50000000 / 20; // CLOCK_FREQ / 20 on the U64 +const int kMfmHeaderSize = 2 + (32 * 5); // MFM_TRACK_HEADER_SIZE } // namespace @@ -229,3 +234,70 @@ TEST(TrackParameters, TreatsANegativeLengthAsAbsent) &address, ¶m); EXPECT_EQ(address, 0x00900000u); } + +// --------------------------------------------------------- the MFM header -- +// +// Reported by @chrisgleissner against the second version of this fix. The +// length check above proves that the bytes a track declares were delivered. +// map_gcr_image_to_mfm() then makes a second and larger assumption about the +// same track: that it is long enough to carry the MFM metadata area, which it +// reads whole and reserves the remainder behind. + +TEST(MfmHeader, RejectsATrackTooShortToCarryTheMetadata) +{ + EXPECT_FALSE(gcr_track_can_hold_mfm_header(1, kMfmHeaderSize)); + EXPECT_FALSE(gcr_track_can_hold_mfm_header(kMfmHeaderSize - 1, kMfmHeaderSize)); +} + +TEST(MfmHeader, AcceptsATrackExactlyAsLongAsTheMetadata) +{ + EXPECT_TRUE(gcr_track_can_hold_mfm_header(kMfmHeaderSize, kMfmHeaderSize)); + EXPECT_EQ(gcr_mfm_reserved_space(kMfmHeaderSize, kMfmHeaderSize), 0u); +} + +TEST(MfmHeader, ReservesWhatLiesBehindTheMetadata) +{ + EXPECT_EQ(gcr_mfm_reserved_space(0x1E0C, kMfmHeaderSize), (uint32_t)(0x1E0C - 162)); + EXPECT_EQ(gcr_mfm_reserved_space(1000, kMfmHeaderSize), 838u); +} + +TEST(MfmHeader, NeverUnderflowsTheReservedSpace) +{ + // The field is a uint32_t and gates every track write. A short track used + // to reach it as a subtraction, so "1 - 162" became 4294967135 and + // UpdateTrack() admitted a write of any size at all. + EXPECT_EQ(gcr_mfm_reserved_space(1, kMfmHeaderSize), 0u); + EXPECT_EQ(gcr_mfm_reserved_space(0, kMfmHeaderSize), 0u); + EXPECT_EQ(gcr_mfm_reserved_space(-1, kMfmHeaderSize), 0u); + for (int len = -4; len < kMfmHeaderSize; len++) { + EXPECT_EQ(gcr_mfm_reserved_space(len, kMfmHeaderSize), 0u); + } +} + +TEST(MfmHeader, TheReportedEndOfBufferCase) +{ + // @chrisgleissner's file, exactly: GCRIMAGE_MAXSIZE bytes long, track 0 + // pointing at GCRIMAGE_MAXSIZE - 3, declaring 0x8001 -- one byte of track, + // marked MFM -- with a sector count of 32 in the final byte of the file. + const uint32_t offset = kMaxSize - 3; + const uint16_t declared = 0x8001; + + // The header is readable and the one declared byte really was delivered, + // so the length check passes it. That is correct and not the bug. + EXPECT_TRUE(gcr_track_header_is_readable(offset, kMaxSize)); + EXPECT_EQ(gcr_validated_track_length(declared, offset, kMaxSize, kMaxTrackLen), 1); + + // The mapping is where it has to stop. Believing the marker would read the + // full 162-byte metadata area from a track holding one byte at the very end + // of the buffer, so the sector loop runs past the allocation. + const int length = gcr_validated_track_length(declared, offset, kMaxSize, kMaxTrackLen); + EXPECT_FALSE(gcr_track_can_hold_mfm_header(length, kMfmHeaderSize)); + EXPECT_EQ(gcr_mfm_reserved_space(length, kMfmHeaderSize), 0u); +} + +TEST(MfmHeader, TreatsAnAbsurdHeaderSizeAsNoHeader) +{ + EXPECT_FALSE(gcr_track_can_hold_mfm_header(0x1E0C, 0)); + EXPECT_FALSE(gcr_track_can_hold_mfm_header(0x1E0C, -162)); + EXPECT_EQ(gcr_mfm_reserved_space(0x1E0C, 0), 0u); +} From be8452c5102ff25f6b933f851bbefb251c13214a Mon Sep 17 00:00:00 2001 From: Enver Haase Date: Fri, 25 Sep 2026 15:59:27 +0200 Subject: [PATCH 5/7] Read the track length from the fifteen bits it is written with The length word carries the MFM marker in bit 15, so the length is what the other fifteen bits say. load() masked with 0x3FFF and took fourteen, and nothing anywhere gives bit 14 a meaning: it is not a flag in this firmware, not in Denise, which reads the same marker and masks with 0x7fff, and not in the published G64 format, where the field is a plain size. The writers never masked at all. save() at disk_image.cc:847 and write_track() at :924 both build the word from reported_length >> 8 and then set bit 15 on top, so an image could not be read back as it was written. A declared 0x4123 came out as 0x0123: 291 bytes, small enough to pass every check in gcr_validated_track_length() and be programmed as a track. That is the part worth stating plainly, because the mask defeats the function it sits in. Read as a length, 0x4123 is 16675, above GCRIMAGE_MAXTRACKLEN, and the track is refused with the diagnostic this file already prints. Masked, it became a plausible short track and was programmed instead -- "rejected here rather than programmed" is what the comment above the function promises. Where 0x3FFF came from is not a mystery: the drive engine's parameter field is exactly fourteen bits wide, so masking the declared length to the same width reads like the obvious thing to do. It holds everywhere except here. Decoding the file and asking what the drive can do are separate steps. The drive engine's ceiling is 16383: max_offset in floppy_param_mem.vhd is fourteen bits wide (line 36), and floppy_mem.vhd wraps offset_count at it for reads and writes alike. That ceiling never comes into play here, because the bound this function already applies -- GCRIMAGE_MAXTRACKLEN, 7928 -- is less than half of it. So the parse can stay faithful to the file and let the bound do the rejecting, which is what it is for. Both facts are now written into the comment above the function, so the next reader does not have to rediscover where 0x3FFF came from. against this commit 31 passed against the same header masking with 0x3FFF 2 failed The two are the new cases, 0x4123 with and without the MFM marker, each returning 291 where it has to return 0. No hardware is involved: these are free functions over plain integers, and make -C software/drive/tests runs them in about a second. --- software/drive/disk_image.cc | 2 +- software/drive/gcr_track_bounds.h | 20 ++++++++++++++++--- .../drive/tests/gcr_track_bounds_test.cpp | 16 +++++++++++++++ 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/software/drive/disk_image.cc b/software/drive/disk_image.cc index d56230984..b9dc2ffbd 100644 --- a/software/drive/disk_image.cc +++ b/software/drive/disk_image.cc @@ -725,7 +725,7 @@ bool GcrImage :: load(File *f) int length = gcr_validated_track_length(w, offset, bytes_read, GCRIMAGE_MAXTRACKLEN); if(!length) { printf("Track %d: declared length %d was not delivered by the file. Track skipped.\n", - i, (int)(w & 0x3FFF)); + i, (int)(w & 0x7FFF)); continue; // invalidate() left this entry unused; leave it that way } tracks[i].track_address = tr + 2; diff --git a/software/drive/gcr_track_bounds.h b/software/drive/gcr_track_bounds.h index 2a6a62fa5..422213b47 100644 --- a/software/drive/gcr_track_bounds.h +++ b/software/drive/gcr_track_bounds.h @@ -48,13 +48,27 @@ static inline bool gcr_track_header_is_readable(uint32_t offset, uint32_t bytes_ * offset + 2 + length <= bytes_read, written as subtractions so that no sum * can wrap. * - * The masked field holds 14 bits, so it also admits lengths more than twice - * the longest legitimate track. Rejected here rather than programmed. + * The length is taken from fifteen bits, not fourteen. Bit 15 is the MFM + * marker this firmware writes and Denise reads; nothing gives bit 14 a + * meaning, so it belongs to the length. write_track() and save() already + * write the length unmasked and set bit 15 on top of it, so reading fourteen + * bits meant the image could not be read back as it was written: a declared + * 0x4123 came out as 0x0123, small enough to pass every check below and be + * programmed as a 291 byte track. + * + * The width is not arbitrary -- the drive's own parameter field is fourteen + * bits -- but the file and the engine are two different things, and masking + * at parse time conflates them. The drive engine's own ceiling is 16383: + * max_offset in floppy_param_mem.vhd is fourteen bits wide, and + * floppy_mem.vhd wraps offset_count at it for reads and writes alike. That + * ceiling never comes into play here, because max_length -- 7928 for a G64, + * GCRIMAGE_MAXTRACKLEN -- is less than half of it and is checked below. So + * the parse stays faithful to the file and the bound does the rejecting. */ static inline int gcr_validated_track_length(uint16_t declared, uint32_t offset, uint32_t bytes_read, int max_length) { - int length = (int)(declared & 0x3FFF); + int length = (int)(declared & 0x7FFF); if (length <= 0 || length > max_length) { return 0; /* zero would also divide by zero in insert_disk() */ diff --git a/software/drive/tests/gcr_track_bounds_test.cpp b/software/drive/tests/gcr_track_bounds_test.cpp index 3b0432e1f..c33ec21e4 100644 --- a/software/drive/tests/gcr_track_bounds_test.cpp +++ b/software/drive/tests/gcr_track_bounds_test.cpp @@ -54,6 +54,22 @@ TEST(ValidatedLength, IgnoresTheFlagBitsAboveTheLength) EXPECT_EQ(gcr_validated_track_length(0x8000 | 0x1E0C, 0x2000, kMaxSize, kMaxTrackLen), 0x1E0C); } +TEST(ValidatedLength, ReadsBitFourteenAsLengthAndRejectsWhatItMakes) +{ + // Nothing gives bit 14 a meaning, and the writer sets neither a mask nor a + // flag there, so it is part of the length. Masking it away turned 0x4123 + // into 291 -- a plausible track that passes every check and gets + // programmed. Read as length it is 16675, above the format maximum, and + // the track is refused instead. + EXPECT_EQ(gcr_validated_track_length(0x4123, 0x2000, kMaxSize, kMaxTrackLen), 0); +} + +TEST(ValidatedLength, ReadsBitFourteenAsLengthWithTheMfmMarkerSet) +{ + // Both flag positions at once: the marker is stripped, the rest is length. + EXPECT_EQ(gcr_validated_track_length(0x8000 | 0x4123, 0x2000, kMaxSize, kMaxTrackLen), 0); +} + TEST(ValidatedLength, RejectsALengthAboveTheFormatMaximum) { // 14 bits admit 16383, more than twice the longest real track. Before the From 4c79cd1a2c3e4ae6e8924a7ee67e698ee6c83624 Mon Sep 17 00:00:00 2001 From: Enver Haase Date: Fri, 25 Sep 2026 16:10:35 +0200 Subject: [PATCH 6/7] Exercise a mixed GCR/MFM G71 against the bounds this branch adds The checks in gcr_track_bounds.h are host tests over plain integers, which is what makes them worth having, but nothing so far mounts an image that actually trips them on a device. This suite does, and it is the first thing anywhere to drive the firmware's mixed GCR/MFM path on real hardware. Three G71 images, built in the test rather than committed, because what is interesting about them is the two-byte length word in front of each track and a binary fixture would hide it: mixed 35 GCR tracks and one track carrying the MFM payload this firmware writes -- sector count, version byte of zero, five bytes per sector, sector data from offset 162 on, exactly as mfm_update_callback() lays it out. shortmfm the same disk with the MFM-marked track declaring 0x8001. The metadata area alone is 162 bytes, so there is nothing to read. overlong the same disk with a track declaring 0x4123: 16675 read as a length, 291 read as fourteen bits. Each case asserts the same two things. The device is still reachable, and track 18 sector 1 still comes back. A malformed track on track 39 or 40 may not cost the rest of the disk, and that is the property these bounds are for. The block comes back through the drive's own U1 command rather than a job queue, so the reader needs no ROM entry point and does not change when the drive model does; an I before it makes the DOS re-read the BAM instead of refusing a block whose header carries an ID it does not expect yet. The GCR encoder is 30 lines and is checked by its own output: the sentinel written into track 18 sector 1 decodes back byte for byte through the inverse table, and the longest run of zero bits in the encoded stream is two, which is the defining property of the code. Track lengths land inside each speed zone's nominal size with room to spare -- 6897 of 7142 on track 18. lint_test OK (ruff 0.16.5 over tests, run-tests) registry_test OK (71 suites) runner_policy OK (99 checks) 64tass g71_block_reader.asm assembles, 259 bytes at $0801 Not run on a device yet: the suite is registered and its fixtures are verified, but the device half needs hardware on the network. Whoever runs it first should expect the two malformed cases to be the interesting ones. --- run-tests | 7 + tests/e2e/drive/g71_block_reader.asm | 173 ++++++++++++ tests/e2e/drive/g71_mfm_bounds_test.py | 374 +++++++++++++++++++++++++ 3 files changed, 554 insertions(+) create mode 100644 tests/e2e/drive/g71_block_reader.asm create mode 100644 tests/e2e/drive/g71_mfm_bounds_test.py diff --git a/run-tests b/run-tests index 4b87a1bf2..3a1d55e89 100755 --- a/run-tests +++ b/run-tests @@ -295,6 +295,13 @@ SUITES: Sequence[Suite] = ( # oversized D81 and proves the bounded layout leaves the device usable. Suite("e2e", "d81-track-bounds", "tests/e2e/drive/d81_track_bounds_test.py", "-H @HOST@ -p @PASS@ -t @TIMEOUT@ --test all", profile=profiles.QUICK), + # Mounts a G71 whose MFM-marked track is malformed -- one declaring a + # single byte where the metadata area alone is 162, and one declaring + # 0x4123, which is 16675 read as a length and 291 read as fourteen bits -- + # and proves the GCR side of the same disk still reads afterwards. The + # first exercise of the mixed GCR/MFM path on real hardware. + Suite("e2e", "g71-mfm-bounds", "tests/e2e/drive/g71_mfm_bounds_test.py", + "-H @HOST@ -p @PASS@ -t @TIMEOUT@", profile=profiles.QUICK), # Calls every endpoint the firmware registers, and fails when one is # neither probed nor excluded with a reason. The lockup that prompted it # shipped because no suite called that route at all, so the gate matters diff --git a/tests/e2e/drive/g71_block_reader.asm b/tests/e2e/drive/g71_block_reader.asm new file mode 100644 index 000000000..e9c2cefc2 --- /dev/null +++ b/tests/e2e/drive/g71_block_reader.asm @@ -0,0 +1,173 @@ +; Read one block through the drive's U1 command and hand it to the host. +; +; U1 is used on purpose: it is served by the DOS of a 1541, a 1571 and a 1581 +; alike, so this reader needs no ROM entry point and no job queue, and it does +; not change when the drive model does. The host supplies DEVICE, TRACK and +; SECTOR and checks the copied block at RESULT_DATA. + +SETLFS = $ffba +SETNAM = $ffbd +OPEN = $ffc0 +CLOSE = $ffc3 +CHKIN = $ffc6 +CHKOUT = $ffc9 +CLRCHN = $ffcc +CHRIN = $ffcf +CHROUT = $ffd2 +READST = $ffb7 + +RESULT_STATUS = $c000 +RESULT_READY = $c001 +RESULT_DOS = $c002 +RESULT_IO = $c003 +RESULT_DATA = $c100 + +STATUS_RUNNING = $00 +STATUS_DONE = $01 +STATUS_IO_ERROR = $02 +READY_MARK = $a5 + +* = $0801 + .word basic_end, 2026 + .null $9e, format("%d", start) +basic_end: + .word 0 + +start: + lda #STATUS_RUNNING + sta RESULT_STATUS + lda #READY_MARK + sta RESULT_READY + lda #0 + sta RESULT_DOS + sta RESULT_IO + + ; command channel + lda #15 + ldx #DEVICE + ldy #15 + jsr SETLFS + lda #0 + ldx #0 + ldy #0 + jsr SETNAM + jsr OPEN + bcs io_error + + ; buffer channel on "#" + lda #2 + ldx #DEVICE + ldy #2 + jsr SETLFS + lda #1 + ldx #buffer_name + jsr SETNAM + jsr OPEN + bcs io_error + + jsr init_drive + jsr send_u1 + jsr read_block + jsr read_error_channel + + lda #STATUS_DONE +finish: + sta RESULT_STATUS + lda #2 + jsr CLOSE + lda #15 + jsr CLOSE + jsr CLRCHN + rts + +io_error: + jsr READST + sta RESULT_IO + lda #STATUS_IO_ERROR + bne finish + +; The DOS caches the disk ID and refuses a block whose header carries another +; one. An initialise makes it re-read the BAM, so a freshly mounted image is +; read on its own terms rather than on the previous mount's. +init_drive: + ldx #15 + jsr CHKOUT + bcc + + jmp io_error ++ + ldy #0 +- lda init_command,y + jsr CHROUT + iny + cpy #init_command_end-init_command + bne - + lda #13 + jsr CHROUT + jsr CLRCHN + rts + +send_u1: + ldx #15 + jsr CHKOUT + bcc + + jmp io_error ++ + ldy #0 +- lda u1_command,y + jsr CHROUT + iny + cpy #u1_command_end-u1_command + bne - + lda #13 + jsr CHROUT + jsr CLRCHN + rts + +read_block: + ldx #2 + jsr CHKIN + bcc + + jmp io_error ++ + ldy #0 +- jsr CHRIN + sta RESULT_DATA,y + iny + bne - + jsr CLRCHN + rts + +; The first byte of the DOS answer is the tens digit of the error number, the +; second the units. Both are kept so that a failure says which error it was +; rather than only that there was one. +read_error_channel: + ldx #15 + jsr CHKIN + bcc + + jmp io_error ++ + jsr CHRIN + and #$0f + asl + asl + asl + asl + sta RESULT_DOS + jsr CHRIN + and #$0f + ora RESULT_DOS + sta RESULT_DOS + jsr CLRCHN + rts + +buffer_name: + .text "#" + +init_command: + .text "i0" +init_command_end: + +u1_command: + .text "u1 2 0 ", format("%d", TRACK), " ", format("%d", SECTOR) +u1_command_end: diff --git a/tests/e2e/drive/g71_mfm_bounds_test.py b/tests/e2e/drive/g71_mfm_bounds_test.py new file mode 100644 index 000000000..1d0998ada --- /dev/null +++ b/tests/e2e/drive/g71_mfm_bounds_test.py @@ -0,0 +1,374 @@ +#!/usr/bin/env python3 +"""E2E: a G71 whose MFM-marked tracks are malformed must not take the drive +with it, and the GCR side of the same disk has to keep reading. + +The fixtures are built here rather than shipped, because what is interesting +about them is exactly the two-byte length word in front of each track, and a +committed binary would hide it. Three images, one normal and two malformed in +the ways the bounds checks in `gcr_track_bounds.h` are about: + + mixed a well-formed disk: GCR tracks, and one track carrying the MFM + payload this firmware writes -- sector count, version byte, five + bytes per sector, sector data from offset 162 on. + shortmfm the same disk, but the MFM-marked track declares one single byte. + The metadata area alone is 162, so there is nothing to read; the + mapping used to read it anyway. + overlong the same disk, but one track declares 0x4123. Read as fifteen + bits that is 16675, above GCRIMAGE_MAXTRACKLEN, and the track has + to be refused. Masked to fourteen it became 291 and was mounted. + +Every case asserts the same two things: the device is still reachable, and +track 18 sector 0 still comes back through the drive's own U1 command. A +malformed track elsewhere on the disk may not cost the rest of it. +""" + +import argparse +import posixpath +import sys +import time +from pathlib import Path + +SCRIPT_DIR = Path(__file__).resolve().parent + +# The one stanza that puts the shared library on sys.path; see tests/lib/bootstrap.py. +sys.path.insert(0, str(next(p for p in Path(__file__).resolve().parents + if (p / "tests" / "lib").is_dir()) / "tests" / "lib")) +import bootstrap # noqa: E402,F401 +import cli # noqa: E402 + +import ftp as ftp_lib # noqa: E402 +from api import DriveInfo, UltimateApi # noqa: E402 +from assembler import assemble # noqa: E402 +from report import ( # noqa: E402 + Failure, + check, + detail, + format_exception, + section, + suite_fail, + suite_ok, +) + +SUITE = "g71_mfm_bounds_test" +SOURCE = SCRIPT_DIR / "g71_block_reader.asm" + +# ---------------------------------------------------------------- GCR encoding + +# The 1541's 4-to-5 code. Its point is that no legal sequence has more than two +# zero bits in a row, which is what makes a run of zeros recognisable as an +# unformatted or weak area rather than as data. +GCR_NIBBLE = ( + 0x0a, 0x0b, 0x12, 0x13, 0x0e, 0x0f, 0x16, 0x17, + 0x09, 0x19, 0x1a, 0x1b, 0x0d, 0x1d, 0x1e, 0x15, +) + +SYNC = b"\xff" * 5 +GAP = b"\x55" + +# Sectors per track, and the nominal track length of each speed zone. +ZONES = ((31, 0, 17, 6250), (25, 1, 18, 6666), (18, 2, 19, 7142), (1, 3, 21, 7692)) + +DISK_ID = b"E2" + + +def zone_of(track: int) -> tuple: + """Returns (speed zone, sectors, nominal length) for a 1541 track.""" + for first, zone, sectors, length in ZONES: + if track >= first: + return zone, sectors, length + raise ValueError(track) + + +def gcr_encode(data: bytes) -> bytes: + """Four bytes in, five out: eight nibbles of five bits each.""" + if len(data) % 4: + raise ValueError("GCR encodes whole groups of four bytes") + out = bytearray() + for i in range(0, len(data), 4): + bits = 0 + for byte in data[i:i + 4]: + bits = (bits << 5) | GCR_NIBBLE[byte >> 4] + bits = (bits << 5) | GCR_NIBBLE[byte & 0x0f] + out += bits.to_bytes(5, "big") + return bytes(out) + + +def sector_image(track: int, sector: int, payload: bytes) -> bytes: + """One sector as it lies on the medium: header, gap, data, gap.""" + if len(payload) != 256: + raise ValueError("a sector holds 256 bytes") + id1, id2 = DISK_ID[0], DISK_ID[1] + header = bytes((0x08, sector ^ track ^ id2 ^ id1, sector, track, id2, id1, 0x0f, 0x0f)) + checksum = 0 + for byte in payload: + checksum ^= byte + block = bytes((0x07,)) + payload + bytes((checksum, 0x00, 0x00)) + return (SYNC + gcr_encode(header) + GAP * 9 + + SYNC + gcr_encode(block) + GAP * 9) + + +def gcr_track(track: int, payload_of) -> bytes: + """A whole GCR track, padded to its zone's nominal length.""" + _, sectors, length = zone_of(track) + data = b"".join(sector_image(track, s, payload_of(track, s)) for s in range(sectors)) + if len(data) > length: + raise ValueError(f"track {track} came out at {len(data)}, zone allows {length}") + return data + GAP * (length - len(data)) + + +# --------------------------------------------------------------- image layout + +SIGNATURE = b"GCR-1571" +HALF_TRACKS = 168 # 84 per side, the 1571 in double-sided mode +FIRST_SIDE_1 = 84 +MAX_TRACK_SIZE = 7928 # the usual value; the header is what actually binds +SLOT = MAX_TRACK_SIZE + 2 # length word plus the track it announces +TABLES = 12 + HALF_TRACKS * 4 * 2 + +DOS_TRACK = 18 +SENTINEL_SECTOR = 1 +SENTINEL = bytes((value ^ 0xa5) for value in range(256)) + +# Physical track 40 on side 0. Outside what the DOS ever touches, so a broken +# one cannot be confused with a broken directory. +MFM_HALF_TRACK = (40 - 1) * 2 +OVERLONG_HALF_TRACK = (39 - 1) * 2 + +# The payload this firmware writes for an MFM track: a sector count, a version +# byte of zero, and five bytes for each of up to 32 sectors -- track, side, +# sector, size code, error byte. Sector data starts behind that fixed area, +# whatever the sector count is (c1541.cc, mfm_update_callback). +MFM_MAX_SECTORS = 32 +MFM_HEADER_SIZE = 2 + MFM_MAX_SECTORS * 5 +MFM_SECTORS = 10 +MFM_SIZE_CODE = 2 # 1 << (7 + 2) = 512 bytes +MFM_SECTOR_BYTES = 1 << (7 + MFM_SIZE_CODE) + +MFM_MARKER = 0x8000 + + +def bam() -> bytes: + """Track 18 sector 0. Only the fields the DOS reads to learn the disk.""" + block = bytearray(256) + block[0:2] = bytes((DOS_TRACK, 1)) + block[2] = 0x41 # 'A', DOS version + for track in range(1, 36): + _, sectors, _ = zone_of(track) + entry = 4 + (track - 1) * 4 + block[entry] = 0 if track == DOS_TRACK else sectors + block[entry + 1:entry + 4] = b"\xff\xff\x1f" + block[0x90:0xa2] = b"\xa0" * 18 + block[0x90:0x99] = b"MFM BOUNDS"[:9] + block[0xa2:0xa4] = DISK_ID + block[0xa4] = 0xa0 + block[0xa5:0xa7] = b"2A" + block[0xa7:0xab] = b"\xa0" * 4 + return bytes(block) + + +def sector_payload(track: int, sector: int) -> bytes: + if track == DOS_TRACK and sector == 0: + return bam() + if track == DOS_TRACK and sector == SENTINEL_SECTOR: + return SENTINEL + return bytes((track, sector)) + bytes(254) + + +def mfm_track(sectors: int = MFM_SECTORS) -> bytes: + """A well-formed MFM payload, laid out as the firmware lays it out.""" + header = bytearray(MFM_HEADER_SIZE) + header[0] = sectors + header[1] = 0 # version + for index in range(sectors): + entry = 2 + index * 5 + header[entry:entry + 5] = bytes((40, 0, index + 1, MFM_SIZE_CODE, 0)) + data = bytearray() + for index in range(sectors): + data += bytes((index + 1,)) * MFM_SECTOR_BYTES + return bytes(header) + bytes(data) + + +def g71(tracks: dict) -> bytes: + """`tracks` maps a half-track index to (declared word, track bytes). + + Tracks that fit the header's maximum go into fixed slots behind the + tables, the way a G64 is normally laid out. One that does not is appended + behind them, which the format allows outright: "The location of the actual + track or speed zone data is not important." + """ + image = bytearray(TABLES) + image[0:8] = SIGNATURE + image[8] = 0 # G64 version + image[9] = HALF_TRACKS + image[10:12] = MAX_TRACK_SIZE.to_bytes(2, "little") + + offsets = {} + inline = sorted(index for index, (_, data) in tracks.items() + if len(data) <= MAX_TRACK_SIZE) + for slot, index in enumerate(inline): + offsets[index] = TABLES + slot * SLOT + image += bytearray(len(inline) * SLOT) + + for index in sorted(tracks): + if index not in offsets: + offsets[index] = len(image) + image += bytearray(2 + len(tracks[index][1])) + + for index, (declared, data) in tracks.items(): + at = offsets[index] + image[at:at + 2] = declared.to_bytes(2, "little") + image[at + 2:at + 2 + len(data)] = data + + speeds = 12 + HALF_TRACKS * 4 + for index in tracks: + at = 12 + index * 4 + image[at:at + 4] = offsets[index].to_bytes(4, "little") + track = index // 2 + 1 + zone = zone_of(track)[0] if index % 2 == 0 and track <= 35 else 0 + at = speeds + index * 4 + image[at:at + 4] = zone.to_bytes(4, "little") + return bytes(image) + + +def fixtures() -> dict: + """The three images, each built from the same GCR side 0.""" + gcr = {(track - 1) * 2: gcr_track(track, sector_payload) for track in range(1, 36)} + common = {index: (len(data), data) for index, data in gcr.items()} + + payload = mfm_track() + well_formed = dict(common) + well_formed[MFM_HALF_TRACK] = (MFM_MARKER | len(payload), payload) + + short = dict(common) + short[MFM_HALF_TRACK] = (MFM_MARKER | 1, b"\x00") + + overlong = dict(common) + overlong[OVERLONG_HALF_TRACK] = (0x4123, bytes(0x4123 & 0x7fff)) + + return {"mixed": well_formed, "shortmfm": short, "overlong": overlong} + + +CASES = ("mixed", "shortmfm", "overlong") + +RESULT_STATUS = 0xc000 +RESULT_BYTES = 4 +RESULT_DATA = 0xc100 +STATUS_DONE = 0x01 +READY_MARK = 0xa5 +PROGRAM_TIMEOUT_SECONDS = 60.0 +POLL_SECONDS = 0.5 +LIVENESS_TIMEOUT_SECONDS = 20.0 + + +def mounted_path(drive: DriveInfo) -> str: + if drive.image_file.startswith("/"): + return drive.image_file + return posixpath.join(drive.image_path, drive.image_file) if drive.image_file else "" + + +class SuiteRunner: + def __init__(self, args: argparse.Namespace) -> None: + self.args = args + self.api = UltimateApi(args.host, args.password or None, args.timeout) + self.slot = "" + self.original: DriveInfo | None = None + self.paths: dict[str, str] = {} + + def prepare(self) -> bytes: + drives = self.api.drives.list() + self.slot = "b" if "b" in drives else "a" + if self.slot not in drives: + raise Failure("the device exposes neither drive a nor drive b") + self.original = drives[self.slot] + if self.original.bus_id is None: + raise Failure(f"drive {self.slot} has no IEC bus ID") + + self.api.drives.set_mode(self.slot, "1571") + self.api.drives.on(self.slot) + images = fixtures() + with ftp_lib.session(self.args.host, self.args.password or None) as client: + for name in CASES: + path = f"/Temp/g71-mfm-{name}.g71" + ftp_lib.store(client, path, g71(images[name])) + self.paths[name] = path + return assemble(SOURCE, {"DEVICE": self.original.bus_id, + "TRACK": DOS_TRACK, "SECTOR": SENTINEL_SECTOR}) + + def await_program(self) -> bytes: + deadline = time.monotonic() + PROGRAM_TIMEOUT_SECONDS + while True: + result = self.api.machine.readmem(RESULT_STATUS, RESULT_BYTES) + if result[1] == READY_MARK and result[0] != 0: + return result + if time.monotonic() >= deadline: + raise Failure(f"block reader did not finish: result={result.hex(' ')}") + time.sleep(POLL_SECONDS) + + def run_case(self, name: str, prg: bytes) -> None: + path = self.paths[name] + section(name) + + with check(f"mount the {name} G71 on drive {self.slot}"): + self.api.drives.mount(self.slot, path, type="g71", mode="readonly") + mounted = self.api.drives.get(self.slot) + if posixpath.basename(path) not in mounted.image_file: + raise Failure(f"drive reports {mounted.image_file!r}, expected {path!r}") + + with check("device remains reachable after mount"): + reason = self.api.unreachable_reason(LIVENESS_TIMEOUT_SECONDS) + if reason: + raise Failure(reason) + + with check(f"track {DOS_TRACK} sector {SENTINEL_SECTOR} still reads"): + self.api.machine.writemem(RESULT_STATUS, bytes(RESULT_BYTES), idempotent=True) + self.api.machine.writemem(RESULT_DATA, bytes(256), idempotent=True) + status, _, body = self.api.runners.upload("run_prg", prg) + if status != 200: + raise Failure(f"run_prg returned HTTP {status}: {body[:160]!r}") + result = self.await_program() + data = self.api.machine.readmem(RESULT_DATA, 256) + detail(f"dos=${result[2]:02x}, io=${result[3]:02x}") + if result[0] != STATUS_DONE: + raise Failure(f"reader failed: result={result.hex(' ')}") + if data != SENTINEL: + raise Failure(f"sentinel came back as {data[:8].hex(' ')}...") + + def cleanup(self) -> None: + try: + if self.slot: + self.api.drives.remove(self.slot) + except Exception: + pass + try: + with ftp_lib.session(self.args.host, self.args.password or None) as client: + for path in self.paths.values(): + try: + ftp_lib.delete(client, path) + except Exception: + pass + except Exception: + pass + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + cli.add_device_arguments(parser, timeout=15.0, colour=False) + args = parser.parse_args() + runner = SuiteRunner(args) + try: + prg = runner.prepare() + for name in CASES: + runner.run_case(name, prg) + except Failure as failure: + suite_fail(SUITE, str(failure)) + return 1 + except Exception as exc: # noqa: BLE001 - the harness reports, it does not raise + suite_fail(SUITE, format_exception(exc)) + return 1 + finally: + runner.cleanup() + return suite_ok(SUITE) + + +if __name__ == "__main__": + sys.exit(main()) From de633b34b666c5263bc854156e03d57df27b90e6 Mon Sep 17 00:00:00 2001 From: Enver Haase-Beer Date: Sun, 4 Oct 2026 02:03:10 +0200 Subject: [PATCH 7/7] Send the G71 block reader's DOS commands in upper case, initialise first The block reader sent "i0" and "u1 2 0 " in lower case. 64tass passes .text through as ASCII, $69 and $75, and the DOS knows its commands only as the PETSCII $49 "I" and $55 "U": both came back as 31, SYNTAX ERROR, and the suite failed on master and on this branch alike with dos=$31. In upper case the initialise then freed the buffer channel opened just before it, and U1 answered 70, NO CHANNEL. The initialise now comes before "#" is opened. On an Ultimate II+L in a C64, master 7117d885: 9/9, the three G71s read track 18 sector 1 back. --- tests/e2e/drive/g71_block_reader.asm | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/e2e/drive/g71_block_reader.asm b/tests/e2e/drive/g71_block_reader.asm index e9c2cefc2..65ac09b49 100644 --- a/tests/e2e/drive/g71_block_reader.asm +++ b/tests/e2e/drive/g71_block_reader.asm @@ -54,6 +54,9 @@ start: jsr OPEN bcs io_error + ; initialise first: it frees every buffer, so a "#" opened before it is gone + jsr init_drive + ; buffer channel on "#" lda #2 ldx #DEVICE @@ -66,7 +69,6 @@ start: jsr OPEN bcs io_error - jsr init_drive jsr send_u1 jsr read_block jsr read_error_channel @@ -164,10 +166,12 @@ read_error_channel: buffer_name: .text "#" +; Upper case: 64tass passes .text through as ASCII, and the DOS knows its +; commands only as the PETSCII $49 "I" and $55 "U"; "i0" is error 31. init_command: - .text "i0" + .text "I0" init_command_end: u1_command: - .text "u1 2 0 ", format("%d", TRACK), " ", format("%d", SECTOR) + .text "U1 2 0 ", format("%d", TRACK), " ", format("%d", SECTOR) u1_command_end: