CODEWITHSUNDEEP
crealloc
Fmstrat
Fmstrat

Reputation: 1692

Realloc Crashing on Passed Pointer

I can't seem to find a question that matches exactly what I'm doing, so here goes. Below is a cut down version of my C app down to where a problem lies. I know it's ugly code and missing a few error checks but it was just for me to figure out this problem. As it stand the sample below should convert all 'A's to 'BCDE's. The comments in the code describe the issue. (runMe is executed first)

int runMe2(char *in, char **buffer) {
    long x;
    long b_size = 0; 
    long i_size = 1000;
    long i = 0;
    char t_buffer[1006];

    // Initial malloc small as it will grow
    *buffer = (char *)malloc(2*sizeof(char));
    strcpy(*buffer, "");
    for (x = 0; x < 999; x++)
        t_buffer[x] = 0;
    for (x = 0; x < strlen(in); x++) {
        if (i >= i_size) {
            char *r_buffer;
            b_size = b_size + 1006*sizeof(char);
            i_size = 0;
            // Here is where the problem is.
            // The first time through, i=1000, b_size=1006 and everything is fine
            // The second time throgh, i=1004, b_size=2012 and the heap crashes on the realloc
            r_buffer = (char *)realloc(*buffer, b_size);
            if (r_buffer == NULL)
                exit(0);
            *buffer = r_buffer;
            strcat(*buffer, t_buffer);
            for (x = 0; x < 999; x++)
                t_buffer[x] = 0;
        }
        if (in[x] == 'A') {
            t_buffer[i++] = 'B';
            t_buffer[i++] = 'C';
            t_buffer[i++] = 'D';
            t_buffer[i++] = 'E';
        }
    }
}

int runMe() {
    char *out;
    char in[30000];
    int x = 0;

    // Set up a 29,999 character string
    for (x = 0; x < 30000; x++)
        in[x] = 'A';
    in[29999] = 0;
    // Send it as pointer so we can do other things here
    runMe2(in, &out);
    // Eventually, other things will happen here
    free(out);
}

Upvotes: 0

Views: 168

Answers (2)

001
001

Reputation: 13533

Just for fun here's a different algo that works and is much simpler than yours:

int runMe2(char *in, char **buffer)
{
    // Count number of A's
    int number_of_As = 0;
    int i, j;
    for (i = 0; 0 != in[i]; i++) {
        if (in[i] == 'A') {
            number_of_As += 1;
        }
    }

    // If number_of_As == 0, you can just do a strdup here and return

    // Because of 1st loop, i == strlen(in), no need to call strlen
    long bytes = (i - number_of_As + (number_of_As * 4) + 1) * sizeof(char);
    // Error check here

    // Only 1 memeory allocation needed
    *buffer = (char *)malloc(bytes);

    // Simple copy loop
    for (i = 0, j = 0; 0 != in[i]; i++) {
            // If it's an A replace
        if (in[i] == 'A') {
            (*buffer)[j++] = 'B';
            (*buffer)[j++] = 'C';
            (*buffer)[j++] = 'D';
            (*buffer)[j++] = 'E';
        }
            // Not an A, just copy
        else {
            (*buffer)[j++] = in[i];
        }
    }
    // Null terminate
    (*buffer)[j] = 0;

    return j;
}

Upvotes: 2

Mat
Mat

Reputation: 206679

if (i >= i_size) {
  ...
  i_size = 0;
  ...
}
if (in[x] == 'A') {
  t_buffer[i++] = 'B';
  ...

This can't be right. You'll be writing past the end of t_buffer if in is ever longer than the original i_size. You probably meant to reset i there, not i_size.

Then you're using string functions with t_buffer when you've not guaranteed that it is properly null-terminated - you initialize the first thousand values, but overwrite those in your loop. If you're going to use strcat and friends, you need to take more care to make sure it stays null-terminated. But using memcpy would lead to simpler code since you know the lengths of the arrays involved.

for (x = 0; x < strlen(in); x++) {
  ...
  for (x = 0; x < 999; x++)
    ...
    t_buffer[x] = 0;

This can't be right either, as spotted by Useless. Use a second variable for that, or better use memset.

Upvotes: 3

Related Questions