[libxml2/fix-more-xmlBuf-and-xmlBuffer-issues: 8/8] Fix more overflow checks, off-by-ones and missing NUL terminators in xmlBuf and xmlBuffer
- From: David Kilzer <ddkilzer src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [libxml2/fix-more-xmlBuf-and-xmlBuffer-issues: 8/8] Fix more overflow checks, off-by-ones and missing NUL terminators in xmlBuf and xmlBuffer
- Date: Thu, 26 May 2022 01:32:02 +0000 (UTC)
commit 776e1c312dc273a031ef0c3f4d67ba702c5308d3
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.
* 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.
- 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 | 33 +++++++++++++++++++--------------
encoding.c | 12 +++---------
testchar.c | 2 +-
tree.c | 36 +++++++++++++++++++++++++-----------
xmlIO.c | 2 +-
5 files changed, 49 insertions(+), 36 deletions(-)
---
diff --git a/buf.c b/buf.c
index 0a798f59..a5df00bd 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);
}
/**
@@ -591,14 +597,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);
}
@@ -658,7 +661,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);
}
/**
@@ -762,7 +765,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);
@@ -788,7 +790,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) {
@@ -798,6 +799,7 @@ xmlBufResize(xmlBufPtr buf, size_t size)
buf->content = rebuf;
}
buf->size = newSize;
+ buf->content[buf->use] = 0;
UPDATE_COMPAT(buf)
return 1;
@@ -839,9 +841,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) {
/*
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 df17fa33..9ad7616f 100644
--- a/tree.c
+++ b/tree.c
@@ -7369,10 +7369,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;
@@ -7400,7 +7404,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);
}
/**
@@ -7413,7 +7419,7 @@ xmlBufferGrow(xmlBufferPtr buf, unsigned int len) {
*/
int
xmlBufferDump(FILE *file, xmlBufferPtr buf) {
- int ret;
+ size_t ret;
if (buf == NULL) {
#ifdef DEBUG_BUFFER
@@ -7432,7 +7438,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);
}
/**
@@ -7497,7 +7503,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;
}
@@ -7548,7 +7554,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);
@@ -7574,7 +7579,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) {
@@ -7584,6 +7588,7 @@ xmlBufferResize(xmlBufferPtr buf, unsigned int size)
buf->content = rebuf;
}
buf->size = newSize;
+ buf->content[buf->use] = 0;
return 1;
}
@@ -7623,9 +7628,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");
@@ -7690,11 +7698,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]