realloc corrupting data 2

[Andorin]

Hi,

I'm trying to get a dynamic array working inside my program. I'm new to DMA and so am not up to scratch with it - i've just been plugging knowledge in from everywhere.

Anyway, i've got the arrays working so i can add new entries as and when i want. BUT, the data seems to get corrupted at random points by realloc statements. Can anyone explain this to me?

Heres my code:

// Get space for dynamic arrays
freq = (int *)malloc(size*sizeof(int)+1);
num = (int *)malloc(size*sizeof(int)+1);
num[1] = *list[0];
freq[1]=1;

// Go through price list
for(count=1;count<=days-1;count++)
{
// Note frequency of entries
match=0;
for(c=1;c<=days;c++){if(*list[count]==num[c]){freq[c]++;match=1;break;}}
if(match==0){
size++;
realloc(freq,size*sizeof(int)+1);
freq[size] = 1;
realloc(num,size*sizeof(int)+1);
num[size] = *list[count];
}
}

Cheers in advance

 

[chipperMDW]

You ignore the return value of [tt]realloc[/tt], so if [tt]realloc[/tt] needs to move the buffer, you don't know where it moved it.

 

[xwb]

Chipper meant

            freq = realloc(freq,size*sizeof(int)+1);           
            freq[size] = 1;          
            num = realloc(num,size*sizeof(int)+1);
            num[size] = *list[count];

It is a very common mistake

 

[DaveSinkula]

So is assigning to the same value passed as the first parameter to [tt]realloc[/tt]. Instead a temporary should be used so that if [tt]realloc[/tt] fails the original memory is not leaked.

temp = realloc(ptr, elements * sizeof *ptr);
if ( temp != NULL )
{
   ptr = temp;
}
else
{
   free(ptr);
   /* etc. */
}

 

[WebDrake]

Instead a temporary should be used so that if realloc fails the original memory is not leaked.

Suppose that in this event the code would be producing wrong results anyway---how would you code something to kill the job and print an error message to file, if a realloc failed in this way?

Under what sort of conditions might a realloc fail (except for the obvious one where it reallocs to size zero)? It seems to me that this is a sort of safety catch for large-scale coding rather than an essential thing. If you can be sure what you are being given and when, it shouldn't happen, right?

In any case, in the code where I use realloc, if it fails, the whole simulation is screwed anyway... :-(

 

[Salem]

> if a realloc failed in this way?
Like dave said, you put code into the else part.

free ( ptr );
fprintf ( stderr, "OOPS" );  /* your error message */
exit ( EXIT_FAILURE );       /* bail out, telling the environment */

> Under what sort of conditions might a realloc fail (except for the obvious one where it reallocs to size zero)?
Actually, realloc(ptr,0) is the same as free(ptr), so all things being equal, that is the one which is guaranteed to work. As for anything else, you have to assume some small chance that any allocator (malloc, calloc, realloc) will fail at some point if the OS/Environment runs out of memory.

And it's not just making the memory bigger which can fail, making the amount smaller can also fail or make the block move in memory.

> It seems to me that this is a sort of safety catch for large-scale coding rather than an essential thing.
I'm sure you'd really appreciate an editor losing all your work just because of sloppy realloc handling. Instead of simply crashing out, say that there is no more memory to complete the edit operation and prompt the user to save what they have.

> realloc(freq,size*sizeof(int)+1);
But to answer your original 'corruption' question, you have to realise that at the same time that realloc can fail at any time, you also have to realise that the block can move in memory at any time as well. If the block moved, then you're using memory which isn't yours.

At the very least, you need an assignment

freq = realloc(freq,size*sizeof(int)+1);

Better would be a check

freq = realloc(freq,size*sizeof(int)+1);
if ( freq == NULL ) { exit(1); }

Sure it's a memory leak, but you're exiting the program anyway.

Best of all is to use a temp variable, and make an informed decision as to how to proceed if the realloc fails.

> freq = (int *)malloc(size*sizeof(int)+1);
Two more problems here
1. Do not cast allocate calls in C - if you did the right thing, then the code will do the right thing. If you didn't, then you should fix the problem rather than hiding it with a cast.
2. All the +1 you have in your code is adding 1 'char' to the size, not 1 'int'. Essentially, you're allocating less space than you think, with the likelyhood that you're then accessing memory you didn't allocate.

Try for example

freq = malloc ( (size+1) * sizeof *freq );

On reflection, perhaps all the +1 is just mis-placed security on your part.

> freq[1]=1;
What was the value of 'size' on entry to this code?
If you have

freq = malloc ( sizeof *freq ); /* just 1 int */

Then freq[0] is the only int you have.
Did you allow for your count starting at 1 basically?

--

 

[WebDrake]

You seem to be mixing up me and the original questioner... ;-)

Anyway,

I'm sure you'd really appreciate an editor losing all your work just because of sloppy realloc handling. Instead of simply crashing out, say that there is no more memory to complete the edit operation and prompt the user to save what they have.

Yes, I poorly phrased my earlier comment. Your point is well made.