String::changeBuffer memory leak and crash

[me21]

According to realloc() description, if reallocation fails, old buffer is not freed and null pointer is returned.

Now see String::changeBuffer() implementation:

unsigned char String::changeBuffer(unsigned int maxStrLen) {
    size_t newSize = (maxStrLen + 16) & (~0xf);
    char *newbuffer = (char *) realloc(buffer, newSize);
    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;
    return 0;
}

If newbuffer is null, then old buffer pointer is overwritten and lost, leading to memory leak.

Moreover, if String::indexOf is called later, it will try to dereference null buffer pointer.

[me21]

Other places where buffer is not checked for NULL are String::getBytes, String::substring, String::replace(const String&, const String&), String::remove and String::lastIndexOf.

[me21]

This issue is partially duplicate of #3516, but perhaps buffer checking in the aforementioned functions is worth adding.

[d-a-v]

@me21 the bugs you mention have been fixed since then:

String::changeBuffer() and also realloc() behaviour in case of OOM.

Can you check if now all is OK on your side / ESPAsync libs ?

[me21]

Looks like. I will let know if something happens.

[d-a-v]

Thanks!
Please open a new issue if something wrong is found.