realloc()

[Andrew Clark]

Hi all,

Wow, has it been a long time since I've been here. Too long.

Anyway, I thing I have found the source of a segfault in my program, but
I can't see anything wrong with this code (used to "push" a value on to
the end of an array):

int push (int **arr, int x)
{
int rc, *p;
static int count = 1 + 1;

*arr = realloc (*arr, sizeof x * count); /* segfault here */
p = *arr;

rc = (p ? 1 : 0);
count = (p ? count + 1 : 1);

if (p)
{
p [count - 2] = x;
p [count - 1] = 0;
}

return rc;
}

Can you?

 

[Thomas J. Gritzan]

Hi all,

Wow, has it been a long time since I've been here. Too long.

Anyway, I thing I have found the source of a segfault in my program, but
I can't see anything wrong with this code (used to "push" a value on to
the end of an array):

int push (int **arr, int x)

How do you call this function?

 

[Andrew Clark]

How do you call this function?

I call it like this:
....

for (i = 0; i < num; ++i)
{
/* 4. Start with the lowest number in the sieve list */
/* 5. Take the next number in the sieve list still marked prime */

/* 6. Include the number in the results list */
if (baIsPrime ) push (&resList, sieveList );

/* 7. Square the number and mark all multiples of that square as
nonprime */
nTemp = sieveList * sieveList ;
for (j = i; j < num; ++j)
{
if (sieveList [j] % nTemp == 0) baIsPrime [j] = 0;
}

/* 8. Repeat steps five through eight */
/* continue; */
}

(This is an excerpt from my program to create a list of primes using the
Sieve of Atkin)

 

[Frederick Gotham]

Andrew Clark posted:

int push (int **arr, int x)
{
int rc, *p;
static int count = 1 + 1;

*arr = realloc (*arr, sizeof x * count); /* segfault here */
p = *arr;

rc = (p ? 1 : 0);
count = (p ? count + 1 : 1);

if (p)
{
p [count - 2] = x;
p [count - 1] = 0;
}

return rc;
}

Without context, I don't know the objective of the function, so I can't say
what's right or wrong. Nonetheless, I'd probably clean it up a bit:

int push (int * *const arr, int const x)
{
#define p (*arr)

int static count = 1+1;

p = realloc(p, sizeof x * count); /* segfault here */

if (p)
{
++count;
p[count - 2] = x;
p[count - 1] = 0;
}
else count = 1;

return !!p;

#undef p
}

 

[bwaichu]

Andrew Clark posted:

That would still be wrong if realloc failed. Better to assign to
a local pointer first, test to see if realloc was successful, and then
assign over the original pointer if successful. Also, the idiom
of num * size should be avoid in malloc and realloc unless we
have previously check to make sure we do not have an overflow
condition.

And without knowing what occurred in the calling function, we
have no idea why the OP is saying his program is seg faulting.

 

[Stephen Sprunk]

Anyway, I thing I have found the source of a segfault in my program,
but
I can't see anything wrong with this code (used to "push" a value on
to
the end of an array):

int push (int **arr, int x)
{
int rc, *p;
static int count = 1 + 1;

*arr = realloc (*arr, sizeof x * count); /* segfault here */

The only reason I can see this segfaulting is if arr was NULL (or some
other garbage value). You don't have any code to detect arr==NULL,
though the context in your program may make that "impossible". I've
found the impossible tends to happen with disturbing regularity in
practice, though.

p = *arr;

rc = (p ? 1 : 0);
count = (p ? count + 1 : 1);

if (p)
{
p [count - 2] = x;
p [count - 1] = 0;
}

return rc;
}

Can you?

Is there any reason you didn't write that as:

p = *arr;

if (p) {
count++;
p[count - 2] = x;
p[count - 1] = 0;
return 1;
}

count = 1;
return 0;

That's a lot clearer to me, though I'd bet it compiles into the same
code.

It's also still not correct. If the realloc() fails, *arr will be NULL
and count will be 1 (and you've leaked memory). If you then push
another value, you will realloc() for sizeof(int)*1, but then try to
store x before the start of *arr if the second realloc() succeeds,
giving you UB (and probably cratering the next realloc()).

I think it's correct if you do this:

int push (int **arr, int x) {
static int count = 1;
int *p;

assert(arr);
p = realloc(*arr, sizeof x * (count+1));

if (p) {
*arr = p;
count++;
p[count - 2] = x;
p[count - 1] = 0;
return 1;
}

printf("realloc() failed while pushing %d; *arr emptied\n", x);
count = 1; /* not sure this makes sense, but it's what your code did
*/
return 0;
}

S

 

[exacube]

int push (int **arr, int x)

{
int rc, *p;
static int count = 1 + 1;

*arr = realloc (*arr, sizeof x * count); /* segfault here */
p = *arr;

rc = (p ? 1 : 0);
count = (p ? count + 1 : 1);

if (p)
{
p [count - 2] = x;
p [count - 1] = 0;
}

return rc;
}

I find that it is unnessesary to have to pass a pointer to an array of
integers. Insted, it makes more sense if you pass the array of integers
it self (not a pointer to it).

Like so:

int push (int *arr, int x) {
int rc;
static int count = 1 + 1;

arr = realloc (arr, sizeof x * count);

rc = (arr ? 1 : 0);
count = (arr ? count + 1 : 1);

if (arr)
{
arr[count - 2] = x;
arr[count - 1] = 0;
}

return rc;
}

And you could call, using:

push(arr, 5);

or something like that.


Passing an array (regardless of type) actually passes a pointer to the
first element of the array. So these two calls are the same:

push(&arr[0], 5);
push(arr, 5);

Not sure if it will work, but it's worth a try

Good Luck,
Vardhan

P.S. I'm exploring C myself. I can't garuntee that the above code will
work.

 

[websnarf]

[...] Anyway, I thing I have found the source of a segfault in my program, but
I can't see anything wrong with this code (used to "push" a value on to
the end of an array):

