Skip to content

Fixes String(float) issue with Stack Smashing #6138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 17, 2022

Conversation

SuGlider
Copy link
Collaborator

Summary

The PR fixes an issue related to Stack Smashing when trying to execute this sketch:

void setup() {
  Serial.begin(115200);
  String s = String(1e32f, 0);
  Serial.println(s);
  Serial.println(String(-1.0f, 30));
  Serial.println(String(0.0f, 31));
}

void loop() {
  // put your main code here, to run repeatedly:
}

The wrong output is:

Stack smashing protect failure!

abort() was called at PC 0x400d74a0 on core 1

Backtrace:0x40082e19:0x3ffb26800x40087a21:0x3ffb26a0 0x4008c465:0x3ffb26c0 0x400d74a0:0x3ffb2740 0x400d1731:0x3ffb2760 0x400d0f73:0x3ffb27c0 0x400d17de:0x3ffb2820 

Expected correct output is:

100000003318135366470187364029698
-1.000000000000000000000000000000
0.0000000000000000000000000000000

Impact

None.

Related links

Fixes #5873

@MitchBradley
Copy link
Contributor

The proposed change does not fix the general problem. It fixes the specific minimal test case where the float number is 1e32, but all it would take to break it again is to change 1e32 to 1e33. As stated in the original report, a general fix would require a buffer size of about 80 characters plus additional code to limit the precision value. Furthermore, the general problem also exists in the double precision version of the function, where the buffer size would need to be much larger still.

All of that is clearly described in the original issue report.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Jan 14, 2022

I see. I tested other implementations of Arduino and it seems that all of them have the same problem.

None is user safe.

Your propose would be a user safe implementation that makes sure that it won't break, no matter parameters used.

It is an edge case testing.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Jan 14, 2022

@MitchBradley

I had already considered it and just sent a new commit.
It'll limit the String to 33 characters, no matter the float number.
Now any usage won't reset the ESP32. Even the sketch below:

void setup() {
  Serial.begin(115200);
  
  Serial.printf("\nTesting potential issues with Float-String:\n\n");
  String s = String(1e32f, 0);
  Serial.println(s);
  Serial.println(String(-1.0f, 30));
  Serial.println(String(0.0f, 31));
  Serial.println(String(0.0f, 100));
  Serial.println(String(-234223.4f, 32));
}

void loop() {
  // put your main code here, to run repeatedly:
}

Output of this sketch:

ets Jul 29 2019 12:21:46

rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:1
load:0x3fff0030,len:1324
ho 0 tail 12 room 4
load:0x40078000,len:13508
load:0x40080400,len:3604
entry 0x400805f0

Testing potential issues with Float-String:

100000003318135351409612647563264
-1.000000000000000000000000000000
0.0000000000000000000000000000000
0.0000000000000000000000000000000
-234223.4062500000000000000000000

@MitchBradley
Copy link
Contributor

Not crashing is better than crashing, but silently giving wrong answers is not good either. This is a valid single precision floating point number:

-340282346638528859811704183484516925440

It is the most negative number than can be exactly represented by a float. It is 40 digits without a decimal point or postdecimal digits. There is no way to represent that in 33 characters without either switching to exponential notation or completely losing the meaning.

Unfortunately, the Arduino String Reference does not say what is supposed to happen for valid but inconveniently large numbers. It would be plausible to convert to exponential notation after a certain size, and to truncate the number of decimal places, or to throw a documented exception, but crashing or silently giving blatantly wrong results is not a reasonable behavior.

The fundamental problem is that whoever designed String(float, precision) did not think through all the implications.

The simplest solution for single floats is to limit the precision to 38 and use a buffer of 80 characters, thus allowing 39 predecimal characters to represent the largest number, 38 postdecimal characters for the smallest number, 1 sign, 1 decimal point, and a null. That uses more RAM, but not that much more RAM, and it works.

