From 9aaf6093599c0018c2639836395f7dba27ae0593 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Sun, 20 Jan 2013 16:39:32 +0100 Subject: [PATCH] give: Fix integer overflows on silly third arguments give() silently caps the resulting number of items to 0..ITEM_MAX. However, its test for "< 0" suffers integer overflow on two's complement machines (i.e. practically everywhere) when the amount argument is INT_MIN. give() proceeds as if the result was in range: it sets the number of items to (short)(n + INT_MIN), telexes the owner that INT_MIN items were stolen (obviously bogus), and tells the deity that there are now n + INT_MIN items in X,Y. On common machines, (short)(n + INT_MIN) == n, i.e. nothing is given. On an oddball machine with short as wide as int, the cast to short does nothing, item_prewrite() oopses, and corrects the number of items to zero. In both cases, output and telegram lie. Likewise, its test for "> ITEM_MAX" suffers integer overflow for sufficiently big amount arguments. Again, give() proceeds as if the result was in range: it sets the number of items to (short)(n + amt), telexes the owner that -amt items were stolen (obviously bogus), and tells the deity that there are now close to INT_MIN items in X,Y. On common machines, (short)(n + amt) = n + INT_MAX - amt - 1, i.e. some items are stolen. On an oddball machine with short as wide as int, the cast to short does nothing, item_prewrite() oopses, and corrects the number of items to zero. Again, output and telegram lie. Aside: setsector can suffer similar overflows, but it reports the resulting change correctly. Good enough. Signed-off-by: Markus Armbruster --- src/lib/commands/give.c | 7 ++++--- tests/actofgod/actofgod.xdump | 6 +++--- tests/actofgod/journal.log | 7 +++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/lib/commands/give.c b/src/lib/commands/give.c index dfe1df77e..f4bf59f87 100644 --- a/src/lib/commands/give.c +++ b/src/lib/commands/give.c @@ -29,6 +29,7 @@ * Known contributors to this file: * David Muir Sharnoff * Steve McClure, 1997 + * Markus Armbruster, 2004-2013 */ #include @@ -65,11 +66,11 @@ give(void) if (!check_sect_ok(§)) return RET_FAIL; n = sect.sct_item[ip->i_uid]; - if (amt < 0 && -amt > n) { + if (amt < 0 && n + amt < 0) m = 0; - } else if (amt > 0 && amt + n > ITEM_MAX) { + else if (amt > 0 && n > ITEM_MAX - amt) m = ITEM_MAX; - } else + else m = n + amt; sect.sct_item[ip->i_uid] = m; putsect(§); diff --git a/tests/actofgod/actofgod.xdump b/tests/actofgod/actofgod.xdump index ac03677cb..cf3315da1 100644 --- a/tests/actofgod/actofgod.xdump +++ b/tests/actofgod/actofgod.xdump @@ -75,7 +75,7 @@ owner xloc yloc des effic mobil off loyal terr0 terr1 terr2 terr3 dterr xdist yd 0 0 6 0 0 0 0 0 0 0 0 0 0 0 6 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 healthy 0 0 0 0 0 0 0 0 1 2 6 4 0 0 0 0 0 0 0 0 0 2 6 0 0 100 1 4 0 0 0 0 0 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 healthy 0 0 0 0 0 0 0 0 1 4 6 4 0 0 0 0 0 0 0 0 0 4 6 0 0 100 0 4 0 0 0 0 0 1 2 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 healthy 0 0 0 0 0 0 0 0 -1 6 6 4 0 0 0 0 0 0 0 0 0 6 6 0 0 100 0 4 0 0 0 0 0 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 healthy 0 0 0 0 0 0 0 0 +1 6 6 4 0 0 0 0 0 0 0 0 0 6 6 0 0 100 0 4 0 0 0 0 0 1 9999 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 healthy 0 0 0 0 0 0 0 0 1 8 6 4 0 0 0 0 0 0 0 0 0 8 6 0 0 100 0 4 0 0 0 0 0 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 healthy 0 0 0 0 0 0 0 0 1 10 6 4 0 0 0 0 0 0 0 0 0 10 6 0 0 100 0 4 0 0 0 0 0 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 healthy 0 0 0 0 0 0 0 0 2 -12 6 4 0 0 0 0 0 0 0 0 0 -12 6 0 0 100 0 4 0 0 0 0 0 2 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 healthy 0 0 0 0 0 0 0 0 @@ -364,8 +364,8 @@ actor action victim times duration time 0 43 98 1 0 0 0 43 2 1 0 0 0 43 1 54 0 0 -1 45 0 3 0 0 -0 42 1 3 0 0 +0 42 1 4 0 0 +1 45 0 1 0 0 0 44 2 1 0 0 0 43 3 1 0 0 /config diff --git a/tests/actofgod/journal.log b/tests/actofgod/journal.log index b29b35e4d..b73f80f90 100644 --- a/tests/actofgod/journal.log +++ b/tests/actofgod/journal.log @@ -752,7 +752,7 @@ Play#0 output Play#0 6 0 640 Play#0 input give l 2,6 -2147483648 Play#0 command give - Play#0 output Play#0 1 -2147483648 light products in 2,6 + Play#0 output Play#0 1 0 light products in 2,6 Play#0 output Play#0 6 0 640 Play#0 input give c 4:8,6 1 Play#0 command give @@ -762,7 +762,7 @@ Play#0 output Play#0 6 0 640 Play#0 input give c 6,6 2147483647 Play#0 command give - Play#0 output Play#0 1 -2147483647 civilians in 6,6 + Play#0 output Play#0 1 9999 civilians in 6,6 Play#0 output Play#0 6 0 640 Play#0 input give c 8,6 -1 Play#0 command give @@ -1535,11 +1535,10 @@ Play#0 output Play#0 1 Available workforce in 1,5 was changed from 0 to 1 by an act of POGO Play#0 output Play#0 1 Mobility in 3,5 was changed from 0 to 2 by an act of POGO Play#0 output Play#0 1 Available workforce in 3,5 was changed from 0 to 1 by an act of POGO - Play#0 output Play#0 1 POGO stole -2147483648 light products from 2,6 Play#0 output Play#0 1 POGO gave you 1 civilians in 4,6 Play#0 output Play#0 1 POGO gave you 1 civilians in 6,6 Play#0 output Play#0 1 POGO gave you 1 civilians in 8,6 - Play#0 output Play#0 1 POGO stole -2147483647 civilians from 6,6 + Play#0 output Play#0 1 POGO gave you 9997 civilians in 6,6 Play#0 output Play#0 1 POGO stole 1 civilians from 8,6 Play#0 output Play#0 1 Military reserves changed from 0 to 0 by divine intervention. Play#0 output Play#0 1 Money changed from 0 to -2147483648 by divine intervention. -- 2.43.0