GNOME.org
 

[libxml2/fix-more-xmlBuf-and-xmlBuffer-issues] Fix more overflow checks, off-by-ones and missing NUL terminators in xmlBuf and xmlBuffer


commit 22dd9bd284a0b9b2f3baf02e167cf56b1557b136
Author: David Kilzer <ddkilzer apple com>
Date:   Fri May 13 14:43:33 2022 -0700

    Fix more overflow checks, off-by-ones and missing NUL terminators in xmlBuf and xmlBuffer
    
    In broad strokes, this does the following:
    - Do not include the NUL terminator byte for lengths returned
      from functions.  This lets functions be more defensive.
    - Set error messages when returning early due to out-of-memory
      or buffer-too-large errors.
    - Set NUL terminator consistently on buffer boundaries before
      returning.
    - Add a few more integer overflow checks.
    
    * buf.c:
    (xmlBufGrowInternal):
    - Do not include NUL terminator byte when returning length.
    - Always set NUL terminator at the end of the new buffer length
      before returning.
    - Call xmlBufMemoryError() when the buffer size would overflow.
    - Account for NUL terminator byte when using XML_MAX_TEXT_LENGTH.
    - Always set NUL terminator at the end of the current buffer
      after resizing the buffer.
    (xmlBufAddLen):
    - Return an error if the buffer does not have free space for the
      NUL terminator byte.
    (xmlBufAvail):
    - Do not include the NUL terminator byte in the length returned.
      (See changes to encoding.c and xmlIO.c.)
    (xmlBufResize):
    - Move setting of NUL terminator to common code.  More than one
      path through the function failed to set it.
    (xmlBufAdd):
    - Call xmlBufMemoryError() when the buffer size would overflow.
    (xmlBufAddHead):
    - Set NUL terminator before returning early when shifting
      contents.
    - Add overflow checks similar to those in xmlBufAdd().
    
    * encoding.c:
    (xmlCharEncFirstLineInput):
    (xmlCharEncInput):
    (xmlCharEncOutput):
    - No longer need to subtract one from the return value of
      xmlBufAvail() since the function does this now.
    
    * testchar.c:
    (testCharRanges):
    - Pass the string length without the NUL terminator.
    
    * tree.c:
    (xmlBufferGrow):
    - Do not include NUL terminator byte when returning length.
    - Always set NUL terminator at the end of the new buffer length
      before returning.
    - Call xmlTreeErrMemory() when the buffer size would overflow.
    - Always set NUL terminator at the end of the current buffer
      after resizing the buffer.
    (xmlBufferDump):
    - Change type of the return variable to match fwrite().
    - Clamp return value to INT_MAX to prevent overflow.
    (xmlBufferResize):
    - Update error message in xmlTreeErrMemory() to be consistent
      with other similar messages.
    - Fix double semi-colon typos from commit 6c283d83.
    - Move setting of NUL terminator to common code.  More than one
      path through the function failed to set it.
    (xmlBufferAdd):
    - Call xmlTreeErrMemory() when the buffer size would overflow.
    (xmlBufferAddHead):
    - Set NUL terminator before returning early when shifting
      contents.
    - Add overflow checks similar to those in xmlBufferAdd().
    
    * xmlIO.c:
    (xmlOutputBufferWriteEscape):
    - No longer need to subtract one from the return value of
      xmlBufAvail() since the function does this now.

 buf.c      | 43 +++++++++++++++++++++++++++----------------
 encoding.c | 12 +++---------
 testchar.c |  2 +-
 tree.c     | 40 +++++++++++++++++++++++++++-------------
 xmlIO.c    |  2 +-
 5 files changed, 59 insertions(+), 40 deletions(-)