For double floats the RAM usage is worse, requiring 524 bytes.

The other simple solution is to switch to exponential notation above some threshold - but you still have to limit the precision value. Exponential notation does not exactly meet the implied specification - but the spec is so vague that you might get away with it.

The real thing that needs to happen is that whoever is responsible for the Arduino specification should address the issue and fully specify the behavior of String({float/double}, precision).

@SuGlider
Copy link
Collaborator Author

@MitchBradley

I did some experiments. The idea is to find out how ESP32 different routines can print a number such as 1/3.

First I just used C sprintf():

    char fstr[512];
    double d = 1/3.0f;
    sprintf(fstr, "%70.70f", d);
    Serial.printf("\n[%s]\n", fstr);

Output is:
[0.3333333432674407958984375000000000000000000000000000000000000000000000]

Then I modified the String class double method to support a large number of characters:

String::String(double value, unsigned char decimalPlaces) {
    init();
    char buf[512];
    *this = dtostrf(value, (decimalPlaces + 2), decimalPlaces, buf);
}

and also changed the dostrf() back to the original ESP32/ESP8266 code.

The result of the excution of

      Serial.println(String((double)1/3.0f, 99));

is 0.333333333333333303727386009995825588703155517578125000000000000000000000000000000000000000000000000

Lastly, I tried to print -340282346638528859811704183484516925440 using sprintf() and the original ESP32 dtostrf().

    char fstr[512];
    double d = -340282346638528859811704183484516925440;
    sprintf(fstr, "%70.70f", d);
    Serial.printf("\n[%s]\n", fstr); 
    Serial.println();
    Serial.println(String(d, 99));

The output:

[0.0000000000000000000000000000000000000000000000000000000000000000000000]

0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

I think that C is not Fortran... So maybe, Arduino may never be suited for what you are looking for.

@SuGlider
Copy link
Collaborator Author

I finally tried this:

  double d = 0.333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333;   // forcing compiler to reach 1 / 3.0f;
  sprintf(fstr, "%70.70f", d);
  Serial.println();
  Serial.printf("\n[%s]\n", fstr);
  Serial.println(String((double)0.333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333, 99));
  Serial.println();

Results:

[0.3333333333333333148296162562473909929394721984863281250000000000000000]
0.333333333333333303727386009995825588703155517578125000000000000000000000000000000000000000000000000

@SuGlider
Copy link
Collaborator Author

I think that given the results of these experiments, the user shall find out a way to achieve the desired outcome when treating floats, double or any sort of precision decimal point numbers.

Arduino String is not designed for this purpose. It's just a way to convert different types to the String Class.

@MitchBradley
Copy link
Contributor

The results from the 1/3 experiments are exactly as expected. In IEEE 754 single precision floating point representation, [0.33333334326744079589843750...] is the closest you can get to the exact value. Single precision floating point is represented by 23 binary fractional bits. Since a binary number has only powers of 2, a number that has a factor of 3 cannot be represented exactly.

@SuGlider
Copy link
Collaborator Author

The results from the 1/3 experiments are exactly as expected. In IEEE 754 single precision floating point representation, [0.33333334326744079589843750...] is the closest you can get to the exact value. Single precision floating point is represented by 23 binary fractional bits. Since a binary number has only powers of 2, a number that has a factor of 3 cannot be represented exactly.

Interesting because #6138 (comment) represents better 1/3.

@MitchBradley
Copy link
Contributor

Similarly, for the double case, the underlying representation has 52 fraction bits. The error for a number than cannot be represented exactly will be smaller, but still nonzero. In single precision, the value will be accurate to between 6 and 9 decimal digits, and double precision to between 15 and 17 decimal digits. All of this is explained in the following Wikipedia articles: https://en.wikipedia.org/wiki/Single-precision_floating-point_format and https://en.wikipedia.org/wiki/Double-precision_floating-point_format

@MitchBradley
Copy link
Contributor

