Concatenating a char array in C: dynamically handling memory

Question

I'm confused on what I'm doing wrong in my C program: I'm trying to create a string that begins with a '!' and adds 6 values read from a sensor (separated by commas) and then sends it over a serial port. A sample output would be: "!5,5,5,5,5,5" or "!34,34,34,34,34,34".

The problem: Because the sensor values (5 or 34 in the example above) can range from 0 to 255, I don't know at runtime how big my char array needs to be. This means I have to dynamically reallocate memory every time I want to add to my string. Below is my attempt to do so, but I'm doing this wrong because I see nothing coming across my serial port (indicating that there's a runtime error).

How can I properly implement code to allocate memory dynamically for a string? My attempts to use malloc and realloc aren't behaving as expected.

char* convertIntToString(uint8_t integerValue){
    char *str = malloc(4);          //up to 3 digits + 1 for null termination 
    utoa(integerValue, str, 10);
    return str;
}

char* concat(char *s1, char *s2)
{
    char *result = malloc(strlen(s1)+strlen(s2)+1);//+1 for the zero-terminator
    //in real code you would check for errors in malloc here
    strcpy(result, s1);
    strcat(result, s2);
    return result;
}

int main(void)
{
    uint8_t analogValue;
    char *outputStr = malloc(1);  //initalize size of char array = 1 element

    while (1) {
        outputStr = realloc(outputStr, 1);
        outputStr = concat(outputStr, "!");
        analogValue = ReadADC(0);
        outputStr = concat(outputStr, convertIntToString(analogValue));
        for(int i = 0; i < 5; i++){
            outputStr = concat(outputStr, ",");
            outputStr = concat(outputStr, convertIntToString(analogValue));
        }
        CDC_Device_SendString(&VirtualSerial_CDC_Interface, outputStr); //send string via USB
        free(outputStr);
    }  
}

Answer 1

You are running into undefined behavior since the contents of outputStr are not initialized properly in the first statement inside the while loop.

   outputStr = realloc(outputStr, 1); // outputStr is not initialized.

Change them to:

    outputStr = realloc(outputStr, 2);
    strcpy(outputStr, "!");

You are also leaking a whole bunch of memory. The value returned from convertToString is never freed.

You can avoid that problem by changing the strategy a little bit.

Change the function to expect a string and use it.

char* convertIntToString(uint8_t integerValue,
                         char* str)
{
   utoa(integerValue, str, 10);
   return str;
}

Then, change its usage as:

    outputStr = concat(outputStr, convertIntToString(analogValue, str));

You are also leaking memory due to the way you are using concat.

        outputStr = concat(outputStr, ",");

That leaks the old value of outputStr. You need to keep the old value of outputStr for a bit longer so you can free it.

Here's my suggestion for the while loop:

while (1) {

    outputStr = realloc(outputStr, 2);
    strcpy(outputStr, "!");

    analogValue = ReadADC(0);

    char str[4]; // This is the max you need.
                 // There is no need to malloc and free.

    outputStr = concat(outputStr, convertIntToString(analogValue, str));

    for(int i = 0; i < 5; i++){

        char* newStr = concat(outputStr, ",");

        // free the old memory before using the new memory
        free(outputStr);
        outputStr = newStr;

        newStr = concat(outputStr, convertIntToString(analogValue, str));

        // free the old memory before using the new memory
        free(outputStr);
        outputStr = newStr;
    }
    CDC_Device_SendString(&VirtualSerial_CDC_Interface, outputStr); //send string via USB
    free(outputStr);
}