From 0e8a17ce82260230b77fa8abe5a3fe064f2da797 Mon Sep 17 00:00:00 2001 From: Sylvie <35663410+Rangi42@users.noreply.github.com> Date: Thu, 22 Aug 2024 12:51:52 -0400 Subject: [PATCH] Report any section overflows at the end of assembly (#1482) * Report any section overflows at the end of assembly * Immediately handle overflow of the 32-bit size counter --- include/asm/section.hpp | 2 + src/asm/main.cpp | 1 + src/asm/section.cpp | 149 +++++++++---------------- test/asm/incbin-empty-bad.err | 2 +- test/asm/incbin-end-bad.err | 2 +- test/asm/load-overflow.asm | 22 ++-- test/asm/load-overflow.err | 24 ++-- test/asm/section-unsigned-overflow.asm | 9 ++ test/asm/section-unsigned-overflow.err | 2 + 9 files changed, 94 insertions(+), 119 deletions(-) create mode 100644 test/asm/section-unsigned-overflow.asm create mode 100644 test/asm/section-unsigned-overflow.err diff --git a/include/asm/section.hpp b/include/asm/section.hpp index 66eea114..1b71ac6f 100644 --- a/include/asm/section.hpp +++ b/include/asm/section.hpp @@ -78,6 +78,8 @@ uint32_t sect_GetOutputOffset(); uint32_t sect_GetAlignBytes(uint8_t alignment, uint16_t offset); void sect_AlignPC(uint8_t alignment, uint16_t offset); +void sect_CheckSizes(); + void sect_StartUnion(); void sect_NextUnionMember(); void sect_EndUnion(); diff --git a/src/asm/main.cpp b/src/asm/main.cpp index 73ad4e1c..5738d48d 100644 --- a/src/asm/main.cpp +++ b/src/asm/main.cpp @@ -381,6 +381,7 @@ int main(int argc, char *argv[]) { nbErrors = 1; sect_CheckUnionClosed(); + sect_CheckSizes(); if (nbErrors != 0) errx("Assembly aborted (%u error%s)!", nbErrors, nbErrors == 1 ? "" : "s"); diff --git a/src/asm/section.cpp b/src/asm/section.cpp index 3fdd9496..4f03261b 100644 --- a/src/asm/section.cpp +++ b/src/asm/section.cpp @@ -72,41 +72,17 @@ int32_t loadOffset; // Offset into the LOAD section's parent (see sect_GetOutput return false; } -[[nodiscard]] static bool checkSectionSize(Section const §, uint32_t size) { - uint32_t maxSize = sectionTypeInfo[sect.type].size; - - // If the new size is reasonable, keep going - if (size <= maxSize) - return true; - - error( - "Section '%s' grew too big (max size = 0x%" PRIX32 " bytes, reached 0x%" PRIX32 ").\n", - sect.name.c_str(), - maxSize, - size - ); - return false; -} - -// Check if the section has grown too much. -[[nodiscard]] static bool reserveSpace(uint32_t delta_size) { - // This check is here to trap broken code that generates sections that are too big and to - // prevent the assembler from generating huge object files or trying to allocate too much - // memory. - // A check at the linking stage is still necessary. - - // If the section has already overflowed, skip the check to avoid erroring out ad nauseam - if (currentSection->size != UINT32_MAX - && !checkSectionSize(*currentSection, curOffset + loadOffset + delta_size)) - // Mark the section as overflowed, to avoid repeating the error - currentSection->size = UINT32_MAX; - - if (currentLoadSection && currentLoadSection->size != UINT32_MAX - && !checkSectionSize(*currentLoadSection, curOffset + delta_size)) - currentLoadSection->size = UINT32_MAX; - - return currentSection->size != UINT32_MAX - && (!currentLoadSection || currentLoadSection->size != UINT32_MAX); +void sect_CheckSizes() { + for (Section const § : sectionList) { + if (uint32_t maxSize = sectionTypeInfo[sect.type].size; sect.size > maxSize) + error( + "Section '%s' grew too big (max size = 0x%" PRIX32 " bytes, reached 0x%" PRIX32 + ").\n", + sect.name.c_str(), + maxSize, + sect.size + ); + } } Section *sect_FindSectionByName(std::string const &name) { @@ -583,6 +559,8 @@ void sect_AlignPC(uint8_t alignment, uint16_t offset) { } static void growSection(uint32_t growth) { + if (growth > 0 && curOffset > UINT32_MAX - growth) + fatalerror("Section size would overflow internal counter\n"); curOffset += growth; if (curOffset + loadOffset > currentSection->size) currentSection->size = curOffset + loadOffset; @@ -591,7 +569,8 @@ static void growSection(uint32_t growth) { } static void writebyte(uint8_t byte) { - currentSection->data[sect_GetOutputOffset()] = byte; + if (uint32_t index = sect_GetOutputOffset(); index < currentSection->data.size()) + currentSection->data[index] = byte; growSection(1); } @@ -665,8 +644,6 @@ void sect_CheckUnionClosed() { void sect_AbsByte(uint8_t b) { if (!checkcodesection()) return; - if (!reserveSpace(1)) - return; writebyte(b); } @@ -674,8 +651,6 @@ void sect_AbsByte(uint8_t b) { void sect_AbsByteString(std::vector const &s) { if (!checkcodesection()) return; - if (!reserveSpace(s.size())) - return; for (int32_t v : s) { if (!checkNBit(v, 8, "All character units")) @@ -689,8 +664,6 @@ void sect_AbsByteString(std::vector const &s) { void sect_AbsWordString(std::vector const &s) { if (!checkcodesection()) return; - if (!reserveSpace(s.size() * 2)) - return; for (int32_t v : s) { if (!checkNBit(v, 16, "All character units")) @@ -704,8 +677,6 @@ void sect_AbsWordString(std::vector const &s) { void sect_AbsLongString(std::vector const &s) { if (!checkcodesection()) return; - if (!reserveSpace(s.size() * 4)) - return; for (int32_t v : s) writelong(static_cast(v)); @@ -715,8 +686,6 @@ void sect_AbsLongString(std::vector const &s) { void sect_Skip(uint32_t skip, bool ds) { if (!checksection()) return; - if (!reserveSpace(skip)) - return; if (!sect_HasData(currentSection->type)) { growSection(skip); @@ -740,8 +709,6 @@ void sect_Skip(uint32_t skip, bool ds) { void sect_RelByte(Expression &expr, uint32_t pcShift) { if (!checkcodesection()) return; - if (!reserveSpace(1)) - return; if (!expr.isKnown()) { createPatch(PATCHTYPE_BYTE, expr, pcShift); @@ -756,8 +723,6 @@ void sect_RelByte(Expression &expr, uint32_t pcShift) { void sect_RelBytes(uint32_t n, std::vector &exprs) { if (!checkcodesection()) return; - if (!reserveSpace(n)) - return; for (uint32_t i = 0; i < n; i++) { Expression &expr = exprs[i % exprs.size()]; @@ -776,8 +741,6 @@ void sect_RelBytes(uint32_t n, std::vector &exprs) { void sect_RelWord(Expression &expr, uint32_t pcShift) { if (!checkcodesection()) return; - if (!reserveSpace(2)) - return; if (!expr.isKnown()) { createPatch(PATCHTYPE_WORD, expr, pcShift); @@ -792,8 +755,6 @@ void sect_RelWord(Expression &expr, uint32_t pcShift) { void sect_RelLong(Expression &expr, uint32_t pcShift) { if (!checkcodesection()) return; - if (!reserveSpace(2)) - return; if (!expr.isKnown()) { createPatch(PATCHTYPE_LONG, expr, pcShift); @@ -808,11 +769,8 @@ void sect_RelLong(Expression &expr, uint32_t pcShift) { void sect_PCRelByte(Expression &expr, uint32_t pcShift) { if (!checkcodesection()) return; - if (!reserveSpace(1)) - return; - Symbol const *pc = sym_GetPC(); - if (!expr.isDiffConstant(pc)) { + if (Symbol const *pc = sym_GetPC(); !expr.isDiffConstant(pc)) { createPatch(PATCHTYPE_JR, expr, pcShift); writebyte(0); } else { @@ -863,35 +821,32 @@ void sect_BinaryFile(std::string const &name, int32_t startPos) { } Defer closeFile{[&] { fclose(file); }}; - int32_t fsize = -1; - if (fseek(file, 0, SEEK_END) != -1) { - fsize = ftell(file); - - if (startPos > fsize) { - error("Specified start position is greater than length of file\n"); + if (startPos > ftell(file)) { + error("Specified start position is greater than length of file '%s'\n", name.c_str()); return; } - + // The file is seekable; skip to the specified start position fseek(file, startPos, SEEK_SET); - - if (!reserveSpace(fsize - startPos)) - return; } else { if (errno != ESPIPE) error( "Error determining size of INCBIN file '%s': %s\n", name.c_str(), strerror(errno) ); - // The file isn't seekable, so we'll just skip bytes - while (startPos--) - (void)fgetc(file); + // The file isn't seekable, so we'll just skip bytes one at a time + while (startPos--) { + if (fgetc(file) == EOF) { + error( + "Specified start position is greater than length of file '%s'\n", + name.c_str() + ); + return; + } + } } - for (int byte; (byte = fgetc(file)) != EOF;) { - if (fsize == -1) - growSection(1); + for (int byte; (byte = fgetc(file)) != EOF;) writebyte(byte); - } if (ferror(file)) error("Error reading INCBIN file '%s': %s\n", name.c_str(), strerror(errno)); @@ -903,18 +858,14 @@ void sect_BinaryFileSlice(std::string const &name, int32_t startPos, int32_t len error("Start position cannot be negative (%" PRId32 ")\n", startPos); startPos = 0; } - if (length < 0) { error("Number of bytes to read cannot be negative (%" PRId32 ")\n", length); length = 0; } - if (!checkcodesection()) return; if (length == 0) // Don't even bother with 0-byte slices return; - if (!reserveSpace(length)) - return; FILE *file = nullptr; if (std::optional fullPath = fstk_FindFile(name); fullPath) @@ -932,44 +883,50 @@ void sect_BinaryFileSlice(std::string const &name, int32_t startPos, int32_t len Defer closeFile{[&] { fclose(file); }}; if (fseek(file, 0, SEEK_END) != -1) { - int32_t fsize = ftell(file); - - if (startPos > fsize) { - error("Specified start position is greater than length of file\n"); + if (int32_t fsize = ftell(file); startPos > fsize) { + error("Specified start position is greater than length of file '%s'\n", name.c_str()); return; - } - - if ((startPos + length) > fsize) { + } else if (startPos + length > fsize) { error( - "Specified range in INCBIN is out of bounds (%" PRIu32 " + %" PRIu32 " > %" PRIu32 - ")\n", + "Specified range in INCBIN file '%s' is out of bounds (%" PRIu32 " + %" PRIu32 + " > %" PRIu32 ")\n", + name.c_str(), startPos, length, fsize ); return; } - + // The file is seekable; skip to the specified start position fseek(file, startPos, SEEK_SET); } else { if (errno != ESPIPE) error( "Error determining size of INCBIN file '%s': %s\n", name.c_str(), strerror(errno) ); - // The file isn't seekable, so we'll just skip bytes - while (startPos--) - (void)fgetc(file); + // The file isn't seekable, so we'll just skip bytes one at a time + while (startPos--) { + if (fgetc(file) == EOF) { + error( + "Specified start position is greater than length of file '%s'\n", + name.c_str() + ); + return; + } + } } while (length--) { - int byte = fgetc(file); - - if (byte != EOF) { + if (int byte = fgetc(file); byte != EOF) { writebyte(byte); } else if (ferror(file)) { error("Error reading INCBIN file '%s': %s\n", name.c_str(), strerror(errno)); } else { - error("Premature end of file (%" PRId32 " bytes left to read)\n", length + 1); + error( + "Premature end of INCBIN file '%s' (%" PRId32 " bytes left to read)\n", + name.c_str(), + length + 1 + ); } } } diff --git a/test/asm/incbin-empty-bad.err b/test/asm/incbin-empty-bad.err index 65bad213..f23c9f2c 100644 --- a/test/asm/incbin-empty-bad.err +++ b/test/asm/incbin-empty-bad.err @@ -1,3 +1,3 @@ error: incbin-empty-bad.asm(3): - Specified range in INCBIN is out of bounds (0 + 1 > 0) + Specified range in INCBIN file 'empty.bin' is out of bounds (0 + 1 > 0) error: Assembly aborted (1 error)! diff --git a/test/asm/incbin-end-bad.err b/test/asm/incbin-end-bad.err index 3f29b02f..c9b4add6 100644 --- a/test/asm/incbin-end-bad.err +++ b/test/asm/incbin-end-bad.err @@ -1,3 +1,3 @@ error: incbin-end-bad.asm(3): - Specified range in INCBIN is out of bounds (123 + 1 > 123) + Specified range in INCBIN file 'data.bin' is out of bounds (123 + 1 > 123) error: Assembly aborted (1 error)! diff --git a/test/asm/load-overflow.asm b/test/asm/load-overflow.asm index c3fe7f11..77976268 100644 --- a/test/asm/load-overflow.asm +++ b/test/asm/load-overflow.asm @@ -1,25 +1,25 @@ SECTION "Overflow", ROM0 ds $6000 LOAD "oops",WRAM0 - ; We might get an error for "oops", but it can also make sense to no-op the directive - ds $2001 - ; We shouldn't get any more errors for "Overflow" nor "oops" + ds $2000 + db db ENDL SECTION "Moar overflow", ROM0 - ds $6001 + ds $4000 + ds $4000 LOAD "hmm", WRAM0 ds $2000 - ; Since the `ds` overflows "Moar overflow", it could be no-op'd, making this `db` not error - db + ds $2000 ENDL + ds $1000 SECTION "Not overflowing", ROM0 - ds $5FFF + ds $800 LOAD "lol", WRAM0 - ds $2000 - db - ; Since the LOAD block is overflowed, this may be no-op'd, not affecting the "parent" + ds $1000 + ds $1000 + ds $1000 ENDL - dw ; This, however... + ds $800 diff --git a/test/asm/load-overflow.err b/test/asm/load-overflow.err index 9a2662be..53ab0582 100644 --- a/test/asm/load-overflow.err +++ b/test/asm/load-overflow.err @@ -1,11 +1,15 @@ -error: load-overflow.asm(5): - Section 'Overflow' grew too big (max size = 0x8000 bytes, reached 0x8001). -error: load-overflow.asm(5): - Section 'oops' grew too big (max size = 0x2000 bytes, reached 0x2001). -error: load-overflow.asm(13): - Section 'Moar overflow' grew too big (max size = 0x8000 bytes, reached 0x8001). -error: load-overflow.asm(22): - Section 'lol' grew too big (max size = 0x2000 bytes, reached 0x2001). -error: load-overflow.asm(25): - Section 'Not overflowing' grew too big (max size = 0x8000 bytes, reached 0x8001). +warning: load-overflow.asm(5): [-Wempty-data-directive] + DB directive without data in ROM +warning: load-overflow.asm(6): [-Wempty-data-directive] + DB directive without data in ROM +error: load-overflow.asm(26): + Section 'Overflow' grew too big (max size = 0x8000 bytes, reached 0x8002). +error: load-overflow.asm(26): + Section 'oops' grew too big (max size = 0x2000 bytes, reached 0x2002). +error: load-overflow.asm(26): + Section 'Moar overflow' grew too big (max size = 0x8000 bytes, reached 0xD000). +error: load-overflow.asm(26): + Section 'hmm' grew too big (max size = 0x2000 bytes, reached 0x4000). +error: load-overflow.asm(26): + Section 'lol' grew too big (max size = 0x2000 bytes, reached 0x3000). error: Assembly aborted (5 errors)! diff --git a/test/asm/section-unsigned-overflow.asm b/test/asm/section-unsigned-overflow.asm new file mode 100644 index 00000000..0817389c --- /dev/null +++ b/test/asm/section-unsigned-overflow.asm @@ -0,0 +1,9 @@ +section "overflow", rom0 +rept 10 + ds $4000_0000 +endr + +section "moar overflow", romx +rept 10 + ds $4000_0000 +endr diff --git a/test/asm/section-unsigned-overflow.err b/test/asm/section-unsigned-overflow.err new file mode 100644 index 00000000..ebad10a3 --- /dev/null +++ b/test/asm/section-unsigned-overflow.err @@ -0,0 +1,2 @@ +FATAL: section-unsigned-overflow.asm(2) -> section-unsigned-overflow.asm::REPT~4(3): + Section size would overflow internal counter