The "better representation" that you mention is the double precision case where there are more fractional bits available to encode the number.

@MitchBradley
Copy link
Contributor

But please note: the accuracy of 6-9 decimal digits (single precision i.e. float) and 15-17 digits (double precision i.e. double) is a different thing than the exponent.

In the situation that we are worried about, which is the conversion of a floating point number to a String that is NOT in exponential notation, we must worry about both. Let's consider two different situations:
1/3 with precision 38. In this case, as you have shown, the first 7 digits are correct, then they start to diverge (0.333333343xxx). So you might think that it is useless to ask for more than 7 digits of precision - BUT
What if the number is instead, say, 1E-38. Then, if you ask for 38 precision bits, the answer would be 0.00000000000000000000000000000000000001 . So it is useful to ask for more than 7 digits of precision when you are not using exponential notation!
In fact, since the fraction and mantissa are separate, you can go even smaller. You can ask for 0.0000001E-38 which is approximately representable, so the non-exponential-notation String would be 0.0<43 more zeroes>1 .

@MitchBradley
Copy link
Contributor

A properly-designed library will give results that are as accurate as possible within the constraints of the underlying number system. This has nothing to do with C vs FORTRAN because the underlying number system - IEEE 754 floating point - is the same in both cases. Sprintf, given a suitably large buffer and the appropriate arguments, does the right thing - it converts the underlying IEEE number to the corresponding string representation. That is the only right answer.

Unfortunately, the corresponding string representation is long in some cases, so long that it could strain the resources of an 8-bit micro. It is not too long for a 32-bit processor, especially not going forward.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Jan 16, 2022

My point of view about the whole discussion is:

There are 2 topics here:

1- Solve the issue with Stack Smashing and the reset.

For this issue, at a first glance, this PR does the job.

2- Getting Arduino String Class to print a float or double with the maximum precision possible.

No solution at this time.

@MitchBradley, if you want so, you or any other contributor can submit a PR to achieve it.

Sumary

I consider this PR ready for being reviewed regarding the first item here listed.

@me-no-dev @pedrominatel @P-R-O-C-H-Y @PilnyTomas @atanisoft
@igrr

please let me know your thoughts.

@dpharris
Copy link

dpharris commented Jan 16, 2022 via email

@SuGlider
Copy link
Collaborator Author

SuGlider commented Jan 16, 2022

@MitchBradley
I made a review and change on the String(float|double) algotithm.
Please test it and let me know.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Jan 16, 2022

I have just finished testing a general solution:

String::String(float value, unsigned int decimalPlaces) {
    init();
    // The maximum number of predecimal digits for an IEEE 754
    // single precision float is 39, plus space for a possible
    // minus sign, a decimal point, and a null character.
    // The number of postdecimal digits is given by the argument.
    // The actual value that is placed in the string will be copied
    // into newly-allocated memory by the assignment operator, then
    // the temporary buffer can be safely freed.
    char *buf = (char*)malloc(decimalPlaces + 42);
    *this = dtostrf(value, (decimalPlaces + 2), decimalPlaces, buf);
    free(buf);
}

String::String(double value, unsigned int decimalPlaces) {
    init();
    // The maximum number of predecimal digits for an IEEE 754
    // double precision float is 309, plus space for a possible
    // minus sign, a decimal point, and a null character.
    // The number of postdecimal digits is given by the argument.
    // The actual value that is placed in the string will be copied
    // into newly-allocated memory by the assignment operator, then
    // the temporary buffer can be safely freed.
    char *buf = (char*)malloc(decimalPlaces + 312);
    *this = dtostrf(value, (decimalPlaces + 2), decimalPlaces, buf);
}

Please post a complete PR for evaluation.
If your solution is better than the one here presented, we will consider it for sure!
Thanks!

@SuGlider
Copy link
Collaborator Author

@MitchBradley

This is the testing sketch and results that I tried with the latest commit:

