From 6888337afe583745cd16f52273a3938c1b7c6c22 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Sun, 26 Jul 2015 20:28:34 +0200 Subject: [PATCH] bomb drop fly paradrop recon sweep: Fix read before array The code computing the length of the flight path checks whether the path ends with 'h'. When getpath() returns an empty path, it accesses flightpath[-1]. This could set the length to -1 (unlikely), or crash (even less likely). The former could be abused to gain mobility for sufficiently inefficient or short-ranged planes. Found with valgrind. Broken in commit 404a76f7, v4.3.27. Historically, getpath() could return paths with or without 'h', and the check was necessary. It returned an empty path only when the player gave no input, aborting the command. When the player entered the assembly point's coordinates, it returned "h". Commit 404a76f7 accidentally changed it to return "" then. Also broke flying to the assembly point's coordinates. Commit 0f1e14f (v4.3.31) fixed that part by changing getpath()'s contract: always return paths without 'h' ("" simply means empty path), and return NULL on invalid input, including no input. The flawed check is superfluous since then. Drop it. Signed-off-by: Markus Armbruster --- src/lib/commands/bomb.c | 2 -- src/lib/commands/drop.c | 2 -- src/lib/commands/fly.c | 2 -- src/lib/commands/para.c | 2 -- src/lib/commands/reco.c | 2 -- 5 files changed, 10 deletions(-) diff --git a/src/lib/commands/bomb.c b/src/lib/commands/bomb.c index 39f1aa17..6af2db6f 100644 --- a/src/lib/commands/bomb.c +++ b/src/lib/commands/bomb.c @@ -117,8 +117,6 @@ bomb(void) pr("target sector is %s\n", xyas(tx, ty, player->cnum)); getsect(tx, ty, &target); ap_to_target = strlen(flightpath); - if (flightpath[ap_to_target - 1] == 'h') - ap_to_target--; pr("range to target is %d\n", ap_to_target); /* * select planes within range diff --git a/src/lib/commands/drop.c b/src/lib/commands/drop.c index ca39b4d4..7fb72ddc 100644 --- a/src/lib/commands/drop.c +++ b/src/lib/commands/drop.c @@ -95,8 +95,6 @@ drop(void) } ap_to_target = strlen(flightpath); - if (flightpath[ap_to_target - 1] == 'h') - ap_to_target--; pr("range to target is %d\n", ap_to_target); /* * select planes within range diff --git a/src/lib/commands/fly.c b/src/lib/commands/fly.c index c82a7a4b..d3218db5 100644 --- a/src/lib/commands/fly.c +++ b/src/lib/commands/fly.c @@ -92,8 +92,6 @@ fly(void) } ap_to_target = strlen(flightpath); - if (flightpath[ap_to_target - 1] == 'h') - ap_to_target--; pr("range to target is %d\n", ap_to_target); /* * select planes within range diff --git a/src/lib/commands/para.c b/src/lib/commands/para.c index 41a0d08a..d8665e9b 100644 --- a/src/lib/commands/para.c +++ b/src/lib/commands/para.c @@ -72,8 +72,6 @@ para(void) getsect(tx, ty, &target); pr("LZ is %s\n", xyas(tx, ty, player->cnum)); ap_to_target = strlen(flightpath); - if (flightpath[ap_to_target - 1] == 'h') - ap_to_target--; pr("range to target is %d\n", ap_to_target); if (target.sct_own == player->cnum) { pr("You can't air-assault your own sector!\n"); diff --git a/src/lib/commands/reco.c b/src/lib/commands/reco.c index 32f590c5..274f662d 100644 --- a/src/lib/commands/reco.c +++ b/src/lib/commands/reco.c @@ -75,8 +75,6 @@ reco(void) cno = target.gen.ef_type == EF_SHIP ? target.gen.uid : -1; ap_to_target = strlen(flightpath); - if (flightpath[ap_to_target - 1] == 'h') - ap_to_target--; pr("range to target is %d\n", ap_to_target); /* * select planes within range