---
diff --git a/buf.c b/buf.c
index 2b1fd793..daf54d8a 100644
--- a/buf.c
+++ b/buf.c
@@ -435,10 +435,14 @@ xmlBufGrowInternal(xmlBufPtr buf, size_t len) {
     CHECK_COMPAT(buf)
 
     if (buf->alloc == XML_BUFFER_ALLOC_IMMUTABLE) return(0);
-    if (len < buf->size - buf->use)
-        return(buf->size - buf->use);
-    if (len > SIZE_MAX - buf->use)
+    if (len < buf->size - buf->use) {
+        buf->content[buf->use + len] = 0;
+        return(buf->size - buf->use - 1);
+    }
+    if (len > SIZE_MAX - buf->use - 1) {
+        xmlBufMemoryError(buf, "growing buffer past SIZE_MAX");
         return(0);
+    }
 
     if (buf->size > (size_t) len) {
         size = buf->size > SIZE_MAX / 2 ? SIZE_MAX : buf->size * 2;
@@ -451,7 +455,7 @@ xmlBufGrowInternal(xmlBufPtr buf, size_t len) {
         /*
         * Used to provide parsing limits
         */
-        if ((buf->use + len >= XML_MAX_TEXT_LENGTH) ||
+        if ((buf->use + len + 1 >= XML_MAX_TEXT_LENGTH) ||
            (buf->size >= XML_MAX_TEXT_LENGTH)) {
            xmlBufMemoryError(buf, "buffer error: text too long\n");
            return(0);
@@ -478,8 +482,10 @@ xmlBufGrowInternal(xmlBufPtr buf, size_t len) {
        buf->content = newbuf;
     }
     buf->size = size;
+    buf->content[buf->use] = 0;
+    buf->content[buf->use + len] = 0;
     UPDATE_COMPAT(buf)
-    return(buf->size - buf->use);
+    return(buf->size - buf->use - 1);
 }
 
 /**
@@ -609,14 +615,11 @@ xmlBufAddLen(xmlBufPtr buf, size_t len) {
     if ((buf == NULL) || (buf->error))
         return(-1);
     CHECK_COMPAT(buf)
-    if (len > (buf->size - buf->use))
+    if (len >= (buf->size - buf->use))
         return(-1);
     buf->use += len;
+    buf->content[buf->use] = 0;
     UPDATE_COMPAT(buf)
-    if (buf->size > buf->use)
-        buf->content[buf->use] = 0;
-    else
-        return(-1);
     return(0);
 }
 
@@ -698,7 +701,7 @@ xmlBufAvail(const xmlBufPtr buf)
         return 0;
     CHECK_COMPAT(buf)
 
-    return(buf->size - buf->use);
+    return((buf->size > buf->use) ? (buf->size - 1) - buf->use : 0);
 }
 
 /**
@@ -802,7 +805,6 @@ xmlBufResize(xmlBufPtr buf, size_t size)
            /* move data back to start */
            memmove(buf->contentIO, buf->content, buf->use);
            buf->content = buf->contentIO;
-           buf->content[buf->use] = 0;
            buf->size += start_buf;
        } else {
            rebuf = (xmlChar *) xmlRealloc(buf->contentIO, start_buf + newSize);
@@ -828,7 +830,6 @@ xmlBufResize(xmlBufPtr buf, size_t size)
            if (rebuf != NULL) {
                memcpy(rebuf, buf->content, buf->use);
                xmlFree(buf->content);
-               rebuf[buf->use] = 0;
            }
        }
        if (rebuf == NULL) {
@@ -838,6 +839,7 @@ xmlBufResize(xmlBufPtr buf, size_t size)
        buf->content = rebuf;
     }
     buf->size = newSize;
+    buf->content[buf->use] = 0;
     UPDATE_COMPAT(buf)
 
     return 1;
@@ -879,9 +881,12 @@ xmlBufAdd(xmlBufPtr buf, const xmlChar *str, int len) {
     if (len < 0) return -1;
     if (len == 0) return 0;
 
+    /* Note that both buf->size and buf->use can be zero here. */
     if ((size_t) len >= buf->size - buf->use) {
-        if ((size_t) len >= SIZE_MAX - buf->use)
+        if ((size_t) len >= SIZE_MAX - buf->use) {
+            xmlBufMemoryError(buf, "growing buffer past SIZE_MAX");
             return(-1);
+        }
         needSize = buf->use + len + 1;
        if (buf->alloc == XML_BUFFER_ALLOC_BOUNDED) {
            /*
@@ -957,12 +962,18 @@ xmlBufAddHead(xmlBufPtr buf, const xmlChar *str, int len) {
             memmove(&buf->content[0], str, len);
            buf->use += len;
            buf->size += len;
+           buf->content[buf->use] = 0;
            UPDATE_COMPAT(buf)
            return(0);
        }
     }
-    needSize = buf->use + len + 2;
-    if (needSize > buf->size){
+    /* Note that both buf->size and buf->use can be zero here. */
+    if ((unsigned) len >= buf->size - buf->use) {
+        if ((unsigned) len >= UINT_MAX - buf->use) {
+            xmlBufMemoryError(buf, "growing buffer past UINT_MAX");
+            return(-1);
+        }
+        needSize = buf->use + len + 1;
        if (buf->alloc == XML_BUFFER_ALLOC_BOUNDED) {
            /*
             * Used to provide parsing limits
diff --git a/encoding.c b/encoding.c
index 0a820498..1b5ae9f3 100644
--- a/encoding.c
+++ b/encoding.c
@@ -2197,7 +2197,7 @@ xmlCharEncFirstLineInput(xmlParserInputBufferPtr input, int len)
     toconv = xmlBufUse(in);
     if (toconv == 0)
         return (0);
-    written = xmlBufAvail(out) - 1; /* count '\0' */
+    written = xmlBufAvail(out);
     /*
      * echo '<?xml version="1.0" encoding="UCS4"?>' | wc -c => 38
      * 45 chars should be sufficient to reach the end of the encoding
@@ -2215,7 +2215,7 @@ xmlCharEncFirstLineInput(xmlParserInputBufferPtr input, int len)
     }
     if (toconv * 2 >= written) {
         xmlBufGrow(out, toconv * 2);
-        written = xmlBufAvail(out) - 1;
+        written = xmlBufAvail(out);
     }
     if (written > 360)
         written = 360;
@@ -2307,13 +2307,9 @@ xmlCharEncInput(xmlParserInputBufferPtr input, int flush)
     if ((toconv > 64 * 1024) && (flush == 0))
         toconv = 64 * 1024;
     written = xmlBufAvail(out);
-    if (written > 0)
-        written--; /* count '\0' */
     if (toconv * 2 >= written) {
         xmlBufGrow(out, toconv * 2);
         written = xmlBufAvail(out);
-        if (written > 0)
-            written--; /* count '\0' */
     }
     if ((written > 128 * 1024) && (flush == 0))
         written = 128 * 1024;
@@ -2495,8 +2491,6 @@ xmlCharEncOutput(xmlOutputBufferPtr output, int init)
 retry:
 
     written = xmlBufAvail(out);
-    if (written > 0)
-        written--; /* count '\0' */
 
     /*
      * First specific handling of the initialization call
@@ -2525,7 +2519,7 @@ retry:
         toconv = 64 * 1024;
     if (toconv * 4 >= written) {
         xmlBufGrow(out, toconv * 4);
-        written = xmlBufAvail(out) - 1;
+        written = xmlBufAvail(out);
     }
     if (written > 256 * 1024)
         written = 256 * 1024;
diff --git a/testchar.c b/testchar.c
index bd1a8d64..7132c921 100644
--- a/testchar.c
+++ b/testchar.c
@@ -607,7 +607,7 @@ static int testCharRanges(void) {
         fprintf(stderr, "Failed to allocate parser context\n");
        return(1);
     }
-    buf = xmlParserInputBufferCreateStatic(data, sizeof(data),
+    buf = xmlParserInputBufferCreateStatic(data, sizeof(data) - 1,
                                            XML_CHAR_ENCODING_NONE);
     if (buf == NULL) {
         fprintf(stderr, "Failed to allocate input buffer\n");
diff --git a/tree.c b/tree.c
index 689a6b66..b1030c92 100644
--- a/tree.c
+++ b/tree.c
@@ -7344,10 +7344,14 @@ xmlBufferGrow(xmlBufferPtr buf, unsigned int len) {
     if (buf == NULL) return(-1);
 
     if (buf->alloc == XML_BUFFER_ALLOC_IMMUTABLE) return(0);
-    if (len < buf->size - buf->use)
+    if (len < buf->size - buf->use) {
+        buf->content[buf->use + len] = 0;
         return(0);
-    if (len > UINT_MAX - buf->use)
+    }
+    if (len > UINT_MAX - buf->use - 1) {
+        xmlTreeErrMemory("growing buffer past UINT_MAX");
         return(-1);
+    }
 
     if (buf->size > (size_t) len) {
         size = buf->size > UINT_MAX / 2 ? UINT_MAX : buf->size * 2;
@@ -7375,7 +7379,9 @@ xmlBufferGrow(xmlBufferPtr buf, unsigned int len) {
        buf->content = newbuf;
     }
     buf->size = size;
-    return(buf->size - buf->use);
+    buf->content[buf->use] = 0;
+    buf->content[buf->use + len] = 0;
+    return(buf->size - buf->use - 1);
 }
 
 /**
@@ -7388,7 +7394,7 @@ xmlBufferGrow(xmlBufferPtr buf, unsigned int len) {
  */
 int
 xmlBufferDump(FILE *file, xmlBufferPtr buf) {
-    int ret;
+    size_t ret;
 
     if (buf == NULL) {
 #ifdef DEBUG_BUFFER
@@ -7407,7 +7413,7 @@ xmlBufferDump(FILE *file, xmlBufferPtr buf) {
     if (file == NULL)
        file = stdout;
     ret = fwrite(buf->content, sizeof(xmlChar), buf->use, file);
-    return(ret);
+    return(ret > INT_MAX ? INT_MAX : (int)ret);
 }
 
 /**
@@ -7472,7 +7478,7 @@ xmlBufferResize(xmlBufferPtr buf, unsigned int size)
         return 1;
 
     if (size > UINT_MAX - 10) {
-        xmlTreeErrMemory("growing buffer");
+        xmlTreeErrMemory("growing buffer past UINT_MAX");
         return 0;
     }
 
@@ -7494,7 +7500,7 @@ xmlBufferResize(xmlBufferPtr buf, unsigned int size)
            }
            break;
        case XML_BUFFER_ALLOC_EXACT:
-           newSize = (size > UINT_MAX - 10 ? UINT_MAX : size + 10);;
+           newSize = (size > UINT_MAX - 10 ? UINT_MAX : size + 10);
            break;
         case XML_BUFFER_ALLOC_HYBRID:
             if (buf->use < BASE_BUFFER_SIZE)
@@ -7512,7 +7518,7 @@ xmlBufferResize(xmlBufferPtr buf, unsigned int size)
             break;
 
        default:
-           newSize = (size > UINT_MAX - 10 ? UINT_MAX : size + 10);;
+           newSize = (size > UINT_MAX - 10 ? UINT_MAX : size + 10);
            break;
     }
 
@@ -7523,7 +7529,6 @@ xmlBufferResize(xmlBufferPtr buf, unsigned int size)
            /* move data back to start */
            memmove(buf->contentIO, buf->content, buf->use);
            buf->content = buf->contentIO;
-           buf->content[buf->use] = 0;
            buf->size += start_buf;
        } else {
            rebuf = (xmlChar *) xmlRealloc(buf->contentIO, start_buf + newSize);
@@ -7549,7 +7554,6 @@ xmlBufferResize(xmlBufferPtr buf, unsigned int size)
            if (rebuf != NULL) {
                memcpy(rebuf, buf->content, buf->use);
                xmlFree(buf->content);
-               rebuf[buf->use] = 0;
            }
        }
        if (rebuf == NULL) {
@@ -7559,6 +7563,7 @@ xmlBufferResize(xmlBufferPtr buf, unsigned int size)
        buf->content = rebuf;
     }
     buf->size = newSize;
+    buf->content[buf->use] = 0;
 
     return 1;
 }
@@ -7598,9 +7603,12 @@ xmlBufferAdd(xmlBufferPtr buf, const xmlChar *str, int len) {
     if (len < 0) return -1;
     if (len == 0) return 0;
 
+    /* Note that both buf->size and buf->use can be zero here. */
     if ((unsigned) len >= buf->size - buf->use) {
-        if ((unsigned) len >= UINT_MAX - buf->use)
+        if ((unsigned) len >= UINT_MAX - buf->use) {
+            xmlTreeErrMemory("growing buffer past UINT_MAX");
             return XML_ERR_NO_MEMORY;
+        }
         needSize = buf->use + len + 1;
         if (!xmlBufferResize(buf, needSize)){
            xmlTreeErrMemory("growing buffer");
@@ -7665,11 +7673,17 @@ xmlBufferAddHead(xmlBufferPtr buf, const xmlChar *str, int len) {
             memmove(&buf->content[0], str, len);
            buf->use += len;
            buf->size += len;
+           buf->content[buf->use] = 0;
            return(0);
        }
     }
-    needSize = buf->use + len + 2;
-    if (needSize > buf->size){
+    /* Note that both buf->size and buf->use can be zero here. */
+    if ((unsigned) len >= buf->size - buf->use) {
+        if ((unsigned) len >= UINT_MAX - buf->use) {
+            xmlTreeErrMemory("growing buffer past UINT_MAX");
+            return(-1);
+        }
+        needSize = buf->use + len + 1;
         if (!xmlBufferResize(buf, needSize)){
            xmlTreeErrMemory("growing buffer");
             return XML_ERR_NO_MEMORY;
diff --git a/xmlIO.c b/xmlIO.c
index 823a0dda..16c29f57 100644
--- a/xmlIO.c
+++ b/xmlIO.c
@@ -3547,7 +3547,7 @@ xmlOutputBufferWriteEscape(xmlOutputBufferPtr out, const xmlChar *str,
         * how many bytes to consume and how many bytes to store.
         */
        cons = len;
-       chunk = xmlBufAvail(out->buffer) - 1;
+       chunk = xmlBufAvail(out->buffer);
 
         /*
         * make sure we have enough room to save first, if this is
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]