Stream::parseFloat() fails if it reads too many digits

[edgar-bonet]

Stream::parseFloat() builds a long integer out of the digits it reads, skipping the decimal point, and then it scales it by the appropriate power of 0.1. For example, "3.14" is interpreted as 314 × 0.1².

If the user provides too many digits after the decimal point, the integer overflows, which triggers undefined behavior and makes the function return garbage. For example, "3.141592654" is read as 3141592654 × 0.1⁹, but the integer overflows to −1153374642, and Stream::parseFloat() returns −1.1534.

It could be argued that it is the user's fault, and that they should just not provide more digits than needed by the resolution of the float data type. The users, however, are not always in control of the format of the data they want to process. I have found an unsuspecting user bitten by this issue.

Test sketch: This uses a class that makes an input Stream out of a string, in order to not be dependent on the serial monitor.

// Stream in a C string.
class StrStream : public Stream {
public:
    StrStream(const char *s_in) : s(s_in) {}
    size_t write(uint8_t) { return 0; }
    int available() { return strlen(s); }
    int read() { return *s ? *s++ : -1; }
    int peek() { return *s ? *s : -1; }
private:
    const char *s;
};

void setup() {
    Serial.begin(9600);
    Serial.println(StrStream("3.1415927 ").parseFloat());
    Serial.println(StrStream("3.14159265 ").parseFloat());
    Serial.println(StrStream("3.141592654 ").parseFloat());
    Serial.println(StrStream("3.1415926536 ").parseFloat());
}

void loop(){}

Output:

3.14
3.14
-1.15
0.14

[edgar-bonet]

I see two options for fixing this issue:

1. Ignore the extra digits.

   If the current value is larger than (LONG_MAX-9)/10, then adding a digit may make it overflow. Since we have already more digits than needed for the precision of a float, we may just ignore those extra digits.

   I did a test on an Arduino Uno. The test shows that this strategy costs 18 bytes of flash and 8 CPU cycles per digit (i.e. about 2% extra processing time). An unfortunate side effect is that integers in the range [2147483640, 2147483647] (the 8 largest long values), which are parsed correctly in the current implementation, would be parsed incorrectly. This corner case could be handled, but that would slightly increase the cost in both flash and time.

2. Read directly into a float.

   Instead of building an integer and then converting it to a float, we could parse the input directly into a float.

   This strategy has the great advantage that it parses correctly numbers way larger than LONG_MAX, but my test shows that it costs nearly 50% extra processing time, although the flash usage is 28 bytes lower.

Any thoughts? Opinions? Should this be fixed? Which strategy brings the right (in the Arduino context) balance between cost and benefit?

Would the maintainers consider a pull-request?

[aentinger]

Hi @edgar-bonet 👋 Given the fact that we now got unit tests automatically built and executed upon each push to the repository - could you create a PR extending test_parseFloat.cpp to include a failing test for this specific issue?

[edgar-bonet]

Hello @aentinger, and thank you for your response.

I am not familiar with this form of test-driven development, but it looks to me like a very interesting approach! I managed to make the test-suite run on my computer, and wrote the pull request for this issue: #133

[edgar-bonet]

Fixed by pull request #133 (merged). Thanks @aentinger!