Reallocing a char*

Question

I am trying to do a function that will store in a char array some information to print on it:

int offset = 0;
size_t size = 1;
char *data = NULL;
data = malloc(sizeof(char));

void create(t_var *var){
    size_t sizeLine = sizeof(char)*(strlen(var->nombre)+2)+sizeof(int);
    size = size + sizeLine;

    realloc(data, size);

    sprintf(data+offset,"%s=%d\n",var->name,var->value);
    offset=strlen(data);
}

list_iterate(aList, (void *)create);

t_var is a struct that has two fields: name (char*) and value (int).

What's wrong with this code? When running it on Valgrind it complains about the realloc and sprintf.

score 4 · Accepted Answer · answered Nov 01 '12 at 02:22

4

Without knowing the specific valgrind errors, the standout one is:

realloc(data, size); should be data = realloc(data, size);

answered Nov 01 '12 at 02:22

John3136

28,809
4
51
69

To back it up, consider this: `realloc` cannot change the pointer to make it point to new memory unless it's passed by reference, which it isn't. – chris Nov 01 '12 at 02:26
@chris Some implementations will sometimes reallocate in place, so that bug can sometimes sort of be missed during simple tests. – Jim Balter Nov 01 '12 at 03:43
@JimBalter, True. When the memory is any bigger and can't fit where it is, though, the pointer must be modified, and you need to keep every scenario in mind when being logical about whether it can do what it might need to. – chris Nov 01 '12 at 03:53
@chris Well, yes, of course, duh -- not assigning the result back to `data` is *wrong*. But that has nothing to do with my comment. – Jim Balter Nov 01 '12 at 04:00
Pointer is modified via the return value, not because it is passed by reference (which would work, but that is not the current API). realloc will try to do it's dirty work in place, but it is free to move the block to a new location. – John3136 Nov 01 '12 at 04:07
And, of course, you should always check if realloc() failed. When it does, it returns NULL and doesn't free (or modify) the original buffer, so if `data` is your only reference to the buffer, `data = realloc(data, size);` only works if you're planning to not free() or use the buffer on error. – potrzebie Nov 01 '12 at 07:29

score 0 · Answer 2 · answered Nov 11 '17 at 17:03

I'm sorry to say that, but almost EVERYTHING is wrong with your code.

First, incomplete code.

You say your t_var type has two members, name and value.

But your code refers to a nombre member. Did you forget to mention it or did you forget to rename it when publishing the code?

Second, misused sizeof.

You use a sizeof(int) expression. Are you aware what you actually do here?!

Apparently you try to calculate the length of printed int value. Alas, operator sizeof retrieves the information about a number of bytes the argument occupies in memory. So, for example, for 32-bits integer the result of sizeof(int) is 4 (32 bits fit in 4 bytes), but the maximum signed 32-bit integer value is power(2,31)-1, that is 2147483647 in decimal. TEN digits, not four.

You can use (int)(2.41 * sizeof(any_unsigned_int_type)+1) to determine a number of characters you may need to print the value of any_unsigned_int_type. Add one for a preceding minus in a case of signed integer types.

The magic constant 2.41 is a decimal logarithm of 256 (rounded up at the 3-rd decimal digi), thus it scales the length in bytes to a length in decimal digits.
If you prefer to avoid floating-point operations you may use another approximation 29/12=2.41666..., and compute (sizeof(any_unsigned_int_type)*29/12+1).

Third, sizeof(char).

You multiply the result of strlen by sizeof(char).

Not an error, actually, but completely useless, as sizeof(char) equals 1 by definition.

Fourth, realloc.

As others already explained, you must store the return value:

    data = realloc(data, size);

Otherwise you risk you loose your re-allocated data AND you continue writing at the previous location, which may result in overwriting (so destroying) some other data on the heap.

Fifth, offset.

You use that value to determine the position to sprintf() at. However, after the print you substitute offset with a length of last printout instead of incrementing it. As a result consecutive sprintfs will overwrite previous output!

Do:

    offset += strlen(data);

Sixth: strlen of sprintf.

You needn't call strlen here at all, as all functions of printf family return the number of characters printed. You can just use that:

    int outputlen = sprintf(data+offset, "%s=%d\n", var->name, var->value);
    offset += outputlen;

Seventh: realloc. Seriously.

This is quite costly function. It may need to do internal malloc for a new size of data, copy your data into a new place and free the old block. Why do you force it? What impact will it have on your program if it needs to print five thousand strings some day...?

It is also quite dangerous. Really. Suppose you need to print 5,000 strings but there is room for 2,000 only. You will get a NULL pointer from realloc(). All the data printed to the point are still at the current data pointer, but what will you do next?
How can you tell list_iterate to stop iterating...?
How can you inform the routine above the list_iterate that the string is incomplete...?

There is no good answer. Luckily you needn't solve the problem — you can just avoid making it!

Solution.

Traverse your list first and calculate the size of buffer you need. Then allocate the buffer — just once! — and go on with filling it. There is just one place where the allocation may fail and you can simply not go into the problem if that ever happens:

int totaloutputlength = 0;
char *outputbuffer    = NULL;
char *currentposition = NULL;

void add_var_length(t_var *var){
    const int numberlength = sizeof(var->value)*29/12 + 1;
    totaloutputlength += strlen(var->name) + 2 + numberlength;
}

void calculate_all_vars_length(t_list *aList){
    totaloutputlength = 0;
    list_iterate(aList, (void *)add_var_length);
}

void sprint_var_value(t_var *var){
    int outputlen = sprintf(currentposition, "%s=%d\n", var->name, var->value);
    currentposition += outputlen;  // advance the printing position
}

int sprint_all_vars(t_list *aList){
    calculate_all_vars_length(aList);
    outputbuffer = malloc(totaloutputlength + 1);  // +1 for terminating NUL char

    // did allocation succeed?
    if(outputbuffer == NULL) {                     // NO
        // possibly print some error message...
        // possibly terminate the program...
        // or just return -1 to inform a caller something went wrong

        return -1;
    }
    else {                                         // YES
        // set the initial printing position
        currentposition = outputbuffer;

        // go print all variables into the buffer
        list_iterate(aList, (void *)sprint_var_value);

        // return a 'success' status
        return 0;
    }
}

Reallocing a char*

2 Answers2