From 2b90124e3d20d0da2f3217527565a0edc465352a Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 11:14:56 +0100 Subject: [PATCH 1/4] Remove unneeded check in String::remove(unsigned int) This check already happens in the remove(unsigned int, unsigned int) method that is caled, so there is no need to also check this here. --- hardware/arduino/avr/cores/arduino/WString.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/hardware/arduino/avr/cores/arduino/WString.cpp b/hardware/arduino/avr/cores/arduino/WString.cpp index ed880ce2b90..a0a3b6cae13 100644 --- a/hardware/arduino/avr/cores/arduino/WString.cpp +++ b/hardware/arduino/avr/cores/arduino/WString.cpp @@ -684,7 +684,6 @@ void String::replace(const String& find, const String& replace) } void String::remove(unsigned int index){ - if (index >= len) { return; } int count = len - index; remove(index, count); } From 2068f88a21b03957d59467b22bb0a423204eae99 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 11:26:30 +0100 Subject: [PATCH 2/4] 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. --- hardware/arduino/avr/cores/arduino/WString.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hardware/arduino/avr/cores/arduino/WString.cpp b/hardware/arduino/avr/cores/arduino/WString.cpp index a0a3b6cae13..e4bed03e6ba 100644 --- a/hardware/arduino/avr/cores/arduino/WString.cpp +++ b/hardware/arduino/avr/cores/arduino/WString.cpp @@ -691,7 +691,7 @@ void String::remove(unsigned int index){ void String::remove(unsigned int index, unsigned int count){ if (index >= len) { return; } if (count <= 0) { return; } - if (index + count > len) { count = len - index; } + if (count > len - index) { count = len - index; } char *writeTo = buffer + index; len = len - count; strncpy(writeTo, buffer + index + count,len - index); From 86015f441e932b43a5a72f540d7256def5632602 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 11:34:41 +0100 Subject: [PATCH 3/4] Simplify String::remove(unsigned int) Previously, this method calculated the length of the string from the given index onwards. However, the other remove() method called already contains code for this calculation, which is used when the count passed in is too big. This means we can just pass in a very big count that is guaranteed to point past the end of the string, shrinking the remove method by a few bytes. --- hardware/arduino/avr/cores/arduino/WString.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/WString.cpp b/hardware/arduino/avr/cores/arduino/WString.cpp index e4bed03e6ba..f494094bddd 100644 --- a/hardware/arduino/avr/cores/arduino/WString.cpp +++ b/hardware/arduino/avr/cores/arduino/WString.cpp @@ -684,8 +684,10 @@ void String::replace(const String& find, const String& replace) } void String::remove(unsigned int index){ - int count = len - index; - remove(index, count); + // Pass the biggest integer as the count. The remove method + // below will take care of truncating it at the end of the + // string. + remove(index, (unsigned int)-1); } void String::remove(unsigned int index, unsigned int count){ From 04dba1e46f030e7e59e6bf27d042347a0acc06ee Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 24 Apr 2014 22:57:27 +0200 Subject: [PATCH 4/4] Fix off-by-one in String::substring When checking the `left` argument, it previously allowed having left == len. However, this means the substring starts one past the last character in the string and should return the empty string. In practice, this already worked correctly, because buffer[len] contains the trailing nul, so it would (re)assign the empty string to `out`. However, fixing this check makes it a bit more logical, and prevents a fairly unlikely out-of-buffer write (to address 0x0) when calling substring on an invalidated String: String bar = (char*)NULL; bar.substring(0, 0); --- hardware/arduino/avr/cores/arduino/WString.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hardware/arduino/avr/cores/arduino/WString.cpp b/hardware/arduino/avr/cores/arduino/WString.cpp index f494094bddd..dcd469d7dd0 100644 --- a/hardware/arduino/avr/cores/arduino/WString.cpp +++ b/hardware/arduino/avr/cores/arduino/WString.cpp @@ -619,7 +619,7 @@ String String::substring(unsigned int left, unsigned int right) const left = temp; } String out; - if (left > len) return out; + if (left >= len) return out; if (right > len) right = len; char temp = buffer[right]; // save the replaced character buffer[right] = '\0';