From dfb0dee773bb7e9f44348ca518292435e9148eba Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 11:14:56 +0100 Subject: 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. --- cores/arduino/WString.cpp | 1 - 1 file changed, 1 deletion(-) (limited to 'cores/arduino/WString.cpp') diff --git a/cores/arduino/WString.cpp b/cores/arduino/WString.cpp index ed880ce..a0a3b6c 100644 --- a/cores/arduino/WString.cpp +++ b/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); } -- cgit v1.2.3-18-g5258 From b2729a515607f1b0108d38b816430797f558c57f Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 11:26:30 +0100 Subject: 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. --- cores/arduino/WString.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cores/arduino/WString.cpp') diff --git a/cores/arduino/WString.cpp b/cores/arduino/WString.cpp index a0a3b6c..e4bed03 100644 --- a/cores/arduino/WString.cpp +++ b/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); -- cgit v1.2.3-18-g5258 From 35a84769d4dbc0670550ea2abde83bd33dc28729 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Sun, 16 Mar 2014 11:34:41 +0100 Subject: 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. --- cores/arduino/WString.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'cores/arduino/WString.cpp') diff --git a/cores/arduino/WString.cpp b/cores/arduino/WString.cpp index e4bed03..f494094 100644 --- a/cores/arduino/WString.cpp +++ b/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){ -- cgit v1.2.3-18-g5258 From fde95cf5b5ace06f497842f9329d5207c4834898 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 24 Apr 2014 22:57:27 +0200 Subject: 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); --- cores/arduino/WString.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cores/arduino/WString.cpp') diff --git a/cores/arduino/WString.cpp b/cores/arduino/WString.cpp index f494094..dcd469d 100644 --- a/cores/arduino/WString.cpp +++ b/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'; -- cgit v1.2.3-18-g5258