diff mbox series

[2/3] toolchain/wrapper: check we did not add more args than expected

Message ID e4024c6c1e1825bd52ab14faafbc7655d3074eb3.1723543467.git.yann.morin@orange.com
State New
Headers show
Series [1/3] toolchain/wrapper: check unsafe paths earlier | expand

Commit Message

Yann E. MORIN Aug. 13, 2024, 10:04 a.m. UTC 
From: "Yann E. MORIN" <yann.morin@orange.com>

We have a hard-coded constant that defines how many expected args we may
conditionally add at most, but it is very easy to miss updating that
when adding new conditional args.

Add a check that we did not overshoot the allowance.

Ideally, we would have a nice way to add to, and extend the *args array
dynamically, but this would be quite costly, while the wrapper is a hot
path to the compiler. So, this test is a better solution in the end: it
is simple and cheap.

Note that this would only trigger at runtime when all conditional args
are added, which can only happen in special conditions, so local testing
might not always catch it.

Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Cc: Romain Naour <romain.naour@gmail.com>
Cc: Giulio Benetti <giulio.benetti@benettiengineering.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 toolchain/toolchain-wrapper.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Peter Korsgaard May 18, 2025, 12:51 p.m. UTC | #1 
>>>>>   <yann.morin@orange.com> writes:

 > From: "Yann E. MORIN" <yann.morin@orange.com>
 > We have a hard-coded constant that defines how many expected args we may
 > conditionally add at most, but it is very easy to miss updating that
 > when adding new conditional args.

 > Add a check that we did not overshoot the allowance.

 > Ideally, we would have a nice way to add to, and extend the *args array
 > dynamically, but this would be quite costly, while the wrapper is a hot
 > path to the compiler. So, this test is a better solution in the end: it
 > is simple and cheap.

Costly? It would just be a realloc call, and the list of argument
pointers is not very long, so I doubt it would be noticable compared to
all the argument parsing and finally running the real compiler.

In fact, I think it would make more sense to get rid of our
EXCLUSIVE_ARGS constant and just allocate room for E.G. 1024 arguments
and then just realloc in the special cases where that is not enough.

I've sent a small series doing that here:

https://patchwork.ozlabs.org/project/buildroot/patch/20250518124949.4159568-2-peter@korsgaard.com/
diff mbox series

Patch

diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index 03977eb408..7647a1a12d 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -240,7 +240,7 @@  bool parse_source_date_epoch_from_env(void)
 
 int main(int argc, char **argv)
 {
-	char **args, **cur, **exec_args;
+	char **args, **cur, **exec_args, **cond_args;
 	char *relbasedir, *absbasedir;
 	char *progpath = argv[0];
 	char *basename;
@@ -365,7 +365,7 @@  int main(int argc, char **argv)
 
 	/* start with predefined args */
 	memcpy(cur, predef_args, sizeof(predef_args));
-	cur += sizeof(predef_args) / sizeof(predef_args[0]);
+	cond_args = cur += sizeof(predef_args) / sizeof(predef_args[0]);
 
 #ifdef BR_FLOAT_ABI
 	/* add float abi if not overridden in args */
@@ -496,6 +496,13 @@  int main(int argc, char **argv)
 #endif
 	}
 
+	/* Check that we did not add more conditional args than we expected */
+	if ((cur-cond_args) > EXCLUSIVE_ARGS) {
+		errno = E2BIG;
+		perror(__FILE__ ": Not enough EXCLUSIVE_ARGS");
+		return 3;
+	}
+
 	/* append forward args */
 	memcpy(cur, &argv[1], sizeof(char *) * (argc - 1));
 	cur += argc - 1;