Fix ancient off-by-one at end boundary in string array size calculation

- String array size calculation could read one byte past data end
  pointer when expected count and number of \0's disagree (ie invalid data)
  due to while condition side-effects + bounds checking being in
  the inner loop.
- Lift the string length calculation to inline helper function, used for
  both string and string array types.
- Streamline the calculations:
  - Eliminate unnecessary length increments, calculate the length
    from pointer distance
  - Eliminate end pointer NULL checking within the loop: when caller
    doesn't supply end pointer, cap to HEADER_MAX_DATA (ie 16MB),
    anything larger would trip up in later hdrchkData() checks anyway.
  - Avoid the off-by-one by eliminating the problematic inner loop.

diff --git a/lib/header.c b/lib/header.c
index e54ef70..8fafc86 100644 (file)
--- a/lib/header.c
+++ b/lib/header.c
@@ -297,6 +297,18 @@ unsigned headerSizeof(Header h, int magicp)
     return size;
 }
 
+/* Bounded header string (array) size calculation, return -1 on error */
+static inline int strtaglen(const char *str, rpm_count_t c, const char *end)
+{
+    const char *s;
+
+    for (s = str; s < end; s++) {
+       if ((*s == '\0') && (--c == 0))
+           break;
+    }
+    return (c > 0) ? -1 : (s - str + 1);
+}
+
 /**
  * Return length of entry data.
  * @param type         entry data type
@@ -309,20 +321,16 @@ unsigned headerSizeof(Header h, int magicp)
 static int dataLength(rpm_tagtype_t type, rpm_constdata_t p, rpm_count_t count,
                         int onDisk, rpm_constdata_t pend)
 {
-    const unsigned char * s = p;
-    const unsigned char * se = pend;
+    const char * s = p;
+    /* Not all callers supply data end, avoid falling over edge of the world */
+    const char * se = pend ? pend : s + HEADER_DATA_MAX;
     int length = 0;
 
     switch (type) {
     case RPM_STRING_TYPE:
        if (count != 1)
            return -1;
-       while (*s++) {
-           if (se && s > se)
-               return -1;
-           length++;
-       }
-       length++;       /* count nul terminator too. */
+       length = strtaglen(s, 1, se);
        break;
 
     case RPM_STRING_ARRAY_TYPE:
@@ -331,14 +339,7 @@ static int dataLength(rpm_tagtype_t type, rpm_constdata_t p, rpm_count_t count,
        /* Compute sum of length of all strings, including nul terminators */
 
        if (onDisk) {
-           while (count--) {
-               length++;       /* count nul terminator too */
-               while (*s++) {
-                   if (se && s > se)
-                       return -1;
-                   length++;
-               }
-           }
+           length = strtaglen(s, count, se);
        } else {
            const char ** av = (const char **)p;
            while (count--) {

diff --git a/lib/header_internal.h b/lib/header_internal.h
index 82b96e3..6400711 100644 (file)
--- a/lib/header_internal.h
+++ b/lib/header_internal.h
@@ -47,7 +47,8 @@ struct indexEntry_s {
  * Sanity check on data size and/or offset and/or count.
  * This check imposes a limit of 16 MB, more than enough.
  */
-#define hdrchkData(_nbytes) ((_nbytes) & 0xff000000)
+#define HEADER_DATA_MAX 0x00ffffff
+#define hdrchkData(_nbytes) ((_nbytes) & (~HEADER_DATA_MAX))
 
 /**
  * Sanity check on data alignment for data type.