Freeing returned pointer from function after it is malloc'd inside that function causes chip to crash

Question

Freeing returned pointer from function after it is malloc'd inside that function causes chip to crash

I have this call inside main

char* IDQueueString = getIDQueue();
pc.printf("[%9.6f] IDQueue: %s\r\n", t.read(), IDQueueString);
free(IDQueueString);

And getIDQueue(); is

char* getIDQueue(void)
{
    char* returnString;
    char idString[6];
    uint8_t pointer = 0;
    uint8_t queueLength = 0;
    returnString = (char*)malloc((uint32_t)(sizeof(char*)*128));
    if(returnString != 0){
        pc.printf("[%9.6f] MALLOC: %p, len: %d | LINE %d\r\n", t.read(), returnString, (int)(sizeof(char*)*128), __LINE__);
        for(uint8_t i = 0; i < ID_QUEUE_LENGTH; i ++){
            if(ReceivedIDs[i].id != -1){
                queueLength++;
                sprintf(idString, "%d", (int)ReceivedIDs[i].id);
                for(uint8_t j = 0; j < strlen(idString); j++){
                    returnString[pointer] = idString[j];
                    pointer++;
                }
                returnString[pointer++] = ',';
                returnString[pointer++] = ' ';
            }
        }
        if(queueLength > 0){
            returnString[pointer-2] = '.';
            returnString[pointer-1] = '\0';
        }
        if(queueLength == 0){
            returnString = (char*)"Empty!";
        }
        return returnString;
    }else{
        free(returnString);
        error("\x1b[31mMALLOC %d!\x1b[0m\r\n", __LINE__);
        return (char*)"MALLOC!";
    }
}

When it reaches free(IDQueueString); in the binary it does this somewhere down the chain of instructions:

0x08009d6c:   ldr     r6, [r1, #8]  // *0x2000a208, *0x20008500
0x08009d6e:   ldr     r0, [r2, #4]  // *0x200080d8, *0x1002db40
0x08009d70:   cmp     r2, r6        // *0x1002db40, *0x2000a208

But it hangs at 0x08009d6e (or rather returns to a WIRQ loop that loops on itself with

0x08004ffc:   b.n     0x8004ffc

I'm wondering what causes it to do this? It's clearly free(); causing a problem but I can't figure out what's the issue. The value at r1 and r2 is 0x00000000 and r0 is 0x00000009 and r6 is 0x5DECC885.

Addresses 0x20000001-0x2001BFFF (112KB) is the range for SRAM1 and 0x2001C000-0x2001FFFF (16KB) is the range for SRAM2.

I have no idea what that value 0x5DECC885 is about but it is possibly something to do with the USB-OCD interface.

I'm assuming the way I've used free is incorrect but since I return a pointer to a malloc(); why can't I free the returned pointer the way I am trying to?

c++

pointers

stm32

asked on Stack Overflow Jun 24, 2017 by

Supernovah • edited Jun 24, 2017 by (unknown user)

2 Answers

You can't free things that didn't come from malloc - it is "undefined behaviour" to do so, so depending on how your runtime is designed, what actually happens is "anything is possible", it could hang, crash, start a nuclear war by sending rude messages to Putin signed by Trump, or be "just fine".

There are two solutions for your particular scenario:

If malloc fails, return NULL (or preferrably nullptr if we're in C++). If the queue is empty, return a malloc'd string that contains the string "Empty!" (by assigning it with strcpy or similar).

Alternatively, and this only works if you KNOW that the queue won't contain words like "Empty!" or "MALLOC!": before you call free, check if that it isn't one of those strings (using if(strcmp("Emtpy!", ...) != 0 && strcmp("MALLOC!", ...) != 0) free(...);.

This could be made easier by having those strings in a list of known strings, and just return that entry, then you could have a function that checks "is it a known constant string" by comparing the pointer value itself. [In fact, this would also work when there is the same content string in the content you are processing, since the content string will have a different address if it came from your code] - it will also help avoid the problems of "Oh, I changed the value of the returned string, but not the check before free", since it's only one place to change for bot uses.

answered on Stack Overflow Jun 24, 2017 by

Mats Petersson

Every time I see the disassebly listing I know that it will be something silly. Wrong program logic - but the compiler has to be wrong :)

It is a good to allocate memory which will before the function call. All local mallocs should be freed before the return. It makes it easier to debug the code (especially the memory leaks).

You have a zillion ways to communicate with another part of the program, but the one you have chosen is the worst one. First it is good to separate errors (like failed mallocs) from the application exceptions like empty name in your case. Myself I would make this function to return int (for example negative ones as system errors, zero if OK and positive for application exceptions) - making dealing with them easier, and pointer to be returned via pointer to pointer. But it you want to malloc (and to do not free) in the function -

if((resultCode = getIDQueue(& IDQueueString) >= 0)
{
   // do something (you can have another switch here to deal with application exceptions like empty name for example)
   free(IDQueueString);
}
else
{
 switch (resultCode) 
 {
   case -1:
     // do something malloc failed
     break:
   case -2:
     //deal with another error 
     break;
  //...... etc etc 
   default:
     // ----
     break;
 }
}

answered on Stack Overflow Jun 24, 2017 by

0___________

User contributions licensed under CC BY-SA 3.0