int push (int **arr, int x)
{
int rc, *p;
static int count = 1 + 1;

*arr = realloc (*arr, sizeof x * count); /* segfault here */
p = *arr;

rc = (p ? 1 : 0);
count = (p ? count + 1 : 1);

Do a "structured walk through". Here you will notice that you are
incrementing count to one more than the value used to allocate the size
above.

if (p)
{
p [count - 2] = x;
p [count - 1] = 0;

Meaning that count-1 actually is the offset which is one past the end
of the array which *arr is pointing at. I.e., if you just move the
count = (p ? count + 1 : 1) line down to below this if() clause I think
you will be ok.

}

return rc;
}

BTW, how are you planning on accessing the items of this array? You
don't know how long it is except for within the scope of the push()
call. Are you just going to assume that 0 is a sufficient of array
marker? I.e., are you basically disallowing 0's in your array except
as an end marker?

 

[Stephen Sprunk]

I find that it is unnessesary to have to pass a pointer to an array of
integers. Insted, it makes more sense if you pass the array of
integers
it self (not a pointer to it). ....
Not sure if it will work, but it's worth a try

It's faulty. Consider what happens when (not if) realloc() moves the
block. When that happens, push() will happily push x onto the end of
the array, but when push() returns, the new block gets leaked because
the caller's array pointer will still be the old value, which is now
invalid. That gets you UB as soon as you touch or even examine it after
that point, though it's unlikely to do anything odd until the _next_
time you call push(), at which point you probably will end up stuck
staring at the debugger and scratching your head.

S

 

[Andrew Clark]

(e-mail address removed) wrote in

Andrew Clark wrote:
[...]


Do a "structured walk through". Here you will notice that you are
incrementing count to one more than the value used to allocate the
size above.

That's intended. See below.

if (p)
{
p [count - 2] = x;
p [count - 1] = 0;

Meaning that count-1 actually is the offset which is one past the end
of the array which *arr is pointing at. I.e., if you just move the
count = (p ? count + 1 : 1) line down to below this if() clause I
think you will be ok.

}

return rc;
}

BTW, how are you planning on accessing the items of this array? You
don't know how long it is except for within the scope of the push()
call. Are you just going to assume that 0 is a sufficient of array
marker? I.e., are you basically disallowing 0's in your array except
as an end marker?

In the context of the ints in the array, 0 is not allowed, so it can
serve my purpose of an end marker. That's why I allocate 1 more than I
actually use - to store that 0.

Andrew

 

[websnarf]

(e-mail address removed) wrote in

That's intended. See below.

But you are missing the point.

if (p)
{
p [count - 2] = x;
p [count - 1] = 0;

Meaning that count-1 actually is the offset which is one past the end
of the array which *arr is pointing at. I.e., if you just move the
count = (p ? count + 1 : 1) line down to below this if() clause I
think you will be ok.

}

return rc;
}

BTW, how are you planning on accessing the items of this array? You
don't know how long it is except for within the scope of the push()
call. Are you just going to assume that 0 is a sufficient of array
marker? I.e., are you basically disallowing 0's in your array except
as an end marker?

In the context of the ints in the array, 0 is not allowed, so it can
serve my purpose of an end marker. That's why I allocate 1 more than I
actually use - to store that 0.

Change your code to the following:

int push (int **arr, int x) {
int rc, *p;
static int count = 1 + 1;

*arr = realloc (*arr, sizeof x * count); /* segfault here */
p = *arr;

rc = (p ? 1 : 0);

if (p)
{
p [count - 2] = x;
p [count - 1] = 0; /* <==== */
}

count = (p ? count + 1 : 1); /* this line has been moved down */
return rc;

}

And your segfault will magically disappear. Now, walk through the code
by hand step by step and try to figure out what happens at the /* <====
*/ line before and after the change.

 

[Barry Schwarz]

How do you call this function?

I call it like this:
...

for (i = 0; i < num; ++i)
{
/* 4. Start with the lowest number in the sieve list */
/* 5. Take the next number in the sieve list still marked prime */

/* 6. Include the number in the results list */
if (baIsPrime ) push (&resList, sieveList );

/* 7. Square the number and mark all multiples of that square as
nonprime */
nTemp = sieveList * sieveList ;
for (j = i; j < num; ++j)
{
if (sieveList [j] % nTemp == 0) baIsPrime [j] = 0;
}

/* 8. Repeat steps five through eight */
/* continue; */
}

You need to provide a compilable example that manifests the undesired
behavior, including the calling and called functions. In particular,
how is resList defined and initialized?


Remove del for email

 

[Andrew Clark]

(e-mail address removed) wrote in @i42g2000cwa.googlegroups.com:

Andrew Clark wrote:
[...]


Change your code to the following:

int push (int **arr, int x) {
int rc, *p;
static int count = 1 + 1;

*arr = realloc (*arr, sizeof x * count); /* segfault here */
p = *arr;

rc = (p ? 1 : 0);

if (p)
{
p [count - 2] = x;
p [count - 1] = 0; /* <==== */
}

count = (p ? count + 1 : 1); /* this line has been moved down */
return rc;

}

And your segfault will magically disappear. Now, walk through the code
by hand step by step and try to figure out what happens at the /* <====
*/ line before and after the change.

Yep, that was the problem. I shouldn't have incremented the count until
after assigning the array values. I noticed that earlier. Now this
funtion works as expected.

Thanks to everyone,
Andrew