From 1d89d75381d30d7eda8bf6c67cbc6002f83cebb9 Mon Sep 17 00:00:00 2001
From: Eldred Habert <me@eldred.fr>
Date: Thu, 25 Jul 2024 23:00:48 +0200
Subject: [PATCH] Fix use-after-free when keeping pointers to args from
 at-files (#1426)

---
 src/gfx/main.cpp           |  12 +++++++++---
 test/gfx/at-file-ref.err   |  14 ++++++++++++++
 test/gfx/at-file-ref.flags |   2 ++
 test/gfx/at-file-ref.png   | Bin 0 -> 712 bytes
 4 files changed, 25 insertions(+), 3 deletions(-)
 create mode 100644 test/gfx/at-file-ref.err
 create mode 100644 test/gfx/at-file-ref.flags
 create mode 100644 test/gfx/at-file-ref.png

diff --git a/src/gfx/main.cpp b/src/gfx/main.cpp
index 240cd129..00955c50 100644
--- a/src/gfx/main.cpp
+++ b/src/gfx/main.cpp
@@ -13,6 +13,7 @@
 #include <string.h>
 #include <string_view>
 #include <type_traits>
+#include <vector>
 
 #include "extern/getopt.hpp"
 #include "file.hpp"
@@ -601,7 +602,6 @@ int main(int argc, char *argv[]) {
 	struct AtFileStackEntry {
 		int parentInd;            // Saved offset into parent argv
 		std::vector<char *> argv; // This context's arg pointer vec
-		std::vector<char> argPool;
 
 		AtFileStackEntry(int parentInd_, std::vector<char *> argv_)
 		    : parentInd(parentInd_), argv(argv_) {}
@@ -610,18 +610,24 @@ int main(int argc, char *argv[]) {
 
 	int curArgc = argc;
 	char **curArgv = argv;
+	std::vector<std::vector<char>> argPools;
 	for (;;) {
 		char *atFileName = parseArgv(curArgc, curArgv);
 		if (atFileName) {
+			// We need to allocate a new arg pool for each at-file, so as not to invalidate pointers
+			// previous at-files may have generated to their own arg pools.
+			// But for the same reason, the arg pool must also outlive the at-file's stack entry!
+			auto &argPool = argPools.emplace_back();
+
 			// Copy `argv[0]` for error reporting, and because option parsing skips it
 			AtFileStackEntry &stackEntry =
 			    atFileStack.emplace_back(musl_optind, std::vector{atFileName});
 			// It would be nice to compute the char pointers on the fly, but reallocs don't allow
 			// that; so we must compute the offsets after the pool is fixed
-			auto offsets = readAtFile(&musl_optarg[1], stackEntry.argPool);
+			auto offsets = readAtFile(&musl_optarg[1], argPool);
 			stackEntry.argv.reserve(offsets.size() + 2); // Avoid a bunch of reallocs
 			for (size_t ofs : offsets) {
-				stackEntry.argv.push_back(&stackEntry.argPool.data()[ofs]);
+				stackEntry.argv.push_back(&argPool.data()[ofs]);
 			}
 			stackEntry.argv.push_back(nullptr); // Don't forget the arg vector terminator!
 
diff --git a/test/gfx/at-file-ref.err b/test/gfx/at-file-ref.err
new file mode 100644
index 00000000..b0358a13
--- /dev/null
+++ b/test/gfx/at-file-ref.err
@@ -0,0 +1,14 @@
+error: Failed to fit tile colors [$7f55, $7fff] in specified palettes
+error: Failed to fit tile colors [$7f55] in specified palettes
+error: Failed to fit tile colors [$6c8a, $7f55, $7fff] in specified palettes
+error: Failed to fit tile colors [$6c8a, $7fff] in specified palettes
+error: Failed to fit tile colors [$6c8a, $7f55] in specified palettes
+error: Failed to fit tile colors [$6c8a] in specified palettes
+error: Failed to fit tile colors [$7fff] in specified palettes
+note: The following palettes were specified:
+        [$3f65, $36b3, $262a, $50b0]
+        [$5f77, $213d, $41a6, $40ee]
+        [$5f77, $267c, $213d, $40ee]
+        [$53c3, $3f65, $36b3]
+        [$267c, $41a6]
+Conversion aborted after 7 errors
diff --git a/test/gfx/at-file-ref.flags b/test/gfx/at-file-ref.flags
new file mode 100644
index 00000000..fe09f6d3
--- /dev/null
+++ b/test/gfx/at-file-ref.flags
@@ -0,0 +1,2 @@
+-m
+-c gbc:result.pal
diff --git a/test/gfx/at-file-ref.png b/test/gfx/at-file-ref.png
new file mode 100644
index 0000000000000000000000000000000000000000..57fdd36f50ceea06ff1b3c6d58a745fd05c30f13
GIT binary patch
literal 712
zcmV;(0yq7MP)<h;3K|Lk000e1NJLTq001BW001Be0ssI2{21+{0004mX+uL$Nkc;*
zaB^>EX>4Tx04R}tkv&MmKp2MKrk09SMA|{bAwzYtAS&XhRVYG*P%E_RU~=gnG%+M8
zE{=k0!NH%!s)LKOt`4q(Aov5~=;Wm6A|-y86k5c1$8itueecWNcYx5SGR^8512o+>
zGpVGQ%dd#xSA-BnKh3zzEMr!Z((oN$_XzO)F2S?>>;4?QYQbVaKqQ`FhG`S86Hjg0
z2Iqa^7%R&v@j3CRNf#u3<htVV8|RYC0?&+@*~}bqj94snu+qV-Y-+@l#Bo*ADPPEW
zta9GstW|5Qbx;1nNI_p-;ySG%q_BWRND!f*h6<{%k)T~C#X^S8lRo}I*DsMvAy);A
z9P`+K2HEw4|H1FxTE&TRFDa4)LNAW<F${!vfo9!tzK<QJc>)BVfh)c3-)I2SpQP8@
zTKov;+XgPK+nTZmT<!n^PljyDt`wvt6pO(78GTb8h~5G{t6tw)=Qw=;vec{e4RCM>
zjFc#Q&F9_SoqhYarq#b6S`%`eYNz2J00009a7bBm001r{001r{0eGc9b^rhX>PbXF
zR7l6oRxu94APfX6FR7F+eM^=MRi9XOV98s$G*Z7phDeo*iERvt;w{0tGv_nlV~p-U
zUhnkJR@`r1%mFjHG8tT5h#X)pUm4(bjs^t$zPoyYFw6mcKEGGI>ak6gJqW|hCtwaJ
z^|gq8w2QhTt*czbmv2MgWeyS-L0fSWbQOUfeVlr#b2Ev?skijN+}jKRow9oL!%tJZ
zi6-S|cCd1^LeER2Ir2__qtSun&CZ0U?;;$J%O{JTGzl*y3JU~34rT4Me=<SQT|^1A
uP1aZ|n^WK-o?~lOD-k0XPi!GL*Pt!wGh*YmoK;f*0000<MNUMnLSTZK&q1sJ

literal 0
HcmV?d00001