void setup() {
  Serial.begin(115200);
  Serial.printf("\nTesting potential issues with Float-String:\n\n");
  Serial.flush();
  delay(1000);

  // some edge cases
  String s = String(1e38f, 0);
  Serial.println(s);
  Serial.println(String(1e39f, 0));
  // comon test cases
  Serial.println(String(1e32f, 0));
  Serial.println(String(-1.0f, 30));
  Serial.println(String(0.0f, 31));
  Serial.println(String(0.0f, 100));
  Serial.println(String(-234223.4f, 32));

  
  double f = 1.0 / 3.0;
  Serial.println(String(f, 100));

  char fstr[512];
  double d = 0.333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333;
  sprintf(fstr, "%70.70f", d);
  Serial.println();
  Serial.printf("\nWith sprintf => [%s] \nand below with String()\n", fstr);
  Serial.println(String((double)0.333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333, 99));
  Serial.println();
}


void loop() {
  // put your main code here, to run repeatedly:
}

Output:


Testing potential issues with Float-String:

99999996802856924650656260769173209088
inf
100000003318135351409612647563264
-1.000000000000000000000000000000000000000000000000000000000000
0.0000000000000000000000000000000000000000000000000000000000000
0.0000000000000000000000000000000000000000000000000000000000000
-234223.4062500000000000000000000000000000000000000000000000000
0.3333333333333333148296162562473909929394721984863281250000000


With sprintf => [0.3333333333333333148296162562473909929394721984863281250000000000000000] 
and below with String()
0.3333333333333333148296162562473909929394721984863281250000000


@MitchBradley
Copy link
Contributor

I observe the following about your test cases:

  1. There are no test cases where the number is large and the precision is also large - like say String(1e38, 40). I expect that you will say "why would you want 40 decimal places with a large number like 1e38"? My response is that, in most cases, the numerical value that is passed to String is not known in advance. The user could reasonably ask for 40 decimal places, expecting that the numbers will be very small so many decimal places are needed. Then, due to unexpected situations in the code, such as division by a small number or invalid input, the actual number turns out to be very large. In that situation, limiting the total string length to 64 will result in a confusing answer than makes no sense.
  2. The string length of 100 was truncated to a smaller size. While that might make sense if the specification said that the lengths larger than some number will be truncated, the specification does not say anything like that, so truncating is not as good as doing what the programmer asked.

I think that the malloc/free formulation, in conjunction with the dtostrf() change to allow more than 255 digits, does the best possible job of doing what the programmer asked for within the inherent characteristics of the IEEE number system.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Jan 16, 2022

@MitchBradley

I have commited your suggestion for the fix.
This is the sketch:

void setup() {
  Serial.begin(115200);
  Serial.printf("\nTesting potential issues with Float-String:\n\n");
  Serial.flush();
  delay(1000);

  Serial.println("==== MitchBradley test cases:");
  Serial.println(String(1e-38, 40));                // smallest normalized single
  Serial.println(String(-3.4e38, 40));              // largest single with extra precision digits
  Serial.println(String(1e-45, 45));                // smallest denormal single
  Serial.println(String((double)4.94e-324, 327));   // smallest denormal double
  Serial.println(String((double)-1e308, 0));        // Largest double
  Serial.println("==== MitchBradley test cases:");
}

void loop() {
  // put your main code here, to run repeatedly:
}

This is the output:

==== MitchBradley test cases:
0.0000000000000000000000000000000000000100
-340000000000000079936057773011270910501.4801025390625000000000000000000000000000
0.000000000000000000000000000000000000000000001
0.00000000000000000000000000000000000000000000000000000000000000000000000
-100000000000000022204460492503130808472633361816406250000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
==== MitchBradley test cases:

It seems there is a failure with Serial.println(String((double)4.94e-324, 327)); // smallest denormal double
Could you please let me know if there is something I didn't commit?

