«

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!

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[i] = malloc(1 * sizeof *p[i]);


> *(*(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.

<snip>

> 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.
--
Flash Gordon

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[i] = 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!

On Jul 4, 6:22 pm, Flash Gordon <s...@flash-gordon.me.uk> wrote:
> 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[i] = malloc(1 * sizeof *p[i]);
>
> > *(*(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.
>
> <snip>
>
> > 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.

On Wed, 04 Jul 2007 05:29:24 -0700, marvinla wrote:

> 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

On Wed, 04 Jul 2007 15:09:59 +0100, Duncan Muirhead wrote:

> On Wed, 04 Jul 2007 05:29:24 -0700, marvinla wrote:
>
>> 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].
>
<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

> 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.

On Wed, 04 Jul 2007 05:29:24 -0700, marvinla
<> wrote:

>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[i], 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[i] (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

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[i] = malloc(2 * sizeof **p);

Check for p[i] 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);

<snip>

> 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.
--
Flash Gordon

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!