Leak in cores/esp32/WString.cpp when malloc fails

[APokorny]

unsigned char String::changeBuffer(unsigned int maxStrLen)
{  
    size_t newSize = (maxStrLen + 16) & (~0xf);  
    char *newbuffer = (char *) malloc(newSize); 
    if(newbuffer) {
        memset(newbuffer, 0, newSize); 
        memcpy(newbuffer, buffer, len);  
        if (buffer) { 
            free(buffer);  
        } 
        capacity = newSize - 1;  
        buffer = newbuffer;
        return 1; 
    } 
    buffer = newbuffer;
    return 0;
}

I think on allocation failure nothing should happen and 0 should be returned.

[stickbreaker]

I agree, if the malloc(newsize) fails, the function should return 0 (fail) and NOT NULL the buffer pointer.

unsigned char String::changeBuffer(unsigned int maxStrLen)
{  
    size_t newSize = (maxStrLen + 16) & (~0xf);  
    char *newbuffer = (char *) malloc(newSize); 
    if(newbuffer) {
        memset(newbuffer, 0, newSize); 
        memcpy(newbuffer, buffer, len);  
        if (buffer) { 
            free(buffer);  
        } 
        capacity = newSize - 1;  
        buffer = newbuffer;
        return 1; 
    } 
   // buffer = newbuffer;  // proposed change, delete this line
    return 0;
}

But, why is malloc() used instead of realloc()? To me, this is the perfect case to use realloc()?

So, something like this:

unsigned char String::changeBuffer(unsigned int maxStrLen)
{  
    size_t newSize = (maxStrLen + 16) & (~0xf);  
    char* newbuffer = (char *)realloc(buffer,newSize); 
    if(newbuffer) {
        if(newSize > capacity+1){// buffer expanded, null out new capacity
          memset(newbuffer, capacity, newSize-capacity); 
          // realloc() already transferred the existing data to the new buffer 
          }
        buffer = newbuffer;  // realloc() potentially moved the buffer
        capacity = newSize - 1;  
        return 1; // success
        }
    else { // realloc() failed, not able to expand, OR newSize is Zero, which should never be possible.
      if(newSize==0) { // should never happen because of (maxStrLen +16)&(~0xf), min of 16
        buffer=NULL;
        capacity = 0; 
        return 1; // succeeded in reducing the buffer to 0? 
        }
     else { // unable to expand 
       return 0; // Fail
       }
    }
}

Chuck.

[APokorny]

also ok .. just free(buffer) before assigning a new pointer to it..

[stickbreaker]

If you free the buffer make sure you update the capacity.

Also, if you free the buffer how does that effect the next time this library is called? And is the error propagated back to the user app?

I would think this failure should generate either an abort or log event.
Just quietly committing suicide should not be an option.

Chuck.

[me-no-dev]

How about this:

unsigned char String::changeBuffer(unsigned int maxStrLen)
{
    size_t newSize = ((maxStrLen + 16) & (~0xf)) - 1;
    char *newbuffer = (char *) realloc(buffer, newSize+1);
    if(newbuffer) {
        if(newSize > len){
            if(newSize > capacity){
                memset(newbuffer+capacity+1, 0, newSize-capacity);
            }
        } else {
            //new buffer can not fit the old len
            newbuffer[newSize] = 0;
            len = newSize;
        }
        capacity = newSize;
        buffer = newbuffer;
        return 1;
    }
    log_e("realloc failed! Buffer unchanged");
    return 0;
}

[stickbreaker]

@me-no-dev , Yes, Better.
@APokorny , Since you found this problem, why don't you create a fork, insert this code and create a pull. Since you have an interest in the String object, have you inspected the rest of the String code for similar errors?

Chuck.

[me-no-dev]

@stickbreaker I have the change staged :) was just waiting for confirmation in case I missed something

[zuqualla]

i keep getting this error [E][WString.cpp:185] changeBuffer(): realloc failed! Buffer unchanged
see https://github.com/vshymanskyy/TinyGSM/issues/268

[me-no-dev]

you are running out of memory

[zuqualla]

thx @me-no-dev!

the heap shows plenty of free space (checked with esp_get_free_heap_size()) and in the 'main' thread i still have >6000 of the allocated memory (checked with uxTaskGetStackHighWaterMark( NULL )).

1. Are all the libraries running in my 'main' thread? or they run in their own? how to check their memory consumption? the esp32-arduino libs seem to not support getting the list of threads.

2. according to here is should increase
   size_t newSize = ((maxStrLen + 16) & (~0xf)) - 1;
   to
   size_t newSize = ((maxStrLen + 32) & (~0xf)) - 1;
   any advice on this?

[stickbreaker]

Stack is not Heap.