multiple realloc w/ malloc blows away data

[WL]

Hey, all. I'm creating an array of strings (char **argv style) on
the fly, and using realloc to create string pointers, and malloc
for the strings itself (if that makes any sense).

I'm using the construct
ptr = realloc(ptr, size);
*ptr = malloc(string_length);
strncpy(ptr, src, string_length);
to call realloc() multiple times. This should be ok, right?

Anyways, the problem I'm seeing is in (no error checking)

char **template, **temp_ptr;

/* initial template malloc */

template = realloc(template, 2 * sizeof **template);
temp_ptr = template + 1;
*temp_ptr = malloc(5);
strncpy(*temp_ptr, "abcd", 5);
printf("%s\n%s\n%d %d\n", *template, *(template+1), template, temp_ptr);

template = realloc(template, 3 * sizeof **template);
temp_ptr = template + 2;
*temp_ptr = malloc(5);
strncpy(*temp_ptr, "wxyz", 5);
printf("%s\n%s\n%s\n%d %d\n", *template, *(template+1), *(template+2), template, temp_ptr);

and the associated output is

1234
abcd
536952928 536952936
@
abcd
wxyz
536952928 536952944

After some futzing, it seems the data in *template gets blown away
after the last malloc(). I've looked this over all morning, but I
don't see where I'm going wrong.

The compiler is gcc 2.95.3 on alpha/netbsd 1.6.1.

WL
--
real mail: wliao at sdf loSnPesAtarM org
(remove the uppercase letters...)

[Juergen Heinzl]

In article <be************@ID-182745.news.uni-berlin.de>, WL wrote:

Hey, all. I'm creating an array of strings (char **argv style) on
the fly, and using realloc to create string pointers, and malloc
for the strings itself (if that makes any sense).

I'm using the construct
ptr = realloc(ptr, size);
*ptr = malloc(string_length);
strncpy(ptr, src, string_length);
to call realloc() multiple times. This should be ok, right?

Anyways, the problem I'm seeing is in (no error checking)

char **template, **temp_ptr;

/* initial template malloc */

template = realloc(template, 2 * sizeof **template);

[-]
Which means template = realloc( template, 2 * sizeof (char) ) and
since you didn't set template to NULL realloc() doesn't do the initial
allocation for you but presumes template, which may be 0xdeadbeef or
anything else here, to be a valid address.

So it'd read ...
char** template = NULL;

template = realloc( template, 2 * sizeof (*template) );

.... to start with.
[-]
Ta',
Juergen

--
\ Real name : Juergen Heinzl \ no flames /
\ EMail Private : ju*****@manannan.org \ send money instead /

[Bertrand Mollinier Toublet]

WL wrote:

Hey, all. I'm creating an array of strings (char **argv style) on
the fly, and using realloc to create string pointers, and malloc
for the strings itself (if that makes any sense).

I'm using the construct
ptr = realloc(ptr, size);
*ptr = malloc(string_length);
strncpy(ptr, src, string_length);
to call realloc() multiple times. This should be ok, right?

Anyways, the problem I'm seeing is in (no error checking)

char **template, **temp_ptr;

/* initial template malloc */

template = realloc(template, 2 * sizeof **template); Ouch ouch ouch.
There are two problems with this line.
1) The alloc'ed size is plain wrong. Think. template is declared as a
char **, right ? Thus **template has type char. Hence sizeof **template
is exactly 1. I don't think that's what you want.

2) That said, remember that realloc, just like malloc, can fail. If it
fails, it returns a NULL pointer. Thus, with your statement above you
are actually erasing the only reference you had to some alloc'ed data.
You thus can't free it anymore. I don't think that's what you want either.

The correct way to do what you want is:

temp_ptr = realloc(template, 2 * sizeof *template);
if (NULL == temp_ptr)
{
/* do some cleanup, possibly:
free(template);
although that's not likely */
}
else
{
template = temp_ptr;
/* carry on with processing */
} temp_ptr = template + 1;
*temp_ptr = malloc(5);
strncpy(*temp_ptr, "abcd", 5);
printf("%s\n%s\n%d %d\n", *template, *(template+1), template, temp_ptr);

