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);
    }  
}

Rather than using malloc, why not just allocate buffers which are big enough for the largest possible string? It doesn't sound like large buffers would be required. — John Coleman
– John Coleman, Commented Jun 5, 2016 at 23:17
You know the maximum size is 3 digits per number, so you could use that and avoid dynamic memory all together. — John3136
– John3136, Commented Jun 5, 2016 at 23:18
Make a reasoned guess about what the maximum size of the data is likely to be then allocate that. Check as you're concatenating to make sure you don't go off the end and realloc if you need to. There is no magic way to do this. Sometimes programming is tedious. — JJF
– JJF, Commented Jun 5, 2016 at 23:18
As currently coded outputStr = concat(outputStr, "!"); will leak the original memory that outputStr points to before the call is made. — Andrew Henle
– Andrew Henle, Commented Jun 5, 2016 at 23:45
First make sure that code with just CDC_Device_SendString sending some fake test data works. That will make sure that api works. Then add printf in your code just before sending to USB, so that you can see what your code is sending. Then the possible issue that I see is that you are not sending !xxx string, but you are sending [junk]!xxx. To eliminate that, after outputStr = realloc(outputStr, 1); add outputStr[0] = 0;. I think this will fix the issue. Also please free up the memory as you are leaking old outputStr at every call to concat. — Ritesh
– Ritesh, Commented Jun 5, 2016 at 23:51

R Sahu · Accepted Answer · 2016-06-05 23:46:12Z

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);
}

what happens when outputStr gets clobbered, that's the contents lost.

user3629249 · Accepted Answer · 2016-06-07 13:19:56Z

the following code:

eliminates the unnecessary calls to malloc, free, realloc, so (amongst other things) has no memory leaks
creates the desired string to output
eliminates the unnecessary sub functions
generates a correct outputStr[] array contents

and now the code:

#include <string.h>

int main(void)
{
    uint8_t analogValue;
    char outputStr[20];  
    char temp[4];

    while (1)
    {
        memset( outputStr, 0, sizeof(outputStr) );
        outputStr = strcat(outputStr, "!");

        for(int i = 0; i < 5; i++)
        {
             analogValue = ReadADC(0);
             if( i < 4 )
                sprintf( temp, "%u,", analogValue );
             else
                sprintf( temp, "%u", analogValue );
            strcat( outputStr, temp );
        }

        CDC_Device_SendString(&VirtualSerial_CDC_Interface, outputStr); //send string via USB
    }  
}

Collectives™ on Stack Overflow

Concatenating a char array in C: dynamically handling memory

2 Answers 2

4 Comments

Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

Comments

Linked

Related