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 <armbru@pond.sub.org>

diff --git a/src/lib/commands/give.c b/src/lib/commands/give.c
index dfe1df77ea7bd40bedefcdd8c1ab2e819a06a09a..f4bf59f878b523906c3061f91a0a08c5f2ca5c0c 100644 (file)
--- 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 <config.h>
@@ -65,11 +66,11 @@ give(void)
        if (!check_sect_ok(&sect))
            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(&sect);

diff --git a/tests/actofgod/actofgod.xdump b/tests/actofgod/actofgod.xdump
index ac03677cb63e8fb78a8e25d471ba4e4241bb93cc..cf3315da1f094b4b3c8f5738bc4c525baab81b95 100644 (file)
--- 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 b29b35e4dd10ffee4b7e353196fe0ca96f13ece3..b73f80f90ce855b087cbdb56f0b71e3bfa0cb27b 100644 (file)
--- 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.