Pointer-to-pointer realloc problem

[marvinla]

Hello!

I'm a beginner in C, and I'm having trouble with a pointer-to-pointer
reallocation.
This piece of code works well, but Valkyrie warns some parts (pointed
below), and is
breaking my real code.

#include <stdio.h>
#include <stdlib.h>

int main() {

int i;
int **p = (int **)calloc(2, sizeof(int *));

for (i = 0; i < 2; i++)
*(p+i) = (int *)calloc(1, sizeof(int));

*(*(p+0)+0) = 0;
*(*(p+0)+1) = 1; // invalid write of size 4
*(*(p+1)+0) = 2;
*(*(p+1)+1) = 3;// invalid write of size 4

printf("%d\n", *(*(p+0)+0));
printf("%d\n", *(*(p+0)+1)); // invalid read of size 4
printf("%d\n", *(*(p+1)+0));
printf("%d\n", *(*(p+1)+1)); // invalid read of size 4

p = (int **)realloc(p, 3);
*(p+2) = (int *)calloc(1, sizeof(int)); // invalid write of size 4

*(*(p+2)+0) = 4; // invalid read of size 4
*(*(p+2)+1) = 5; // invalid read of size 4

printf("%d\n", *(*(p+2)+0)); // invalid read of size 4
printf("%d\n", *(*(p+2)+1)); // invalid read of size 4

free(*(p+0)); // invalid read of size 4
free(*(p+1));// invalid read of size 4
free(*(p+2));// invalid read of size 4

free(p);
return 0;

}

In my real code, glibc detects the "double free or corruption (out)"
error.
Where's my mistake?
Thanks a lot!

 

[Flash Gordon]

marvinla wrote, On 04/07/07 13:29:

Hello!

I'm a beginner in C, and I'm having trouble with a pointer-to-pointer
reallocation.
This piece of code works well, but Valkyrie warns some parts (pointed
below), and is
breaking my real code.

#include <stdio.h>
#include <stdlib.h>

