Merge pull request #131 from arduino/fix-buf-size-float

Bugfix: Small buffer size causes buffer overrun when invoking String::Ctor with large float/double value.
arduino · Dec 10, 2020 · 78f3f41 · 78f3f41
2 parents 4f19438 + 3c76ef2
commit 78f3f41
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 11 deletions.
diff --git a/api/String.cpp b/api/String.cpp
@@ -20,14 +20,24 @@
 */
 
 #include "String.h"
+#include "Common.h"
 #include "itoa.h"
 #include "deprecated-avr-comp/avr/dtostrf.h"
 
+#include <float.h>
+
+namespace arduino {
+
 /*********************************************/
-/*  Constructors                             */
+/*  Static Member Initialisation             */
 /*********************************************/
 
-namespace arduino {
+size_t const String::FLT_MAX_DECIMAL_PLACES;
+size_t const String::DBL_MAX_DECIMAL_PLACES;
+
+/*********************************************/
+/*  Constructors                             */
+/*********************************************/
 
 String::String(const char *cstr)
 {
@@ -111,15 +121,19 @@ String::String(unsigned long value, unsigned char base)
 
 String::String(float value, unsigned char decimalPlaces)
 {
+	static size_t const FLOAT_BUF_SIZE = FLT_MAX_10_EXP + FLT_MAX_DECIMAL_PLACES + 1 /* '-' */ + 1 /* '.' */ + 1 /* '\0' */;
 	init();
-	char buf[33];
+	char buf[FLOAT_BUF_SIZE];
+	decimalPlaces = min(decimalPlaces, FLT_MAX_DECIMAL_PLACES);
 	*this = dtostrf(value, (decimalPlaces + 2), decimalPlaces, buf);
 }
 
 String::String(double value, unsigned char decimalPlaces)
 {
+	static size_t const DOUBLE_BUF_SIZE = DBL_MAX_10_EXP + DBL_MAX_DECIMAL_PLACES + 1 /* '-' */ + 1 /* '.' */ + 1 /* '\0' */;
 	init();
-	char buf[33];
+	char buf[DOUBLE_BUF_SIZE];
+	decimalPlaces = min(decimalPlaces, DBL_MAX_DECIMAL_PLACES);
 	*this = dtostrf(value, (decimalPlaces + 2), decimalPlaces, buf);
 }
 

diff --git a/api/String.h b/api/String.h
@@ -58,6 +58,9 @@ class String
 	typedef void (String::*StringIfHelperType)() const;
 	void StringIfHelper() const {}
 
+	static size_t const FLT_MAX_DECIMAL_PLACES = 10;
+	static size_t const DBL_MAX_DECIMAL_PLACES = FLT_MAX_DECIMAL_PLACES;
+
 public:
 	// constructors
 	// creates a copy of the initial value.

diff --git a/test/src/String/test_String.cpp b/test/src/String/test_String.cpp
@@ -6,6 +6,8 @@
  * INCLUDE
  **************************************************************************************/
 
+#include <float.h>
+
 #include <catch.hpp>
 
 #include <String.h>
@@ -80,16 +82,40 @@ TEST_CASE ("Testing String(unsigned long, unsigned char base = 10) constructor()
 
 TEST_CASE ("Testing String(float, unsigned char decimalPlaces = 2) constructor()", "[String-Ctor-10]")
 {
-  float const val = 1.234f;
-  arduino::String str(val);
-  REQUIRE(strcmp(str.c_str(), "1.23") == 0);
+  WHEN ("String::String (some float value)")
+  {
+    arduino::String str(1.234f);
+    REQUIRE(strcmp(str.c_str(), "1.23") == 0);
+  }
+  WHEN ("String::String (FLT_MAX)")
+  {
+    arduino::String str(FLT_MAX);
+    REQUIRE(strcmp(str.c_str(), "340282346638528859811704183484516925440.00") == 0);
+  }
+  WHEN ("String::String (-FLT_MAX)")
+  {
+    arduino::String str(-FLT_MAX);
+    REQUIRE(strcmp(str.c_str(), "-340282346638528859811704183484516925440.00") == 0);
+  }
 }
 
 TEST_CASE ("Testing String(double, unsigned char decimalPlaces = 2) constructor()", "[String-Ctor-11]")
 {
-  double const val = 5.678;
-  arduino::String str(val);
-  REQUIRE(strcmp(str.c_str(), "5.68") == 0);
+  WHEN ("String::String (some double value)")
+  {
+    arduino::String str(5.678);
+    REQUIRE(strcmp(str.c_str(), "5.68") == 0);
+  }
+  WHEN ("String::String (DBL_MAX)")
+  {
+    arduino::String str(DBL_MAX);
+    REQUIRE(strcmp(str.c_str(), "179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.00") == 0);
+  }
+  WHEN ("String::String (-DBL_MAX)")
+  {
+    arduino::String str(-DBL_MAX);
+    REQUIRE(strcmp(str.c_str(), "-179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.00") == 0);
+  }
 }
 
 TEST_CASE ("Testing String(const __FlashStringHelper) constructor() with invalid buffer", "[String-Ctor-12]")
@@ -131,4 +157,4 @@ TEST_CASE ("Testing String(String &&) with move(String &rhs) from larger to smal
   arduino::String str1("Arduino");
   str = static_cast<arduino::String&&>(str1);
   REQUIRE(str1.compareTo("Arduino") == 0);
-}
+}