Merge pull request #778 from TomHarte/AppleIIDisks

Resolves a potential crash with NIB files

Analyser/Static/DiskII/StaticAnalyser.cpp

@@ -20,7 +20,7 @@ namespace {
 
 Analyser::Static::Target *AppleTarget(const Storage::Encodings::AppleGCR::Sector *sector_zero) {
 	using Target = Analyser::Static::AppleII::Target;
-	auto *target = new Target;
+	auto *const target = new Target;
 
 	if(sector_zero && sector_zero->encoding == Storage::Encodings::AppleGCR::Sector::Encoding::FiveAndThree) {
 		target->disk_controller = Target::DiskController::ThirteenSector;
@@ -33,7 +33,7 @@ Analyser::Static::Target *AppleTarget(const Storage::Encodings::AppleGCR::Sector
 
 Analyser::Static::Target *OricTarget(const Storage::Encodings::AppleGCR::Sector *sector_zero) {
 	using Target = Analyser::Static::Oric::Target;
-	auto *target = new Target;
+	auto *const target = new Target;
 	target->rom = Target::ROM::Pravetz;
 	target->disk_interface = Target::DiskInterface::Pravetz;
 	target->loading_command = "CALL 800\n";
@@ -47,8 +47,8 @@ Analyser::Static::TargetList Analyser::Static::DiskII::GetTargets(const Media &m
 	if(media.disks.empty()) return {};
 
 	// Grab track 0, sector 0: the boot sector.
-	auto track_zero = media.disks.front()->get_track_at_position(Storage::Disk::Track::Address(0, Storage::Disk::HeadPosition(0)));
-	auto sector_map = Storage::Encodings::AppleGCR::sectors_from_segment(
+	const auto track_zero = media.disks.front()->get_track_at_position(Storage::Disk::Track::Address(0, Storage::Disk::HeadPosition(0)));
+	const auto sector_map = Storage::Encodings::AppleGCR::sectors_from_segment(
 		Storage::Disk::track_serialisation(*track_zero, Storage::Time(1, 50000)));
 
 	const Storage::Encodings::AppleGCR::Sector *sector_zero = nullptr;
@@ -75,7 +75,7 @@ Analyser::Static::TargetList Analyser::Static::DiskII::GetTargets(const Media &m
 	// If the boot sector looks like it's intended for the Oric, create an Oric.
 	// Otherwise go with the Apple II.
 
-	auto disassembly = Analyser::Static::MOS6502::Disassemble(sector_zero->data, Analyser::Static::Disassembler::OffsetMapper(0xb800), {0xb800});
+	const auto disassembly = Analyser::Static::MOS6502::Disassemble(sector_zero->data, Analyser::Static::Disassembler::OffsetMapper(0xb800), {0xb800});
 
 	bool did_read_shift_register = false;
 	bool is_oric = false;

OSBindings/Mac/Clock Signal.xcodeproj/xcshareddata/xcschemes/Clock Signal.xcscheme

@@ -67,7 +67,7 @@
       </Testables>
    </TestAction>
    <LaunchAction
-      buildConfiguration = "Release"
+      buildConfiguration = "Debug"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       enableASanStackUseAfterReturn = "YES"

Storage/Disk/DiskImage/Formats/NIB.cpp

@@ -12,6 +12,9 @@
 #include "../../Track/TrackSerialiser.hpp"
 #include "../../Encodings/AppleGCR/Encoder.hpp"
 
+#include "../../Encodings/AppleGCR/Encoder.hpp"
+#include "../../Encodings/AppleGCR/SegmentParser.hpp"
+
 #include <vector>
 
 using namespace Storage::Disk;
@@ -48,7 +51,7 @@ bool NIB::get_is_read_only() {
 }
 
 long NIB::file_offset(Track::Address address) {
-	return static_cast<long>(address.position.as_int()) * track_length;
+	return long(address.position.as_int()) * track_length;
 }
 
 std::shared_ptr<::Storage::Disk::Track> NIB::get_track_at_position(::Storage::Disk::Track::Address address) {
@@ -84,7 +87,10 @@ std::shared_ptr<::Storage::Disk::Track> NIB::get_track_at_position(::Storage::Di
 				++length;
 			}
 
-			// Record a sync position only if there were at least five FFs.
+			// Record a sync position only if there were at least five FFs, and
+			// sync only in the final five. One of the many crazy fictions of NIBs
+			// is the fixed track length in bytes, which is quite long. So the aim
+			// is to be as conservative as possible with sync placement.
 			if(length == 5) {
 				sync_starts.insert((start + 1) % track_data.size());
 
@@ -101,16 +107,21 @@ std::shared_ptr<::Storage::Disk::Track> NIB::get_track_at_position(::Storage::Di
 
 	// If the track started in a sync block, write sync first.
 	if(start_index) {
-		segment += Encodings::AppleGCR::six_and_two_sync(static_cast<int>(start_index));
+		segment += Encodings::AppleGCR::six_and_two_sync(int(start_index));
 	}
 
 	std::size_t index = start_index;
 	for(const auto location: sync_starts) {
 		// Write data from index to sync_start.
+		if(location > index) {
+			// This is the usual case; the only occasion on which it won't be true is
+			// when the initial sync was detected to carry over the index hole,
+			// in which case there's nothing to copy.
 			std::vector<uint8_t> data_segment(
-			track_data.begin() + static_cast<off_t>(index),
-			track_data.begin() + static_cast<off_t>(location));
+				track_data.begin() + off_t(index),
+				track_data.begin() + off_t(location));
 			segment += PCMSegment(data_segment);
+		}
 
 		// Add a sync from sync_start to end of 0xffs, if there are
 		// any before the end of data.
@@ -119,13 +130,15 @@ std::shared_ptr<::Storage::Disk::Track> NIB::get_track_at_position(::Storage::Di
 			++index;
 
 		if(index - location)
-			segment += Encodings::AppleGCR::six_and_two_sync(static_cast<int>(index - location));
+			segment += Encodings::AppleGCR::six_and_two_sync(int(index - location));
 	}
 
-	// If there's still data remaining on the track, write it out.
+	// If there's still data remaining on the track, write it out. If a sync ran over
+	// the notional index hole, the loop above will already have completed the track
+	// with sync, so no need to deal with that case here.
 	if(index < track_length) {
 		std::vector<uint8_t> data_segment(
-			track_data.begin() + static_cast<off_t>(index),
+			track_data.begin() + off_t(index),
 			track_data.end());
 		segment += PCMSegment(data_segment);
 	}
@@ -149,7 +162,7 @@ void NIB::set_tracks(const std::map<Track::Address, std::shared_ptr<Track>> &tra
 		int bit_count = 0;
 		size_t sync_location = 0, location = 0;
 		for(const auto bit: segment.data) {
-			shifter = static_cast<uint8_t>((shifter << 1) | (bit ? 1 : 0));
+			shifter = uint8_t((shifter << 1) | (bit ? 1 : 0));
 			++bit_count;
 			++location;
 			if(shifter & 0x80) {
@@ -167,8 +180,8 @@ void NIB::set_tracks(const std::map<Track::Address, std::shared_ptr<Track>> &tra
 			track.resize(track_length);
 		} else {
 			while(track.size() < track_length) {
-				std::vector<uint8_t> extra_data(static_cast<size_t>(track_length) - track.size(), 0xff);
-				track.insert(track.begin() + static_cast<off_t>(sync_location), extra_data.begin(), extra_data.end());
+				std::vector<uint8_t> extra_data(size_t(track_length) - track.size(), 0xff);
+				track.insert(track.begin() + off_t(sync_location), extra_data.begin(), extra_data.end());
 			}
 		}
 

Storage/Disk/Encodings/AppleGCR/Encoder.hpp

@@ -16,12 +16,14 @@ namespace Storage {
 namespace Encodings {
 namespace AppleGCR {
 
-/// Describes the standard three-byte prologue that begins a header.
-const uint8_t header_prologue[3] = {0xd5, 0xaa, 0x96};
+/// Describes the standard three-byte prologue that begins a header on both the Macintosh and the Apple II from DOS 3.3.
+constexpr uint8_t header_prologue[3] = {0xd5, 0xaa, 0x96};
 /// Describes the standard three-byte prologue that begins a data section.
-const uint8_t data_prologue[3] = {0xd5, 0xaa, 0xad};
+constexpr uint8_t data_prologue[3] = {0xd5, 0xaa, 0xad};
 /// Describes the epilogue that ends both data sections and headers.
-const uint8_t epilogue[3] = {0xde, 0xaa, 0xeb};
+constexpr uint8_t epilogue[3] = {0xde, 0xaa, 0xeb};
+/// Describes the standard three-byte prologue used by DOS 3.2 and earlier.
+constexpr uint8_t five_and_three_header_prologue[3] = {0xd5, 0xaa, 0xb5};
 
 namespace AppleII {
 

Storage/Disk/Encodings/AppleGCR/SegmentParser.cpp

@@ -36,6 +36,25 @@ uint8_t unmap_six_and_two(uint8_t source) {
 	return six_and_two_unmapping[source - 0x96];
 }
 
+const uint8_t five_and_three_unmapping[] = {
+	/* 0xab */	0x00, 0xff,	0x01, 0x02, 0x03,
+	/* 0xb0 */	0xff, 0xff, 0xff, 0xff, 0xff, 0x04, 0x05, 0x06,
+	/* 0xb8 */	0xff, 0xff, 0x07, 0x08, 0xff, 0x09, 0x0a, 0x0b,
+	/* 0xc0 */	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+	/* 0xc8 */	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+	/* 0xd0 */	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x0c, 0x0d,
+	/* 0xd8 */	0xff, 0xff, 0x0e, 0x0f, 0xff, 0x10, 0x11, 0x12,
+	/* 0xe0 */	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+	/* 0xe8 */	0xff, 0xff,	0x13, 0x14, 0xff, 0x15, 0x16, 0x17,
+	/* 0xf0 */	0xff, 0xff,	0xff, 0xff, 0xff, 0x18, 0x19, 0x1a,
+	/* 0xf8 */	0xff, 0xff, 0x1b, 0x1c, 0xff, 0x1d, 0x1e, 0x1f,
+};
+
+uint8_t unmap_five_and_three(uint8_t source) {
+	if(source < 0xab) return 0xff;
+	return five_and_three_unmapping[source - 0xab];
+}
+
 }
 
 using namespace Storage::Encodings::AppleGCR;
@@ -56,6 +75,7 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 	// sector is still being parsed.
 	size_t bit = 0;
 	int header_delay = 0;
+	bool is_five_and_three = false;
 	while(bit < segment.data.size() || pointer != scanning_sentinel || header_delay) {
 		shift_register = static_cast<uint_fast8_t>((shift_register << 1) | (segment.data[bit % segment.data.size()] ? 1 : 0));
 		++bit;
@@ -77,11 +97,16 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 				scanner[0] == header_prologue[0] &&
 				scanner[1] == header_prologue[1] &&
 				(
+					scanner[2] == five_and_three_header_prologue[2] ||
 					scanner[2] == header_prologue[2] ||
 					scanner[2] == data_prologue[2]
 				)) {
 				pointer = 0;
 
+				if(scanner[2] != data_prologue[2]) {
+					is_five_and_three = scanner[2] == five_and_three_header_prologue[2];
+				}
+
 				// If this is the start of a data section, and at least
 				// one header has been witnessed, start a sector.
 				if(scanner[2] == data_prologue[2]) {
@@ -97,7 +122,8 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 			if(new_sector) {
 				// Check whether the value just read is a legal GCR byte, in six-and-two
 				// encoding (which is a strict superset of five-and-three).
-				if(unmap_six_and_two(value) == 0xff) {
+				const bool is_invalid = is_five_and_three ? (unmap_five_and_three(value) == 0xff) : (unmap_six_and_two(value) == 0xff);
+				if(is_invalid) {
 					// The second byte of the standard epilogue is illegal, so this still may
 					// be a valid sector. If the final byte was the first byte of an epilogue,
 					// chop it off and see whether the sector is otherwise intelligible.
@@ -123,8 +149,7 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 						default:	// This is not a decodeable sector.
 						break;
 
-						// TODO: check for five-and-three / 13-sector form sectors.
-
+						case 411:	// Potentially this is an Apple II five-and-three sector.
 						case 343: {	// Potentially this is an Apple II six-and-two sector.
 							// Check for apparent four and four encoding.
 							uint_fast8_t header_mask = 0xff;
@@ -137,13 +162,15 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 							sector->address.sector = ((header[4] << 1) | 1) & header[5];
 
 							// Check the header checksum.
-							uint_fast8_t checksum = ((header[6] << 1) | 1) & header[7];
+							// The 0x11 is reverse engineered from the game 'Alien Rain' and is present even on the boot sector,
+							// so probably isn't copy protection?
+							uint_fast8_t checksum = (((header[6] << 1) | 1) & header[7]) ^ (is_five_and_three ? 0x11 : 0x00);
 							if(checksum != (sector->address.volume^sector->address.track^sector->address.sector)) continue;
 
-							// Unmap the sector contents as 6 and 2 data.
+							// Unmap the sector contents.
 							bool out_of_bounds = false;
 							for(auto &c : sector->data) {
-								c = unmap_six_and_two(c);
+								c = is_five_and_three ? unmap_five_and_three(c) : unmap_six_and_two(c);
 								if(c == 0xff) {
 									out_of_bounds = true;
 									break;
@@ -160,6 +187,27 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 							// Having checked the checksum, remove it.
 							sector->data.resize(sector->data.size() - 1);
 
+							if(is_five_and_three) {
+								// TODO: the above is almost certainly incorrect; Beneath Apple DOS partly documents
+								// the process, enough to give the basic outline below of how five source bytes are
+								// mapped to eight five-bit quantities, but isn't clear on the order those bytes will
+								// end up in on disk.
+
+								std::vector<uint8_t> buffer(256);
+								for(size_t c = 0; c < 0x33; ++c) {
+									const uint8_t *const base = &sector->data[0x032 - c];
+
+									buffer[(c * 5) + 0] = uint8_t((base[0x000] << 3) | (base[0x100] >> 2));
+									buffer[(c * 5) + 1] = uint8_t((base[0x033] << 3) | (base[0x133] >> 2));
+									buffer[(c * 5) + 2] = uint8_t((base[0x066] << 3) | (base[0x166] >> 2));
+									buffer[(c * 5) + 3] = uint8_t((base[0x099] << 3) | ((base[0x100] & 2) << 1) | (base[0x133] & 2) | ((base[0x166] & 2) >> 1));
+									buffer[(c * 5) + 4] = uint8_t((base[0x0cc] << 3) | ((base[0x100] & 1) << 2) | ((base[0x133] & 1) << 1) | (base[0x166] & 1));
+								}
+								buffer[255] = uint8_t((sector->data[0x0ff] << 3) | (sector->data[0x199] >> 2));
+
+								sector->data = std::move(buffer);
+								sector->encoding = Sector::Encoding::FiveAndThree;
+							} else {
 								// Undo the 6 and 2 mapping.
 								const uint8_t bit_reverse[] = {0, 2, 1, 3};
 								#define unmap(byte, nibble, shift)	\
@@ -182,8 +230,9 @@ std::map<std::size_t, Sector> Storage::Encodings::AppleGCR::sectors_from_segment
 								// Throw away the collection of two-bit chunks from the start of the sector.
 								sector->data.erase(sector->data.begin(), sector->data.end() - 256);
 
-							// Add this sector to the map.
 								sector->encoding = Sector::Encoding::SixAndTwo;
+							}
+							// Add this sector to the map.
 							result.insert(std::make_pair(sector_location, std::move(*sector)));
 						} break;