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.

Answer 1

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

Answer 2

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

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