Arduino String bug in WString.cpp - buffer contains invalid pointer

[philbowles]

This is definitely what I'd call a "bug" but I'm not sure what can be done about it, short of try/throw/catch mechanism in all of the library which prints a nicer message than mine...although that at least would have saved me the last two days in finding this! (NOT using arduino strings, perhaps? that what I'm about to do...suggestions welcomed for best way of reading large file from SPIFFS in low-memory situations?)

To the problem...

When realloc fails and returns a zero value, we just stick the invalid pointer it straight into the buffer! The situation occured in my scenario where I was trying to read a SPIFFS file of 10k plus and though there was still plenty of heap space, the largest block available was 8k ( only sometimes though, which is why it took me so long to find. Of course when there is low fragmentation and a 16k block is free, no problem)

I'm sure you must know about this already, but...

unsigned char String::changeBuffer(unsigned int maxStrLen) {
    size_t newSize = (maxStrLen + 16) & (~0xf);
    char *newbuffer = (char *) realloc(buffer, newSize); // ***** here's where the problem starts...
    if(newbuffer) {
        size_t oldSize = capacity + 1; // include NULL.
        if (newSize > oldSize)
        {
            memset(newbuffer + oldSize, 0, newSize - oldSize);
        }
        capacity = newSize - 1;
        buffer = newbuffer;
        return 1;
    }
    buffer = newbuffer;
//      PMB HACK
    if(!buffer) Serial.println("************** PREPARE TO MEET THY DOOM! ************");
    return 0;
}

[Makuna]

The question is, what does realloc do when it fails. Does it dealloc the original buffer? For C++ 11; it should not.

I don't understand the need for the debug output though; you should be checking the return value to see if it fails.

It seems the real fix is to not modify the buffer with the newbuffer.

[earlephilhower]

Per the man page:
RETURN VALUE
The realloc() function returns a pointer to the newly allocated memory, which is suitably aligned for any built-in type and may be different from ptr, or NULL if the request fails. If size was equal to 0, either NULL or a pointer suitable to be passed to free() is returned. If realloc() fails, the original block is left untouched; it is not freed or moved.

So while technically you still have the memory you started with, I'm not sure how a library would be able to handle this and it'd need to fall back to the application to figure out the proper way forward in an embedded system.

One thing that might be worth changing this for is that you end up leaking the old String memory here. A failed realloc() isn't free()ing it, and and you're overwriting the pointer with NULL.

[philbowles]

OK, the point here is - this is the Arduino String library code, not MY code. I put the diagnostics display in to show where the error occurs, nothing more - I'm not suggesting it as a solution! Furthermore - despite any interesting discusssion on return values of realloc - this code is called from inside the constructor of a String object and therefore - by definition has no return type. The point being is that the user has no opportunity to do anything about this: when the low memory condition occurs, the next thing he knows is an illegal access crash as his code tries to dereference address 0...i.e unless he is a) prepared (as I was) to dig into the source code b) knowledgable enough to understand /interpret what he sees, then he is pretty much "stuffed" (as we English say...). That makes it a bug, in my book - the question is, what (if anything) can be done about it?

[suculent]

Arduino String is known to be problematic. Best solution imho is to provide a PR and let it be fixed asap for everyone. Odesláno z iPhonu 14. 8. 2017 v 9:25, Phil Bowles <notifications@github.com>:

…

OK, the point here is - this is the Arduino String library code, not MY code. I put the diagnostics display in to show where the error occurs, nothing more - I'm not suggesting it as a solution! Furthermore - despite any interesting discusssion on return values of realloc - this code is called from inside the constructor of a String object and therefore - by definition has no return type. The point being is that the user has no opportunity to do anything about this: when the low memory condition occurs, the next thing he knows is an illegal access crash as his code tries to dereference address 0...i.e unless he is a) prepared (as I was) to dig into the source code b) knowledgable enough to understand /interpret what he sees, then he is pretty much "stuffed" (as we English say...). That makes it a bug, in my book - the question is, what (if anything) can be done about it? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[Makuna]

The design of the standard Arduino String class is not very robust to low memory situations. Your identification of the use of realloc in the constructor is a prime example. Not sure that really can be fixed without using try/catch which you really don't want to do in microcontrollers.

Fixing the leak is about all that can be done without changing how String is documented to work as it is a Arduino Standard.

p.s. I personally don't use the string class due to its design.

[philbowles]

:) at "not very robust"

[adams13x13]

As the reference says, a null-pointer returned by realloc() means that the old memory was not deallocated. The case "size was zero" of C++98 is excludes since newSize is at least 16 (and so >0). To my understanding, one should delete the next-to-last line ("buffer = newbuffer;") and this function will be correct.

[philbowles]

Thanks guys.

[devyte]

Hi, I realize I'm way too late to this discussion, but I see a rationale behind the previous behavior.
If you run out of memory, there are usually two possibilitie: you detect the case, propagate to the user code layer, and the user can decide what to do, or exit/crash forcefully, because there is nothing further to be done.

The previous code assigned a null pointer, thereby forcing a crash on next access. The crash is not pretty, but the decode dump will tell you where it happened, and you can then deduce the null ptr dereference, and the failed realloc.
The current code does nothing. If I understand correctly, this realloc is usually called when an increment in capacity is needed due to an append operation. If the realloc failed silently, the append will proceed on the old buffer, which is full. The most likely result will be memory corruption due to past-the-end buffer write, which usually ends up with odd unexplained crashes, garbage printouts, etc. Good luck figuring out the reason in that case.
From this point of view, the previous code is better.

In bigger systems, an OOM case would throw with an appropriate message, and the user could catch and handle it, but on the ESP exceptions at disabled due to RAM constraints, so it's not viable.

[Makuna]

As far as the String class is concerned, its not about what is right way to do it, it is about what is compatible. Its a standard Arduino class and should act the same no matter what architecture you are building for.

[adams13x13]

@devyte Hi! I strongly disagree with the idea one should crash on any problem. In your car, you probably will not immediately cut off the engine if the bulb in the glove compartment does not work.

The crime of the old code is in the fact that if there was some memory allocated (variable "buffer" pointed to it) and by such assignment the pointer was just erased without returning the memory to the memory manager (memory leak). This is the thing one should never do.

The point is, the programm will not crash if buffer == NULL. It just means an empty string.

In this function, you are asked to increase the capacity of the string and tell whether it was successfull. New behaviour: try to increase, if successfull - ok, if not - tell about it and do not touch the string. If the caller is satisfied with not enough memory he can for example just let the variable go out of scope and the memory will be returned to the heap by the destructor. Old behaviour: try to increase, if success - ok, if not - freese the memory used by the buffer, erase the pointer and never tell anyone that this peace of memory is not used anymore, do not return it to the heap.

The function changeBuffer() has a return value that says whether capacity increase was successfull or not. If there is no memory available then the function tells about it returning zero. Some other functions that use changeBuffer() like reserve(), concat() also return a status. If you use function copy() or operator "+" to concatenate two strings and there is not enough memory to fit the result, you get an empty string.

@Makuna this is both right thing to do and compatible. On https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/WString.cpp the code is similar to the "new" version.

unsigned char String::changeBuffer(unsigned int maxStrLen)
{
	char *newbuffer = (char *)realloc(buffer, maxStrLen + 1);
	if (newbuffer) {
		buffer = newbuffer;
		capacity = maxStrLen;
		return 1;
	}
	return 0;
}