Make string concat, copy and move work on non-terminated strings

matthijskooijman · matthijskooijman · commit 59dd9950ff79 · 2021-01-25T10:04:20.000+01:00
Previously, these methods took a nul-terminated string and appended it
to the current buffer. The length argument (or length of the passed
String object) was used to allocate memory, but was not used when
actually copying the string. This meant that:

 - If the passed length was too short, or the string passed in was not
   nul-terminated, the buffer would overflow.
 - If the string passed in contained embedded nul characters (i.e,
   among the first length characters), any characters after the embedded
   nul would not be copied (but len would be updated to indicate they
   were).

In practice, neither of the above would occur, since the length passed is
always the known length of the string, usually as returned by strlen.
However, to make this method public, and to allow using this concat
method to pass in strings that are not nul-terminated, it should be
changed to be more robust.

This commit changes the method to use memcpy instead of strcpy, copying
exactly the number of bytes passed in. For the current calls to this
method, which pass a nul-terminated string, without embedded nul
characters and a correct length, this behaviour should not change.

However, this concat method can now also be used in the two cases
mentioned above. Non-nul-terminated strings now work as expected and for
strings with embedded newlines the entire string is copied as-is,
instead of leaving uninitialized memory after the embedded nul byte.

Note that a lot of operations will still only see the string up to the
embedded nul byte, but that's not really fixable unless we reimplement
functions like strcmp.
diff --git a/api/String.cpp b/api/String.cpp
@@ -192,7 +192,7 @@ String & String::copy(const char *cstr, unsigned int length)
 		return *this;
 	}
 	len = length;
-	strcpy(buffer, cstr);
+	memcpy(buffer, cstr, length);
 	return *this;
 }
 
@@ -212,7 +212,7 @@ void String::move(String &rhs)
 {
 	if (buffer) {
 		if (rhs && capacity >= rhs.len) {
-			strcpy(buffer, rhs.buffer);
+			memcpy(buffer, rhs.buffer, rhs.len);
 			len = rhs.len;
 			rhs.len = 0;
 			return;
@@ -284,7 +284,7 @@ unsigned char String::concat(const char *cstr, unsigned int length)
 	if (!cstr) return 0;
 	if (length == 0) return 1;
 	if (!reserve(newlen)) return 0;
-	strcpy(buffer + len, cstr);
+	memcpy(buffer + len, cstr, length);
 	len = newlen;
 	return 1;
 }

Original file line number	Diff line number	Diff line change
`@@ -192,7 +192,7 @@ String & String::copy(const char *cstr, unsigned int length)`
`192`	`192`	`return *this;`
`193`	`193`	`}`
`194`	`194`	`len = length;`
`195`		`- strcpy(buffer, cstr);`
	`195`	`+ memcpy(buffer, cstr, length);`
`196`	`196`	`return *this;`
`197`	`197`	`}`
`198`	`198`
`@@ -212,7 +212,7 @@ void String::move(String &rhs)`
`212`	`212`	`{`
`213`	`213`	`if (buffer) {`
`214`	`214`	`if (rhs && capacity >= rhs.len) {`
`215`		`- strcpy(buffer, rhs.buffer);`
	`215`	`+ memcpy(buffer, rhs.buffer, rhs.len);`
`216`	`216`	`len = rhs.len;`
`217`	`217`	`rhs.len = 0;`
`218`	`218`	`return;`
`@@ -284,7 +284,7 @@ unsigned char String::concat(const char *cstr, unsigned int length)`
`284`	`284`	`if (!cstr) return 0;`
`285`	`285`	`if (length == 0) return 1;`
`286`	`286`	`if (!reserve(newlen)) return 0;`
`287`		`- strcpy(buffer + len, cstr);`
	`287`	`+ memcpy(buffer + len, cstr, length);`
`288`	`288`	`len = newlen;`
`289`	`289`	`return 1;`
`290`	`290`	`}`