template = realloc(template, 3 * sizeof **template); Keep in mind *alloc functions are likely to be expensive. Use them
sparingly. Instead of increasing the size of your array of string one
string pointer at a time, start with a fair size (to be determined
depending on your application's requirements), keep track of the size,
and when you run out of room, *double* the size ! temp_ptr = template + 2;
*temp_ptr = malloc(5);
strncpy(*temp_ptr, "wxyz", 5);
printf("%s\n%s\n%s\n%d %d\n", *template, *(template+1), *(template+2), template, temp_ptr);

--
Bertrand Mollinier Toublet
"Reality exists" - Richard Heathfield, 1 July 2003

[Eric Sosman]

WL wrote:

Hey, all. I'm creating an array of strings (char **argv style) on
the fly, and using realloc to create string pointers, and malloc
for the strings itself (if that makes any sense).

I'm using the construct
ptr = realloc(ptr, size);
*ptr = malloc(string_length);
strncpy(ptr, src, string_length);
to call realloc() multiple times. This should be ok, right?
No, not at all right. In the first line there's a problem
if realloc() fails and returns NULL. Guess what? You've stored
the NULL value into `ptr', and you can no longer find the memory
`ptr' used to point to. This is called a "potential memory leak."

Then in the second line, you always store the new pointer
in the very first slot of the reallocated array, clobbering
whatever lived there before. That is, the second time you do
this you lose track of the string you stored the first time,
and the third time you lose the string stored the second time,
and so on. This is called a "big-time memory leak."

The second and/or third lines may also be in error in the
use of `string_length', whose initialization you haven't shown.
If `string_length' is the strlen() of some string, then the
copied string winds up lacking a terminating '\0' character --
that is, it's no longer a "string" as defined by C, and you
dare not attempt to use it as such.

Finally, there's no error-checking. "Omitted for brevity,"
I'm sure, but considering the rate at which errors are being
committed in this code, brevity here seems to be the soul of
something other than wit.
Anyways, the problem I'm seeing is in (no error checking)

char **template, **temp_ptr;

/* initial template malloc */

template = realloc(template, 2 * sizeof **template);
First, `template' has never been initialized to point to
anything in particular. If it's at file scope it's implicitly
initialized to NULL, and that's all right. But if it's local
to a function, it "points to garbage" and you are in for big
trouble.

Second, see above for a reason to avoid `p = realloc(p,...)'.

Third, that size calculation is wrong. R-O-N-G, wrong.
Hint: the value is two. Not "two pointers' worth," but two.
You want something like `2 * sizeof *template' instead.

Fourth, "anyways" is not an English word.
temp_ptr = template + 1;
*temp_ptr = malloc(5);
Since you only allocated two bytes' worth of memory above
(if you even got that far, given that `template' was uninitialized
when you called realloc() on it), it's quite likely that `temp_ptr'
now points outside the allocated area. When you try to store the
result of malloc() there, who knows what you might be stepping
upon?
strncpy(*temp_ptr, "abcd", 5);
Not an error, exactly, but this habit you've fallen into of
using strncpy() instead of strcpy() is not a good one. The habit
of using magic numbers like `5' and `2' is perhaps even worse.
printf("%s\n%s\n%d %d\n", *template, *(template+1), template, temp_ptr);
You seem unconcerned about the lake of brimstone that awaits
persistent sinners, because here comes another one. Another
three, actually.

First, what's stored in `*template'? Well, you never stored
anything there, did you? So what reason have you to believe it
points to the start of a valid string that you can use with the
"%s" specifier? None at all, that's right ...

Second and third, what makes you think you can print a pointer
value with the "%d" specifier? "%d" is for integers, not for
pointers. If it happens to do something halfway sensible on your
particular system, that's pure dumb luck.
template = realloc(template, 3 * sizeof **template);
Same errors in the previous realloc(). You're up to a whole
three bytes now -- and note that if realloc() decided to copy the
old stuff to a new location, it has not obligingly copied the
things you stored outside the bounds of the old area ...
temp_ptr = template + 2;
*temp_ptr = malloc(5);
strncpy(*temp_ptr, "wxyz", 5);
printf("%s\n%s\n%s\n%d %d\n", *template, *(template+1), *(template+2), template, temp_ptr);
All the same errors, all over again.
and the associated output is
Considering all the problems, it's a bit surprising that
you got any output at all.
1234
abcd
536952928 536952936
@
abcd
wxyz
536952928 536952944

After some futzing, it seems the data in *template gets blown away
after the last malloc(). I've looked this over all morning, but I
don't see where I'm going wrong.

If you can't see what's wrong with what you've written, you
are in serious need of a C course. Pointers are obviously a
mystery to you, and memory management seems to be a dark art.
Read Sections 4 through 7 -- ALL of it! -- in the comp.lang.c
Frequently Asked Questions (FAQ) list

http://www.eskimo.com/~scs/C-faq/faq.html

.... and it would do you no lasting harm to read Section 8 as
well. Until you improve your understanding, you are just
wasting your time writing stuff that looks like code but lacks
sensible content.

--
Er*********@sun.com

[Al Bowers]

WL wrote:

Hey, all. I'm creating an array of strings (char **argv style) on
the fly, and using realloc to create string pointers, and malloc
for the strings itself (if that makes any sense).

