Avoid unsigned overflow in INCBIN slice size check

Allowing these in object files would lose anything after the '\0'
when RGBLINK reads the object.

CMakeLists.txt

@@ -11,21 +11,21 @@ file(STRINGS "include/version.hpp" version_defines REGEX "^[ \t]*#define[ \t]+PA
 foreach(line IN LISTS version_defines)
   # We want the `CMAKE_MATCH_n` variables, so we just need to run *some* regex op.
   string(REGEX MATCH "PACKAGE_(VERSION_[^ \t]+)[ \t]+([0-9]+)" dummy "${line}")
-  set("${CMAKE_MATCH_1}" "${CMAKE_MATCH_2}")
+  set("RGBDS_${CMAKE_MATCH_1}" "${CMAKE_MATCH_2}")
 endforeach()
 
 project(rgbds
-        VERSION "${VERSION_MAJOR}.${VERSION_MINOR}.${VERSION_PATCH}"
+        VERSION "${RGBDS_VERSION_MAJOR}.${RGBDS_VERSION_MINOR}.${RGBDS_VERSION_PATCH}"
         LANGUAGES CXX
         DESCRIPTION "Game Boy assembly toolchain"
         HOMEPAGE_URL "https://rgbds.gbdev.io")
 if(DEFINED VERSION_RC)
-  string(APPEND CMAKE_PROJECT_VERSION "-rc${VERSION_RC}")
+  string(APPEND PROJECT_VERSION "-rc${RGBDS_VERSION_RC}")
 endif()
 
 # Reject in-source builds, as they may conflict with the Makefile.
-get_filename_component(srcdir "${CMAKE_SOURCE_DIR}" REALPATH)
+get_filename_component(srcdir "${CMAKE_CURRENT_SOURCE_DIR}" REALPATH)
-get_filename_component(bindir "${CMAKE_BINARY_DIR}" REALPATH)
+get_filename_component(bindir "${CMAKE_CURRENT_BINARY_DIR}" REALPATH)
 if(srcdir STREQUAL bindir)
   message(FATAL_ERROR "RGBDS should not be built in the source directory.
 Instead, create a separate build directory and specify to CMake the path to the source directory.")
@@ -82,35 +82,45 @@ endif()
 # Use versioning consistent with Makefile:
 # the git revision is used but uses the fallback in an archive.
 
-message(CHECK_START "Determining RGBDS version from Git history")
+if(DEFINED RGBDS_VERSION_STRING) # Possibly stored in the cache, e.g. if specified on the CLI.
-list(APPEND CMAKE_MESSAGE_INDENT "  ")
+  if(RGBDS_VERSION_STRING STREQUAL "")
-set(GIT_REV "") # This fallback is important!
+    message(STATUS "Will generate version string from `version.hpp`")
-find_package(Git)
-list(POP_BACK CMAKE_MESSAGE_INDENT)
-if(NOT Git_FOUND)
-  message(CHECK_FAIL "Git not found")
-else()
-  execute_process(COMMAND "${GIT_EXECUTABLE}" --git-dir=.git -c safe.directory='*'
-                            describe --tags --dirty --always --match "v[0-9]*"
-                  WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}"
-                  OUTPUT_VARIABLE GIT_REV OUTPUT_STRIP_TRAILING_WHITESPACE
-                  ERROR_VARIABLE git_err ERROR_STRIP_TRAILING_WHITESPACE
-                  RESULT_VARIABLE result)
-  if(NOT result EQUAL 0)
-    # Note that this happens e.g. when building from a tarball, so it shouldn't fail the build!
-    message(CHECK_FAIL "error ${result} from Git:")
-    list(APPEND CMAKE_MESSAGE_INDENT "     ")
-    message("${git_err}")
-    list(POP_BACK CMAKE_MESSAGE_INDENT)
   else()
-    message(CHECK_PASS "${GIT_REV}")
+    message(STATUS "Using provided version string - ${RGBDS_VERSION_STRING}")
-    if(NOT "${GIT_REV}" MATCHES "^v[0-9]+\\.[0-9]+\\.[0-9]+(-rc[0-9]+)?")
+  endif()
-      # Can't find an ancestor tag! (That passes `--match`, anyway.)
+else()
-      message(WARNING "No `v*` Git tag reachable; falling back")
+  message(CHECK_START "Determining RGBDS version from Git history")
-    elseif(NOT CMAKE_MATCH_0 STREQUAL "v${CMAKE_PROJECT_VERSION}")
+  # Note that we do NOT store this in the cache, since the Git revision is fairly volatile.
-      message(SEND_ERROR "\
+  set(RGBDS_VERSION_STRING "") # An empty value causes `version.cpp` to generate a version string.
+
+  list(APPEND CMAKE_MESSAGE_INDENT "  ")
+  find_package(Git)
+  list(POP_BACK CMAKE_MESSAGE_INDENT)
+  if(NOT Git_FOUND)
+    message(CHECK_FAIL "Git not found")
+  else()
+    execute_process(COMMAND "${GIT_EXECUTABLE}" --git-dir=.git -c safe.directory='*'
+                              describe --tags --dirty --always --match "v[0-9]*"
+                    WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
+                    OUTPUT_VARIABLE RGBDS_VERSION_STRING OUTPUT_STRIP_TRAILING_WHITESPACE
+                    ERROR_VARIABLE git_err ERROR_STRIP_TRAILING_WHITESPACE
+                    RESULT_VARIABLE result)
+    if(NOT result EQUAL 0)
+      # Note that this happens e.g. when building from a tarball, so it shouldn't fail the build!
+      message(CHECK_FAIL "error ${result} from Git:")
+      list(APPEND CMAKE_MESSAGE_INDENT "     ")
+      message("${git_err}")
+      list(POP_BACK CMAKE_MESSAGE_INDENT)
+    else()
+      message(CHECK_PASS "${RGBDS_VERSION_STRING}")
+      if(NOT "${RGBDS_VERSION_STRING}" MATCHES "^v[0-9]+\\.[0-9]+\\.[0-9]+(-rc[0-9]+)?")
+        # Can't find an ancestor tag! (That passes `--match`, anyway.)
+        message(WARNING "No `v*` Git tag reachable; falling back")
+      elseif(NOT CMAKE_MATCH_0 STREQUAL "v${PROJECT_VERSION}")
+        message(SEND_ERROR "\
 Version mismatch! Git says ${CMAKE_MATCH_0},
-          version.hpp says v${CMAKE_PROJECT_VERSION}!")
+            version.hpp says v${PROJECT_VERSION}")
+      endif()
     endif()
   endif()
 endif()
@@ -215,14 +225,14 @@ endforeach()
 # the rest is rather convention from our side, and thus more appropriate for presets or CLI flags.
 ## CPACK_PACKAGE_NAME: copied from `project()`
 set(CPACK_PACKAGE_VENDOR "GBDev")
-set(CPACK_PACKAGE_VERSION "${CMAKE_PROJECT_VERSION}") # The individual components are defined implicitly.
+set(CPACK_PACKAGE_VERSION "${PROJECT_VERSION}") # The individual components are defined implicitly.
 set(CPACK_PACKAGE_DESCRIPTION "An assembly toolchain for the Nintendo Game Boy and Game Boy Color") # Same as our repo's description.
 ## CPACK_PACKAGE_DESCRIPTION_SUMMARY: copied from `project()`
 set(CPACK_PACKAGE_HOMEPAGE_URL "https://rgbds.gbdev.io")
 ## CPACK_PACKAGE_FILE_NAME: should be provided at runtime (`cpack -P`)
 set(CPACK_PACKAGE_CHECKSUM SHA256)
-set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_SOURCE_DIR}/LICENSE")
+set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_SOURCE_DIR}/LICENSE")
-set(CPACK_RESOURCE_FILE_README "${CMAKE_SOURCE_DIR}/README.md")
+set(CPACK_RESOURCE_FILE_README "${CMAKE_CURRENT_SOURCE_DIR}/README.md")
 set(CPACK_STRIP_FILES ON) # Only applies to binary packages, not sources.
 set(CPACK_VERBATIM_VARIABLES ON)
 set(CPACK_THREADS 0) # Use all available CPU cores.

include/itertools.hpp

@@ -30,8 +30,12 @@ auto end(ReversedIterable<IterableT> r) {
 	return std::rend(r._iterable);
 }
 
+// Prevent creation of a dangling reference to a temporary rvalue
 template<typename IterableT>
-ReversedIterable<IterableT> reversed(IterableT &&_iterable) {
+ReversedIterable<IterableT> reversed(IterableT &&_iterable) = delete;
+
+template<typename IterableT>
+ReversedIterable<IterableT> reversed(IterableT &_iterable) {
 	return {_iterable};
 }
 

man/rgbasm.1

@@ -8,7 +8,8 @@
 .Nd Game Boy assembler
 .Sh SYNOPSIS
 .Nm
-.Op Fl EhVvw
+.Op Fl EhVw
+.Op Fl v Op Fl v No ...
 .Op Fl B Ar param
 .Op Fl b Ar chars
 .Op Fl \-color Ar when

man/rgblink.1

@@ -8,7 +8,8 @@
 .Nd Game Boy linker
 .Sh SYNOPSIS
 .Nm
-.Op Fl dhMtVvwx
+.Op Fl dhMtVwx
+.Op Fl v Op Fl v No ...
 .Op Fl B Ar param
 .Op Fl \-color Ar when
 .Op Fl l Ar linker_script

src/CMakeLists.txt

@@ -9,7 +9,7 @@ add_library(common OBJECT
     "util.cpp"
     "version.cpp"
 )
-target_compile_definitions(common PRIVATE "BUILD_VERSION_STRING=\"${GIT_REV}\"")
+target_compile_definitions(common PRIVATE "BUILD_VERSION_STRING=\"${RGBDS_VERSION_STRING}\"")
 
 find_package(BISON 3.0.0 REQUIRED)
 # BISON 4.0 deprecates passing this BISON_FLAGS string to `bison_target`'s `COMPILE_FLAGS`,

src/asm/actions.cpp

@@ -555,7 +555,7 @@ std::string act_StringFormat(
 	std::string str;
 	size_t argIndex = 0;
 
-	for (size_t i = 0; spec[i] != '\0';) {
+	for (size_t i = 0; i < spec.length();) {
 		if (int c = spec[i]; c != '%') {
 			str += c;
 			++i;

src/asm/lexer.cpp

@@ -542,7 +542,7 @@ static uint32_t readBracketedMacroArgNum() {
 			}
 		}
 		uint32_t n = readNumber<10>(bumpChar(), nullptr);
-		if (n > INT32_MAX) {
+		if (n > INT32_MAX && !(negative && n == static_cast<uint32_t>(INT32_MAX) + 1)) {
 			error("Number in bracketed macro argument is too large");
 			return 0;
 		}

src/asm/macro.cpp

@@ -72,7 +72,7 @@ void MacroArgs::shiftArgs(int32_t count) {
 	    count > 0 && (static_cast<uint32_t>(count) > nbArgs || shift > nbArgs - count)) {
 		warning(WARNING_MACRO_SHIFT, "Cannot shift macro arguments past their end");
 		shift = nbArgs;
-	} else if (count < 0 && shift < static_cast<uint32_t>(-count)) {
+	} else if (count < 0 && (count == INT32_MIN || shift < static_cast<uint32_t>(-count))) {
 		warning(WARNING_MACRO_SHIFT, "Cannot shift macro arguments past their beginning");
 		shift = 0;
 	} else {

src/asm/output.cpp

@@ -162,6 +162,10 @@ void out_CreatePatch(uint32_t type, Expression const &expr, uint32_t ofs, uint32
 void out_CreateAssert(
     AssertionType type, Expression const &expr, std::string const &message, uint32_t ofs
 ) {
+	if (message.find('\0') != std::string::npos) {
+		fatal("Assertion messages cannot contain '\\0' characters");
+	}
+
 	Assertion &assertion = assertions.emplace_front();
 
 	initPatch(assertion.patch, type, expr, ofs);

src/asm/section.cpp

@@ -544,6 +544,10 @@ void sect_NewSection(
     SectionSpec const &attrs,
     SectionModifier mod
 ) {
+	if (name.find('\0') != std::string::npos) {
+		fatal("Section names cannot contain '\\0' characters");
+	}
+
 	for (SectionStackEntry &entry : sectionStack) {
 		if (entry.section && entry.section->name == name) {
 			fatal("Section \"%s\" is already on the stack", name.c_str());
@@ -1049,7 +1053,7 @@ bool sect_BinaryFileSlice(std::string const &name, uint32_t startPos, uint32_t l
 			    *fileSize
 			);
 			return false;
-		} else if (startPos + length > *fileSize) {
+		} else if (length > *fileSize - startPos) {
 			error(
 			    "Specified range in `INCBIN` file \"%s\" is out of bounds (%" PRIu32 " + %" PRIu32
 			    " > %" PRIu64 ")",

src/fix/fix.cpp

@@ -251,9 +251,9 @@ static void
 		error("\"%s\" has more than 65536 banks", name); // LCOV_EXCL_LINE
 	};
 	static constexpr off_t NB_BANKS_LIMIT = 0x10000;
-	static_assert(NB_BANKS_LIMIT * BANK_SIZE <= SSIZE_MAX, "Max input file size too large for OS");
+	static_assert(NB_BANKS_LIMIT * BANK_SIZE < SSIZE_MAX, "Max input file size too large for OS");
 	if (input == output) {
-		if (fileSize >= NB_BANKS_LIMIT * BANK_SIZE) {
+		if (fileSize > NB_BANKS_LIMIT * BANK_SIZE) {
 			return errorTooLarge(); // LCOV_EXCL_LINE
 		}
 		// Compute number of banks and ROMX len from file size

src/fix/main.cpp

@@ -240,7 +240,7 @@ static void parseArg(int ch, char *arg) {
 			len = maxLen;
 			warning(
 			    WARNING_TRUNCATION,
-			    "Truncating title \"%s\" to %u chars",
+			    "Truncating title \"%s\" to %" PRIu8 " chars",
 			    options.title->c_str(),
 			    maxLen
 			);

src/link/layout.cpp

@@ -72,6 +72,7 @@ void layout_SetSectionType(SectionType type, uint32_t bank) {
 		    bank,
 		    typeInfo.lastBank
 		);
+		bank = typeInfo.lastBank;
 	}
 
 	setActiveTypeAndIdx(type, bank - typeInfo.firstBank);

src/link/output.cpp

@@ -279,8 +279,7 @@ static void writeSymName(std::string const &name, FILE *file) {
 		// Decode the UTF-8 codepoint; or at least attempt to
 		Utf8Decoder decoder;
 		do {
-			if (decoder.update(*ptr) != UTF8_REJECT) {
+			if (decoder.update(*ptr++) != UTF8_REJECT) {
-				++ptr;
 				continue;
 			}
 			// This sequence was invalid; emit a U+FFFD, and recover

src/link/section.cpp

@@ -146,6 +146,15 @@ static void mergeSections(Section &target, std::unique_ptr<Section> &&other) {
 
 	case SECTION_FRAGMENT:
 		checkPieceCompat(target, *other, target.size);
+		// Check that `target.size += other->size` below will not overflow
+		if (target.size + other->size > UINT16_MAX) {
+			fatalTwoAt(
+			    target,
+			    *other,
+			    "Section \"%s\" fragments combined are larger than the GB address space",
+			    target.name.c_str()
+			);
+		}
 		// Append `other` to `target`
 		other->offset = target.size;
 		target.size += other->size;

test/asm/assert-nul.asm

@@ -0,0 +1 @@
+assert x, "oops \0 null"

test/asm/assert-nul.err

@@ -0,0 +1,2 @@
+FATAL: Assertion messages cannot contain '\0' characters
+    at assert-nul.asm(1)

test/asm/negative-macro-args.err

@@ -12,9 +12,8 @@ error: Macro argument `\<2147483647>` not defined
     at negative-macro-args.asm::mac(11) <- negative-macro-args.asm(21)
 error: Macro argument `\<-2147483648>` not defined
     at negative-macro-args.asm::mac(14) <- negative-macro-args.asm(21)
-error: Number in bracketed macro argument is too large
+error: Macro argument `\<-2147483648>` not defined
     at negative-macro-args.asm::mac(14) <- negative-macro-args.asm(21)
-    while expanding symbol `i`
 error: Macro argument `\<-2147483648>` not defined
     at negative-macro-args.asm::mac(15) <- negative-macro-args.asm(21)
 error: Number in bracketed macro argument is too large

test/asm/negative-macro-args.out

@@ -8,7 +8,7 @@
 0:  == 
 -8:  == 
 2147483647:  == 
--2147483648:  == >
+-2147483648:  == 
 2147483648:  == >
 -1: G == G
 4294967295: G == >

test/asm/negative-shifted-macro-args.err

@@ -12,8 +12,6 @@ error: Macro argument `\1` not defined
     at negative-shifted-macro-args.asm::test(15) <- negative-shifted-macro-args.asm(22)
 error: Macro argument `\<-1>` not defined
     at negative-shifted-macro-args.asm::test(16) <- negative-shifted-macro-args.asm(22)
-error: Number in bracketed macro argument is too large
+error: Macro argument `\<-2147483648>` not defined
     at negative-shifted-macro-args.asm::test(19) <- negative-shifted-macro-args.asm(22)
-error: syntax error, unexpected >
+Assembly aborted with 8 errors
-    at negative-shifted-macro-args.asm::test(19) <- negative-shifted-macro-args.asm(22)
-Assembly aborted with 9 errors

test/asm/negative-shifted-macro-args.out

@@ -11,3 +11,4 @@ $0
 
 
 $A
+

test/asm/section-name-nul.asm

@@ -0,0 +1,2 @@
+SECTION "test\0 foo", ROM0
+SECTION "test\0 bar", ROM0

test/asm/section-name-nul.err

@@ -0,0 +1,2 @@
+FATAL: Section names cannot contain '\0' characters
+    at section-name-nul.asm(1)

test/asm/shift-int-min.asm

@@ -0,0 +1,6 @@
+MACRO m
+	println \1
+	shift $8000_0000 ; INT32_MIN
+	println \1
+ENDM
+	m 1, 2, 3

test/asm/shift-int-min.err

@@ -0,0 +1,2 @@
+warning: Cannot shift macro arguments past their beginning [-Wmacro-shift]
+    at shift-int-min.asm::m(3) <- shift-int-min.asm(6)

test/asm/shift-int-min.out

@@ -0,0 +1,2 @@
+$1
+$1

test/asm/strfmt.asm

@@ -26,3 +26,7 @@ PRINTLN STRFMT("%d eol %", 1)
 PRINTLN STRFMT("invalid %w spec", 42)
 
 PRINTLN STRFMT("one=%d two=%d three=%d", 1)
+
+DEF NUL EQUS STRFMT("%s \0 %s", "goodbye", "world")
+ASSERT #NUL === "goodbye \0 world"
+PRINTLN #NUL

test/link/script-oob-bank-num.link

@@ -1,5 +1,6 @@
 rom0 1
 romx 0
+romx 0xffff_ffff
 vram 2
 wram0 1
 wramx 0

test/link/script-oob-bank-num.out

@@ -2,16 +2,18 @@ error: ROM0 bank 1 does not exist (the maximum is 0)
     at script-oob-bank-num.link(1)
 error: ROMX bank 0 does not exist (the minimum is 1)
     at script-oob-bank-num.link(2)
-error: VRAM bank 2 does not exist (the maximum is 1)
+error: ROMX bank 4294967295 does not exist (the maximum is 65535)
     at script-oob-bank-num.link(3)
-error: WRAM0 bank 1 does not exist (the maximum is 0)
+error: VRAM bank 2 does not exist (the maximum is 1)
     at script-oob-bank-num.link(4)
-error: WRAMX bank 0 does not exist (the minimum is 1)
+error: WRAM0 bank 1 does not exist (the maximum is 0)
     at script-oob-bank-num.link(5)
-error: WRAMX bank 8 does not exist (the maximum is 7)
+error: WRAMX bank 0 does not exist (the minimum is 1)
     at script-oob-bank-num.link(6)
-error: OAM bank 1 does not exist (the maximum is 0)
+error: WRAMX bank 8 does not exist (the maximum is 7)
     at script-oob-bank-num.link(7)
-error: HRAM bank 1 does not exist (the maximum is 0)
+error: OAM bank 1 does not exist (the maximum is 0)
     at script-oob-bank-num.link(8)
-Linking failed with 8 errors
+error: HRAM bank 1 does not exist (the maximum is 0)
+    at script-oob-bank-num.link(9)
+Linking failed with 9 errors

test/link/script.asm

@@ -7,4 +7,4 @@ SECTION "ROM2 1K", ROMX,BANK[2]
 	ds $1000
 SECTION "ROM2 1", ROMX,BANK[2]
 	ds 1
-SECTION "\\\"\'\n\r\t\0", ROM0
+SECTION "\\\"\'\n\r\t", ROM0

test/link/section-fragment/size-overflow/a.asm

@@ -0,0 +1,2 @@
+SECTION FRAGMENT "output", ROM0
+ds $4000

test/link/section-fragment/size-overflow/out.err

@@ -0,0 +1,5 @@
+FATAL: Section "output" fragments combined are larger than the GB address space
+    at section-fragment/size-overflow/a.asm(1)
+    and also:
+    at section-fragment/size-overflow/a.asm(1)
+Linking aborted with 1 error

test/link/test.sh

@@ -398,6 +398,14 @@ rgblinkQuiet -o "$gbtemp" "$otemp" "$gbtemp2"
 tryCmpRom "$test"/ref.out.bin
 evaluateTest
 
+test="section-fragment/size-overflow"
+startTest
+"$RGBASM" -o "$otemp" "$test"/a.asm
+continueTest
+rgblinkQuiet "$otemp" "$otemp" "$otemp" "$otemp" 2>"$outtemp"
+tryDiff "$test"/out.err "$outtemp"
+evaluateTest
+
 test="section-fragment/jr-offset"
 startTest
 "$RGBASM" -o "$otemp" "$test"/a.asm