@MitchBradley
Copy link
Contributor

The problem with 4.94e-324 is caused by the fact that the precision argument is unsigned char so the largest possible value is 255, which is less than the requested precision of 327.

To fix that, it is necessary to make 3 changes:

  1. The precision argument to String(double, precision) must be unsigned int instead of unsigned char. This affects both WString.h and WString.cpp
  2. dtostrf() has the same problem, affecting both the width argument (signed char -> signed int) and the prec argument (unsigned char -> unsigned int), affecting stdlib_noniso.h and stdlib_noniso.c
  3. The rounding code in dtostrf() has this line:
    for (uint8_t i = 0; i < prec; ++i)
    which should have "unsigned int" instead of "uint8_t"

@SuGlider
Copy link
Collaborator Author

@MitchBradley
Excellent! Thank you for the support and for guiding this PR!

Everything is committed and it is now working as expected.
This is the output of the latest commit:

==== MitchBradley test cases:
0.0000000000000000000000000000000000000100
-340000000000000079936057773011270910501.4801025390625000000000000000000000000000
0.000000000000000000000000000000000000000000001
0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004940
-100000000000000022204460492503130808472633361816406250000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
==== MitchBradley test cases:

@MitchBradley
Copy link
Contributor

After width is changed from signed char to signed int, I think this PR will be ready.

The width problem might not show up in the case where dtostrf() is called from String(), but I think it could cause strange formatting for direct uses of dtostrf(). You need to be able to pass in width values that are larger than the prec value, and that is impossible if width is char while the prec value can be more than 127.

@SuGlider
Copy link
Collaborator Author

Perfect. All done. I think it shall be merged this week.
Thanks!

@me-no-dev me-no-dev merged commit 841599c into espressif:master Jan 17, 2022
@TD-er
Copy link
Contributor

TD-er commented Jan 18, 2022

I do understand where this 312 comes from:
https://github.com/espressif/arduino-esp32/pull/6138/files#diff-9489f13f36d43842447741801f6583431fc1c9afe186b52ceb17b1f9c0f2a9adR129
But can't we make it a bit more memory friendly by interpreting the given value and decimalPlaces?

For ESP32 it may not be an immediate problem as it does have a lot more memory compared to ESP8266, but I guess this will be added to the ESP8266 code too.
Also with fragmented memory, a malloc may take more time to find a suitable free block.

The exponent in the double is in the 12 most significant bits (including the sign).
This exponent is directly proportional to the nr of bytes needed.
But maybe some simple check to see if the (absolute) value is over 2^64 (some arbitrary value) can already make it more memory efficient in most calls.

@MitchBradley
Copy link
Contributor

Your suggestion is indeed possible, but I doubt that it would make much difference in practice. The excess memory will be freed immediately. A malloc/free of several hundred bytes is no more likely to cause fragmentation than one of a smaller size. If you are already so low on memory that it exhausts the heap, it is very likely that you will crash soon anyway from allocations for incoming packets. Regarding the performance, when you are converting a double, you are already engaged in a slow operation involving lots of software floating point computations, so a bit more heap activity is unlikely to be a huge factor.
So, yeah, it could be made a bit more efficient, but it might not be worth the increased complexity, testing effort, and possibility of making a mistake.

@TD-er
Copy link
Contributor

TD-er commented Jan 18, 2022

So, yeah, it could be made a bit more efficient, but it might not be worth the increased complexity, testing effort, and possibility of making a mistake.

I guess that last one is indeed the most important reason not to optimize.
Sure it will not increase fragmentation. My concern was mainly about performance.

TD-er added a commit to TD-er/ESPEasy that referenced this pull request Jan 21, 2022
TD-er added a commit to TD-er/ESPEasy that referenced this pull request Jan 22, 2022
@VojtechBartoska VojtechBartoska added this to the 2.0.3 milestone Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Stack smash with String(float, precision)
6 participants