I'm using the construct
ptr = realloc(ptr, size);
*ptr = malloc(string_length);
strncpy(ptr, src, string_length);
to call realloc() multiple times. This should be ok, right?

Anyways, the problem I'm seeing is in (no error checking)

char **template, **temp_ptr;

/* initial template malloc */

template = realloc(template, 2 * sizeof **template);
temp_ptr = template + 1;
*temp_ptr = malloc(5);
strncpy(*temp_ptr, "abcd", 5);
printf("%s\n%s\n%d %d\n", *template, *(template+1), template, temp_ptr);

template = realloc(template, 3 * sizeof **template);
temp_ptr = template + 2;
*temp_ptr = malloc(5);
strncpy(*temp_ptr, "wxyz", 5);
printf("%s\n%s\n%s\n%d %d\n", *template, *(template+1), *(template+2), template, temp_ptr);

and the associated output is

1234
abcd
536952928 536952936
@
abcd
wxyz
536952928 536952944

After some futzing, it seems the data in *template gets blown away
after the last malloc(). I've looked this over all morning, but I
don't see where I'm going wrong.

The compiler is gcc 2.95.3 on alpha/netbsd 1.6.1.

Nothing is wrong with the compiler. You have the allocations
and printfs in the code all messed up with undefined behavior.
Here is an example of how you may want to proceed.

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

int main(void)
{
char **template, **temp_ptr;
unsigned i, count = 0;

template = malloc((count+1)*sizeof(*template));
if(template == NULL) /* Exit the program */
exit(EXIT_FAILURE);
if((template[count] = malloc(5)) == NULL)
{ /* Deallocate and exit */
free(template);
exit(EXIT_FAILURE);
}
strcpy(template[count],"abcd");
count++;
temp_ptr = realloc(template,(count+1)*sizeof(*template));
if(temp_ptr == NULL)
{ /* Free the allocations and Exit */
for(i = 0; i < count;i++) free(template[i]);
free(template);
exit(EXIT_FAILURE);
}
template = temp_ptr;
if((template[count] = malloc(5)) == NULL)
{
for(i = 0; i < count;i++) free(template[i]);
free(template);
exit(EXIT_FAILURE);
}
strcpy(template[count],"wxyz");
count++;
for(i = 0;i < count;i++)
printf("template[%u] = \"%s\"\n",i,template[i]);

/* Free the allocations */
for(i = 0; i < count;i++) free(template[i]);
free(template);
return 0;
}

--
Al Bowers
Tampa, Fl USA
mailto: xa*@abowers.combase.com (remove the x)
http://www.geocities.com/abowers822/

[Wenchi]

Eric Sosman <Er*********@sun.com> wrote:

WL wrote:

Hey, all. I'm creating an array of strings (char **argv style) on
the fly, and using realloc to create string pointers, and malloc
for the strings itself (if that makes any sense).

strncpy(*temp_ptr, "abcd", 5);

Not an error, exactly, but this habit you've fallen into of
using strncpy() instead of strcpy() is not a good one. The habit
of using magic numbers like `5' and `2' is perhaps even worse.

Why is using strncpy instead of strcpy bad? Doesn't declaring the
length keep the destination buffer safer compared to strcpy?

Thanks for all the comments (in the other followups as well). After
more reading and trying, here is what I ended up with, and it works
on 2 specific compiler/platform combination I'm testing it on.

Critique away :)

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

int main(int ac, char **av)
{
char **template = NULL, **temp_ptr = NULL, **ptr = NULL;

template = malloc(sizeof *template);
if (template == NULL) { exit(1); }
*template = malloc(5);

if (*template == NULL) { exit(1); }
strncpy(*template, "1234", 5);

ptr = realloc(template, 2 * sizeof *template);
if (ptr == NULL) { exit(1); }
template = ptr;
temp_ptr = ptr + 1;
*temp_ptr = malloc(5);
if (*temp_ptr == NULL) { exit(1); }
strncpy(*temp_ptr, "abcd", 5);

ptr = realloc(template, 3 * sizeof *template);
if (ptr == NULL) { exit(1); }
template = ptr;
temp_ptr = ptr + 2;
*temp_ptr = malloc(5);
if (*temp_ptr == NULL) { exit(1); }
strncpy(*temp_ptr, "wxyz", 5);

printf("%s\n%s\n%s\n", *template, *(template+1), *(template+2));

return 0;
}

WL
--
real mail: wliao at sdf loSnPesAtarM org
(remove the uppercase letters...)

[Default User]

Eric Sosman wrote:

Until you improve your understanding, you are just
wasting your time writing stuff that looks like code but lacks
sensible content.

Oooo, dang that's good. If I had a .sig I'd snag that, but I don't so I
won't.


Brian Rodenborn

[Christian Bau]

