xundump: Refuse to undump strings too long for terminating null

We're dealing with three kinds of string storage: char * pointing to a
null-terminated string, char[] holding a null-terminated string, and
char holding a string of length 0 or 1.

Unfortunately, xdump meta data doesn't distinguish the latter two:
both are NSC_STRINGY.  Because of that, xundump happily fills char[]
to the limit, producing strings that aren't null-terminated, resulting
in read beyond buffer and possibly worse.

Affects struct shpstr members shp_path, shp_name, shp_rpath, struct
lndstr member lnd_rpath, and struct natstr members nat_cnam, nat_pnam,
nat_hostaddr, nat_hostname, nat_userid.  Since these are all in game
state, only the empdump utility program is affected, not the
configuration table reader.

We clearly need to require null-termination for char[] values.  Since
using char[1] for null-terminated strings makes no sense, we can still
make NSC_STRINGY with length 1 serve char values as before, by
permitting non-null-terminated strings only when length is 1.  Ugly
wart, but it fixes the bug without a possibly awkward change xdump
meta-data.

Signed-off-by: Markus Armbruster <armbru@pond.sub.org>

diff --git a/include/nsc.h b/include/nsc.h
index 3a6bb1470c738e7a29b39aeef75cf0eb7f18268d..3b7fc35fe5e8c58cd92a8ed727d7b35d6272c3f4 100644 (file)
--- a/include/nsc.h
+++ b/include/nsc.h
@@ -28,7 +28,7 @@
  *
  *  Known contributors to this file:
  *     Dave Pare, 1989
- *     Markus Armbruster, 2004-2010
+ *     Markus Armbruster, 2004-2014
  */
 
 #ifndef NSC_H
@@ -92,9 +92,10 @@ enum nsc_cat {
  * promoted type.
  * If category is NSC_OFF, the value is in a context object, and
  * val_as.sym specifies how to get it, as follows.
- * If sym.get is null, and type is NSC_STRINGY, the value is an array
- * of sym.len characters starting at sym.offs in the context object.
- * sym.idx must be zero.
+ * If sym.get is null, and type is NSC_STRINGY, the value is a string
+ * stored in sym.len characters starting at sym.offs in the context
+ * object.  sym.idx must be zero.  Ugly wart: if sym.len is one, the
+ * terminating null character may be omitted.
  * Else if sym.get is null, and sym.len is zero, the value is in the
  * context object at offset sym.off.  sym.idx must be zero.
  * Else if sym.get is null, sym.len is non-zero, and the value has
@@ -202,7 +203,9 @@ enum {
  * A selector with ca_type NSC_NOTYPE is invalid.
  * If ca_get is null, the selector describes a datum of type ca_type
  * at offset ca_offs in the context object.
- * A datum of type NSC_STRINGY is an array of ca_len characters.
+ * A datum of type NSC_STRINGY is a string stored in an array of
+ * ca_len characters.  Ugly wart: if ca_len is one, the terminating
+ * null character may be omitted.
  * A datum of any other type is either a scalar of that type (if
  * ca_len is zero), or an array of ca_len elements of that type.
  * If ca_get is not null, the selector is virtual.  Values can be

diff --git a/src/lib/common/xundump.c b/src/lib/common/xundump.c
index 83c25820617cc00b821f3d28df68d48ff93803b4..1f486bcb96eb9401856874ea6f1bbf646574a296 100644 (file)
--- a/src/lib/common/xundump.c
+++ b/src/lib/common/xundump.c
@@ -28,7 +28,7 @@
  *
  *  Known contributors to this file:
  *     Ron Koenderink, 2005
- *     Markus Armbruster, 2005-2011
+ *     Markus Armbruster, 2005-2014
  */
 
 /*
@@ -775,7 +775,7 @@ setstr(int fldno, char *str)
 {
     struct castr *ca;
     int must_match, idx;
-    size_t len;
+    size_t sz, len;
     char *memb_ptr, *old;
 
     ca = getfld(fldno, &idx);
@@ -803,13 +803,15 @@ setstr(int fldno, char *str)
            return -1;
        if (!str)
            return gripe("Field %d doesn't take nil", fldno + 1);
-       len = ca->ca_len;
+       /* Wart: if ca_len <= 1, the terminating null may be omitted */
+       sz = ca->ca_len;
+       len = sz > 1 ? sz - 1 : sz;
        if (strlen(str) > len)
            return gripe("Field %d takes at most %d characters",
                         fldno + 1, (int)len);
        old = memb_ptr;
        if (!must_match)
-           strncpy(memb_ptr, str, len);
+           strncpy(memb_ptr, str, sz);
        break;
     default:
        return gripe("Field %d doesn't take strings", fldno + 1);