int main() {

int i;
int **p = (int **)calloc(2, sizeof(int *));

Loose the cast, you don't need it in C and it is one more thing to get
wrong.
int **p = calloc(2, sizeof(int *));
Also, why bother to worry about the type when you can let the compiler
do it, it's another thing to get wrong.
int **p = calloc(2, sizeof *p);
This allocates enough space for two pointers and sets it to all bits 0.
Why use calloc when you immediately initialise it?
int **p = malloc(2 * sizeof *p);
See, much simpler.

Then you need to check if the call succeeded, i.e. check p to see if it
is null.

for (i = 0; i < 2; i++)
*(p+i) = (int *)calloc(1, sizeof(int));

All comments above apply.
*(p+i) = malloc(1 * sizeof **p);
Or, more simply
p = malloc(1 * sizeof *p);

*(*(p+0)+0) = 0;
*(*(p+0)+1) = 1; // invalid write of size 4

Of course. You explicitly allocate space for ONE int, what makes you
think you can write a second one? From this point on all bets are off.

In my real code, glibc detects the "double free or corruption (out)"
error.

You were lucky.

Where's my mistake?
Thanks a lot!

See above for the first few errors. I've not checked the rest of your code.

 

[marvinla]

Flash Gordon, thanks the reply!
I rewrote my sample code, and my real code. Sorry for the dumbs
mistakes.
But Valkyrie still warns me and my real code is still broken. Sorry
for reposting all the code.

#include <stdio.h>
#include <stdlib.h>

int main() {

int i;
int **p = malloc(2 * sizeof *p);

for (i = 0; i < 2; i++)
p = malloc(2 * sizeof **p);

p[0][0] = 0;
p[0][1] = 1;
p[1][0] = 2;
p[1][1] = 3;

printf("%d\n", p[0][0]);
printf("%d\n", p[0][1]);
printf("%d\n", p[1][0]);
printf("%d\n", p[1][1]);

p = realloc(p, 3);

if (p == NULL) {
printf("OUT OF MEMORY!\n");
exit(1);
}

p[2] = malloc(2 * sizeof **p); //Invalid write of size 4

p[2][0] = 4; //Invalid read of size 4
p[2][1] = 5; //Invalid read of size 4

printf("%d\n", p[2][0]); //Invalid read of size 4
printf("%d\n", p[2][1]); //Invalid read of size 4

free(p[0]); //Invalid read of size 4
free(p[1]); //Invalid read of size 4
free(p[2]); //Invalid read of size 4

free(p);
return 0;

}

I think I didn't understand the behavior of the realloc function .
Thanks a lot!

 

[Spade]

marvinla wrote, On 04/07/07 13:29:

Hello!

I'm a beginner in C, and I'm having trouble with a pointer-to-pointer
reallocation.
This piece of code works well, but Valkyrie warns some parts (pointed
below), and is
breaking my real code.

#include <stdio.h>
#include <stdlib.h>

int main() {

int i;
int **p = (int **)calloc(2, sizeof(int *));

Loose the cast, you don't need it in C and it is one more thing to get
wrong.
int **p = calloc(2, sizeof(int *));
Also, why bother to worry about the type when you can let the compiler
do it, it's another thing to get wrong.
int **p = calloc(2, sizeof *p);
This allocates enough space for two pointers and sets it to all bits 0.
Why use calloc when you immediately initialise it?
int **p = malloc(2 * sizeof *p);
See, much simpler.

Then you need to check if the call succeeded, i.e. check p to see if it
is null.

for (i = 0; i < 2; i++)
*(p+i) = (int *)calloc(1, sizeof(int));

All comments above apply.
*(p+i) = malloc(1 * sizeof **p);
Or, more simply
p = malloc(1 * sizeof *p);

*(*(p+0)+0) = 0;
*(*(p+0)+1) = 1; // invalid write of size 4

Of course. You explicitly allocate space for ONE int, what makes you
think you can write a second one? From this point on all bets are off.

In my real code, glibc detects the "double free or corruption (out)"
error.

You were lucky.

Where's my mistake?
Thanks a lot!

See above for the first few errors. I've not checked the rest of your code.
--

In the rest of the code...

p = (int **)realloc(p, 3);

You're reallocating with only three bytes here. Its not allocating
enough memory for 3 pointers as you seem to be trying. If you're
trying to allocate for 3 integer pointers, make it:

p = realloc(p, 3 * sizeof *p);

*(p+2) = (int *)calloc(1, sizeof(int)); // invalid write of size 4

That is because *(p+2) may not belong to your program, you should
allocate properly as stated above.

*(*(p+2)+0) = 4; // invalid read of size 4
*(*(p+2)+1) = 5; // invalid read of size 4
printf("%d\n", *(*(p+2)+0)); // invalid read of size 4
printf("%d\n", *(*(p+2)+1)); // invalid read of size 4
free(*(p+0)); // invalid read of size 4
free(*(p+1));// invalid read of size 4
free(*(p+2));// invalid read of size 4

Previous comment applies for the above statements too.

 

[Duncan Muirhead]

Hello!

I'm a beginner in C, and I'm having trouble with a pointer-to-pointer
reallocation.
This piece of code works well, but Valkyrie warns some parts (pointed
below), and is
breaking my real code.

#include <stdio.h>
#include <stdlib.h>

int main() {

int i;
int **p = (int **)calloc(2, sizeof(int *));

for (i = 0; i < 2; i++)
*(p+i) = (int *)calloc(1, sizeof(int));

*(*(p+0)+0) = 0;
*(*(p+0)+1) = 1; // invalid write of size 4
*(*(p+1)+0) = 2;
*(*(p+1)+1) = 3;// invalid write of size 4

printf("%d\n", *(*(p+0)+0));
printf("%d\n", *(*(p+0)+1)); // invalid read of size 4
printf("%d\n", *(*(p+1)+0));
printf("%d\n", *(*(p+1)+1)); // invalid read of size 4

p = (int **)realloc(p, 3);
*(p+2) = (int *)calloc(1, sizeof(int)); // invalid write of size 4

*(*(p+2)+0) = 4; // invalid read of size 4
*(*(p+2)+1) = 5; // invalid read of size 4

printf("%d\n", *(*(p+2)+0)); // invalid read of size 4
printf("%d\n", *(*(p+2)+1)); // invalid read of size 4

free(*(p+0)); // invalid read of size 4
free(*(p+1));// invalid read of size 4
free(*(p+2));// invalid read of size 4

free(p);
return 0;

}

In my real code, glibc detects the "double free or corruption (out)"
error.
Where's my mistake?
Thanks a lot!

*(p+1)+1 is the same as *p+2 which is the same as *(p+2), so
*(*(p+1)+1) is the same as **(p+2). I suspect you wanted *((*(p+1))+1)
On the other hand it would be a lot easier to understand if you wrote
it as p[1][1].

The second argument to realloc is the size in chars. If you want room for
three integer pointers you need to multiply the 3 by sizeof( int*), or
p = realloc( p, 3*sizeof *p);
Its a good habit to test whether realloc failed (it will if it can't
find the memory). It returns NULL in this case, so you should eg exit() if
p is NULL. If you hope to recover from the realloc failure you need
something like:
int* newp = realloc( p, 3*sizeof *p);
if ( newp == NULL) { /* Argh! but p is still valid */}
else { p = newp; }

If you write somewhere you shouldn't, especially past the end or before
the beginning of a malloc'd block -- sometimes called doing a pooh
in the heap -- there's a good chance you'll cause subsequent malloc's
free's etc to fail mysteriously. Often the heap is managed by having
information written before or after the block you get. Overwriting these
causes disasters.

Duncan

 

[Duncan Muirhead]

*(p+1)+1 is the same as *p+2 which is the same as *(p+2), so
*(*(p+1)+1) is the same as **(p+2). I suspect you wanted *((*(p+1))+1)
On the other hand it would be a lot easier to understand if you wrote
it as p[1][1].

<snip>
Oops, that's nonsense. Sorry.
The mistake is that
*(*(p+0)+1) = 1; (or p[0][1] = 1) writes to the second integer pointed
to by *(p+0) (or p[0]). But p[0] is an allocated block of just 1 integer.
Duncan

 

[marvinla]

You're reallocating with only three bytes here. Its not allocating

enough memory for 3 pointers as you seem to be trying. If you're
trying to allocate for 3 integer pointers, make it:

p = realloc(p, 3 * sizeof *p);

That's it!! Thanks A LOT Spade! I was using realloc the wrong way!
Again, thanks Flash Gordon and Spade!

ps: Just to say: in my real code, I use functions I wrote, xmalloc,
xcalloc and xrealloc, witch
terminates the program and prints a proper message.

 

[Barry Schwarz]

Hello!

I'm a beginner in C, and I'm having trouble with a pointer-to-pointer
reallocation.
This piece of code works well, but Valkyrie warns some parts (pointed
below), and is
breaking my real code.

#include <stdio.h>
#include <stdlib.h>

int main() {

int i;
int **p = (int **)calloc(2, sizeof(int *));

Lose the cast. It never helps and can cause the compiler to suppress
a diagnostic you would really want to see.

calloc is not really appropriate when allocating space for pointers.
It initializes memory to all bits 0. This need not be a valid
representation for a pointer and even if it is it may not represent
NULL. In this case, it is moot since you immediately assign new
values to the pointers.

p holds the address of an area of memory large enough to hold 2 int*.
Since int* is 4 bytes on your system, this memory is 8 bytes wide.

for (i = 0; i < 2; i++)
*(p+i) = (int *)calloc(1, sizeof(int));

*(p+i), which most prefer to write as p, now holds the address of
an area of memory large enough for one int. On your system,
sizeof(int) is 4 so this memory is 4 bytes wide.

*(*(p+0)+0) = 0;

This stores an int at the address contained in p[0]. Most prefer the
notation p[0][0]. This completely fills up the 4 bytes that p[0]
points to.

*(*(p+0)+1) = 1; // invalid write of size 4

This attempts to store an int 4 bytes (a system specific value) beyond
the address contained in p[0]. But these 4 bytes don't exist. The
memory p[0] points to only four bytes wide.

*(*(p+1)+0) = 2;
*(*(p+1)+1) = 3;// invalid write of size 4

printf("%d\n", *(*(p+0)+0));
printf("%d\n", *(*(p+0)+1)); // invalid read of size 4
printf("%d\n", *(*(p+1)+0));
printf("%d\n", *(*(p+1)+1)); // invalid read of size 4

p = (int **)realloc(p, 3);

This changes p so that it no longer points to an area 8 bytes wide but
points to an area only 3 bytes wide. These 3 bytes are insufficient
to hold even one int*, let alone the two you had previously, and
definitely not the third one you attempt to use below. You probably
meant 3*sizeof(int*) but we recommend the self-adjusting expression
3 * sizeof *p
which will work for any type p (except void*).

As a design approach, this construct suffers from the following
problem:

If realloc fails for some reason, it returns NULL and leaves
the originally allocated memory unchanged. With your code however,
the original value of p would be replaced by NULL and you would loose
any chance to do anything with the data in that memory. The end
result is you have three memory leaks (allocated memory you no longer
have any method of accessing): the original memory allocated to p (8
bytes) and the memory allocated to each p (4 bytes each).

The recommended approach is
temp = realloc(p, ...);
if (temp == NULL)
{your error handler goes here}
p = temp

*(p+2) = (int *)calloc(1, sizeof(int)); // invalid write of size 4

If you correct the realloc argument, this problem goes away.

*(*(p+2)+0) = 4; // invalid read of size 4

So does this one.

*(*(p+2)+1) = 5; // invalid read of size 4

But you still cannot store a second int into memory large enough for
only one.

printf("%d\n", *(*(p+2)+0)); // invalid read of size 4
printf("%d\n", *(*(p+2)+1)); // invalid read of size 4

free(*(p+0)); // invalid read of size 4
free(*(p+1));// invalid read of size 4
free(*(p+2));// invalid read of size 4

These error also were caused by the bad realloc.

free(p);
return 0;

}

In my real code, glibc detects the "double free or corruption (out)"
error.
Where's my mistake?

Once you attempt to store data beyond the end of allocated memory, you
are in the realm of undefined behavior. Anything, but anything, can
happen. In this case, glibc (using some system specific
implementation details) was able to detect part of the problem but
that is just fortuitous happenstance.

Thanks a lot!

Remove del for email

 

[Flash Gordon]

marvinla wrote, On 04/07/07 15:00:

Flash Gordon, thanks the reply!

Before anyone complains about lack of context, the post by marvinla does
actually stand on its own since it provides the full new code and a
statement of the problem.

I rewrote my sample code, and my real code. Sorry for the dumbs
mistakes.

Dumb mistakes we don't mind when people listen to the comments and
address them, which you seem to be doing, although you missed some of
the comments.

But Valkyrie still warns me and my real code is still broken. Sorry
for reposting all the code.

Posting your entire corrected code was the correct thing to do.

#include <stdio.h>
#include <stdlib.h>

int main() {

It is better practice to be explicit about there being no parameters. It
does not make a difference here, but it is easier to always "do the
right thing" than to remember when you do not need to.
int main(void) {

int i;
int **p = malloc(2 * sizeof *p);

Check for p being a null pointer here, malloc can fail.

for (i = 0; i < 2; i++)
p = malloc(2 * sizeof **p);

Check for p being null here.

p[0][0] = 0;
p[0][1] = 1;
p[1][0] = 2;
p[1][1] = 3;

printf("%d\n", p[0][0]);
printf("%d\n", p[0][1]);
printf("%d\n", p[1][0]);
printf("%d\n", p[1][1]);

p = realloc(p, 3);

In general you should not use realloc like this, since when it fails you
are throwing away your pointer to the still valid original block. You
should use a temp as in
tmp = realloc(p,newsize);

Not critical in this case since you terminate the program on failure,
but a point to remember for the future.

Now for your actual bug. What you wanted was, I believe,
p = realloc(p, 3 * sizeof *p);

As with malloc, realloc takes a number of bytes, not a number of objects
pointed to.

if (p == NULL) {
printf("OUT OF MEMORY!\n");
exit(1);

1 is not a portable exit value, and under VMS it would actually be a
success code! You want
exit(EXIT_FAILURE);

I think I didn't understand the behavior of the realloc function .
Thanks a lot!

Correct, you don't. Both realloc and malloc take a number of bytes,
which is why I multiplied by the size of the object pointed to on my
malloc calls.

One last comment, please do not use tabs when posting. Sometimes they
seem to get lost, whether it is some peoples news clients, some peoples
posting software, or some news readers I can't be bothered to verify,
but it does happen. In any case, 8 characters is far too large an indent
level in my opinion.

 

[Herhor]

marvinla pisze:

Where's my mistake?

Your main mistake is you are using outdated malloc/calloc/realloc/free
functions.

If you feel bored above zerking explanations, try new/delete combination!

 

[Keith Thompson]

marvinla pisze:

Your main mistake is you are using outdated malloc/calloc/realloc/free
functions.

If you feel bored above zerking explanations, try new/delete combination!

C does not have "new" or "delete". You're probably thinking of C++, a
language which has its own newsgroup, and about which the original
poster specifically did not ask.

 

[CBFalconer]

That's it!! Thanks A LOT Spade! I was using realloc the wrong way!
Again, thanks Flash Gordon and Spade!

ps: Just to say: in my real code, I use functions I wrote, xmalloc,
xcalloc and xrealloc, witch
terminates the program and prints a proper message.

Don't. It is simpler, more accurate, and more flexible to write:

if (!(p = malloc(whatever * sizeof *p))) attemptcorrection();
else /* all is well */ {
....
}

 

[Herhor]

Keith Thompson pisze:

C does not have "new" or "delete". You're probably thinking of C++, a
language which has its own newsgroup, and about which the original
poster specifically did not ask.

Yes, but fortunately most C++ compilers *can build C source code* using
new/delete functions.

 

[CBFalconer]

marvinla pisze:


Your main mistake is you are using outdated
malloc/calloc/realloc/free functions.

If you feel bored above zerking explanations, try new/delete
combination!

Ignore this. He is ignorant, and is talking about C++, which is
not C.

 

[Joe Wright]

Keith Thompson pisze:

Yes, but fortunately most C++ compilers *can build C source code* using
new/delete functions.

Fascinating. You have a C++ compiler that can build C source? Can you
show me how?

 

[Keith Thompson]

Keith Thompson pisze:

Yes, but fortunately most C++ compilers *can build C source code*
using new/delete functions.

Not true. If a compiler can compile this:

void new(void) { }
void delete(void) { }
int main(void)
{
new();
delete();
return 0;
}

then it's not a C++ compiler.

If you're referring to 'new' and 'delete' *operators* rather than
functions, any source code that uses them is not C source code.

Despite their similarities, C and C++ are two different languages.

(I saw the smiley, but if there was a joke in there somewhere, I
missed it.)

 

[Default User]

Keith Thompson pisze:

Yes, but fortunately most C++ compilers *can build C source code*
using new/delete functions.

It would be a good idea for you to learn something about both languages
before discussing them. That way you don't look like such a fool.




Brian

 

[Richard Heathfield]

Herhor said:

marvinla pisze:

Your main mistake is you are using outdated malloc/calloc/realloc/free
functions.

That's not a mistake, and they're not outdated.

If you feel bored above zerking explanations, try new/delete
combination!

Fine, given appropriate definitions of new and delete, and provided
delete isn't 0, of course. Otherwise, the behaviour is undefined.

 

[Ben Bacarisse]

Don't. It is simpler, more accurate, and more flexible to write:

if (!(p = malloc(whatever * sizeof *p))) attemptcorrection();
else /* all is well */ {
....
}

There are a fair few simple programs (and many learning exercises)
where 'attemptcorrection()' is 'print error and exit' because there is
simply no way to continue. You advice seems to suggest that codifying
that pattern is wrong.

I think it is better advice to say "remember, sometimes a program can
take useful emergency action, rather than just stop".

In your suggested pattern, the name 'attemptcorrection' is rather
misleading because, in those cases when actual correction is possible
(rather than, say, an emergency data save), you can't reasonably do it
after malloc. You need a malloc replacement:[1]

void *try_hard_to_malloc(size_t nb)
{
void *p;
while ((p = malloc(nb)) == NULL)
if (!free_up_some_more_non_essential_memory())
break;
return p;
}

To put it another way, your example is incomplete in that it leaves
the OP wondering what to do after the closing brace of your snippet.

[1] Or you put at least two malloc calls at each locus that needs
memory.

 

[Richard Bos]

marvinla pisze:

Your main mistake is you are using outdated malloc/calloc/realloc/free
functions.

If you feel bored above zerking explanations, try new/delete combination!

Great. Now please explain how to replace realloc() - _without_ copying
data where not necessary, please! - using only the inferior new and
delete.

Richard