In article <be************@ID-182745.news.uni-berlin.de>,
Wenchi <wl***@otaku.freeshell.org> wrote:

Eric Sosman <Er*********@sun.com> wrote:

WL wrote:

Hey, all. I'm creating an array of strings (char **argv style) on
the fly, and using realloc to create string pointers, and malloc
for the strings itself (if that makes any sense).

strncpy(*temp_ptr, "abcd", 5);

Not an error, exactly, but this habit you've fallen into of
using strncpy() instead of strcpy() is not a good one. The habit
of using magic numbers like `5' and `2' is perhaps even worse.

Why is using strncpy instead of strcpy bad? Doesn't declaring the
length keep the destination buffer safer compared to strcpy?

Read _exactly_ what strncpy does. Take note of two things: What happens
if the destination buffer is 10 Megabytes and you copy a five character
string? (Ouch, that hurts. ) What happens if the destination buffer is
five bytes and you copy a five character string? (Time bomb ticking. )

[Blah]

"Wenchi" <wl***@otaku.freeshell.org> wrote

Thanks for all the comments (in the other followups as well). After
more reading and trying, here is what I ended up with, and it works
on 2 specific compiler/platform combination I'm testing it on.

Critique away :)

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

int main(int ac, char **av)

Style comment:
main is typically 'int main(int argc, char **argv)'
while ac and av are certainly not misleading, argc and argv are clearer.

<remainder snipped>

[Al Bowers]

Wenchi wrote:

Thanks for all the comments (in the other followups as well). After
more reading and trying, here is what I ended up with, and it works
on 2 specific compiler/platform combination I'm testing it on.

Critique away :)

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

int main(int ac, char **av)
{
char **template = NULL, **temp_ptr = NULL, **ptr = NULL;

template = malloc(sizeof *template);
if (template == NULL) { exit(1); }
*template = malloc(5);

if (*template == NULL) { exit(1); }
strncpy(*template, "1234", 5);

ptr = realloc(template, 2 * sizeof *template);
if (ptr == NULL) { exit(1); }
template = ptr;
temp_ptr = ptr + 1;
*temp_ptr = malloc(5);
if (*temp_ptr == NULL) { exit(1); }
strncpy(*temp_ptr, "abcd", 5);

ptr = realloc(template, 3 * sizeof *template);
if (ptr == NULL) { exit(1); }
template = ptr;
temp_ptr = ptr + 2;
*temp_ptr = malloc(5);
if (*temp_ptr == NULL) { exit(1); }
strncpy(*temp_ptr, "wxyz", 5);

printf("%s\n%s\n%s\n", *template, *(template+1), *(template+2));

return 0;

It is not much of a critique rather a mention of my preference.
I prefer to use array indexing instead of pointer offsets in my
code. To me it makes the code easily understood it you code with
indexing.

However, your code is valid C, will compile and run as you expect.

At this point, you might consider expanding your understanding of
C by using some of the very good features of the language. I am
referring to encapsulating of data in structs and the use of
functions to do specific tasks. Your program of dynamically
allocating an array of strings would be a good example of using
these features of the language. You can make a struct that has
as members a pointer to the array and a counter to keep accurate
the number of elements(strings) in the array. Then you can write
functions to manage the struct.

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

typedef struct ARRAY
{
char **name;
unsigned count;
} ARRAY;

char *addARRAY(ARRAY *p, const char *name);
void freeARRAY(ARRAY *p);
void printARRAY(const ARRAY *p);

int main(void)
{
ARRAY presidents = {NULL, 0U}; /* initialize to begin */

addARRAY(&presidents,"George Washington");
addARRAY(&presidents,"Abe Lincoln");
addARRAY(&presidents,"Richard Nixon");
puts("\tTHE LISTED PRESIDENTS\n");
printARRAY(&presidents);
freeARRAY(&presidents);
return 0;
}

char *addARRAY(ARRAY *p, const char *name)
{
char **tmp;
unsigned i = p->count;

if((tmp = realloc(p->name,(i + 1U)*sizeof(*p->name))) == NULL)
return NULL;
p->name = tmp;
if((p->name[i] = malloc(strlen(name)+1U)) == NULL)
return NULL;
strcpy(p->name[i],name);
p->count++;
return p->name[i];
}

void freeARRAY(ARRAY *p)
{
unsigned i;

for(i = 0U;i < p->count;i++)
free(p->name[i]);
free(p->name);
p->name = NULL;
p->count = 0U;
}

void printARRAY(const ARRAY *p)
{
unsigned i;

for(i = 0U; i < p->count;i++)
puts(p->name[i]);
return;
}

--
Al Bowers
Tampa, Fl USA
mailto: xa*@abowers.combase.com (remove the x)
http://www.geocities.com/abowers822/