C Runtime Error: Dynamically Allocating Memory in a For Loop

Question

I'm posting a continuation of my question from this thread.

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".

My code is mostly working; I'm able to send the value of one analog sensor across my serial port, !215!215!215 for example, but when I un-comment the for loop code below I see nothing across my serial port and the program seems, for lack of a better word, useless.

There seems to be a runtime error occuring in my for loop but I can't determine where it happens. Why does my code below successfully send serial data for one analog sensor without using the for loop, and send nothing when using the for loop? How can I tweak my code to achieve my desired output?

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

char* concat(char *s1, char *s2)
{
    char *result = malloc(strlen(s1)+strlen(s2)+1);
    strcpy(result, s1);
    strcat(result, s2);
    return result;
}


int main(void){
    uint8_t analogValue;
    char *outputStr = malloc(1);

    while (1) {
        outputStr = realloc(outputStr, 2);
        strcpy(outputStr, "!");
        analogValue = ReadADC(0);
        char str[4]; 
        outputStr = concat(outputStr, convertIntToString(analogValue, str));

        //RUNTIME ERROR IN THIS LOOP
        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 sring over serial port
        free(outputStr); 
    }  
}

Looks like you got the obligatory close vote :) (Not from me) — Mad Physicist
– Mad Physicist, Commented Jun 21, 2016 at 3:38
outputStr = concat() causes a memory leak since you've just lost the only pointer to a block of memory (the previous value of outputStr). So much dynamic memory stuff for so little gain... — John3136
– John3136, Commented Jun 21, 2016 at 3:43
Could it be because you try to realloc a free memory block after sending the first string? — Stack Exchange Broke The Law
– Stack Exchange Broke The Law, Commented Jun 21, 2016 at 3:48
After free(outputStr); at the end of the while (1) block, the next iteration attempts to outputStr = realloc(outputStr, 2);. But the real answer is: learn to use the debugger. — dxiv
– dxiv, Commented Jun 21, 2016 at 3:48
As discussed in the previous question: if you can make a sensible guess at the biggest string then you can just use that size: e.g. 5 lots of 5 numbers with a max of 3 digits = 3*5*5. Oops: allow some space for commas, just assume 1 per number, so its 4*5*5. Plus one for the "!" = (4*5 + 1) * 5 If you init buffer[0] to 0 you can just use strcat() to append to it. For debugging and learning just use a fixed buffer - say 256. All those mallocs and copies take some time. Using concat (one malloc, 2 copies) just to add a "," is dumb. — John3136
– John3136, Commented Jun 21, 2016 at 5:16

John3136 · Accepted Answer · 2016-06-23 02:24:30Z

Expanded from the comment above and comments in the previous question.

If you are able to calculate the maximum size of a "packet", then you can avoid dynamic memory all together and just use a fixed buffer size. The calculation doesn't even have to be 100% accurate, as long as it errs on the side of "too big".

e.g.: 5 instances of 5 numbers with a max of 3 digits separated by commas: 5 * 5 * 4 (3 digits + a comma). Not 100% right because the 5th group doesn't need a comma, so you are over estimating by one (or is that 5?). Just be aware of the possible cumulative effect of this if you have multiple "known errors".

So assuming you can estimate the max size, perhaps "encode" it via #defines - perhaps even fixing some of the "known errors").

So now you have char buffer[KNOWN_UPPER_BOUND], as long as you initialize it correctly (e.g. buffer[0] = '\0';, you can just keep appending to it via strcat(). If you were talking big numbers, you could keep track of the last index to avoid repeated scans through the string looking for the end. e.g. (using globals for simplicity)

char buffer[KNOWN_UPPER_BOUND];
int last_index=0;

addString(char* str)
{
    int len = strlen(str);
    if (last_index + len > KNOWN_UPPER_BOUND)
    {
        /* error handling */
    }
    else
    {
        strcat(buffer[last_index], str);
        last_index += n;
    }
}

So what were some of the issues with the dynamic code?

Potential for leaks (much like the errors I mentioned in the calculation - ok (by which I mean 'not overly harmful in a small program') if you leak 2 bytes once, not so good when you put it in a loop and leak 2 bytes over and over again)
Speed issues - malloc is out of you control, it could be very slow. A lot of small allocations can fragment memory which may mean later on when you want a bigger block you can't get one.
Lots of copying and re-copying of data. Your concat is an example here - each concat does a malloc and copies both strings. Every time you call it.

You could still use dynamic memory to hold the final string, but build up each "component" in a fixed size buffer.

Twinkle · Accepted Answer · 2016-06-21 06:20:38Z

0

What if you move the declaration of char* newStr outside the loop. Declaring the newStr as char array will be better than pointer to avoid leakage. something like char newStr[50]

answered Jun 21, 2016 at 6:20

Twinkle

5243 silver badges8 bronze badges

Collectives™ on Stack Overflow

C Runtime Error: Dynamically Allocating Memory in a For Loop

2 Answers 2

Comments

Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Linked

Related