Skip to content

Commit 2068f88

Browse files
Fix bounds check in String::remove()
Previously, if you passed in a very big index and/or count, the `index + count` could overflow, making the count be used as-is instead of being truncated (causing the string to be updated wrongly and potentially writing to arbitrary memory locations). We can rewrite the comparison to use `len - index` instead. Since we know that index < len, we are sure this subtraction does not overflow, regardless of what values of index and count we pass in. As an added bonus, the `len - index` value already needed be calculated inside the if, so this saves a few instructions in the generated code. To illustrate this problem, consider this code: String foo = "foo"; Serial.println(foo.length()); // Prints 3 foo.remove(1, 65535); // Should remove all but first character Serial.println(foo.length()); // Prints 4 without this patch Not shown in this is example is that some arbitrary memory is written as well.
1 parent 2b90124 commit 2068f88

File tree

1 file changed

+1
-1
lines changed

1 file changed

+1
-1
lines changed

hardware/arduino/avr/cores/arduino/WString.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -691,7 +691,7 @@ void String::remove(unsigned int index){
691691
void String::remove(unsigned int index, unsigned int count){
692692
if (index >= len) { return; }
693693
if (count <= 0) { return; }
694-
if (index + count > len) { count = len - index; }
694+
if (count > len - index) { count = len - index; }
695695
char *writeTo = buffer + index;
696696
len = len - count;
697697
strncpy(writeTo, buffer + index + count,len - index);

0 commit comments

Comments
 (0)