Memory Structure Pointer Problems

[Mike]

Hi,
I am new to C and having problems with the following program.
Basically I am trying to read some files, loading data structures into
memory for latter searching.
I am trying to use structres and arrays of pointers to them.

I have gotten the program to compile with gcc on WinXP.

If the file i read doesnt have alot of records, it runs thru. But once
i add more, it dies. In this program i have 4 files setup to read. The
program dies on the second, the number of records it dies at varies
depending on which files i give it, usually around 30-40.

Seems like it is propably some kind of memoery issue. Sorry to post
such a big piece of code. If you guys could catch some problems i might
be having or even some suggestions for storing dynamic structures of
data. I have also tried linked list, but still seem to hit the same
problem. making me think the way i am allocating memory or using it is
causing th problems.

Thanks


#include "stdio.h"

int num_stations;

typedef struct sta {
char* name;
int num_cmpnds;
struct cmp **cmpnds;
} STA;

typedef struct cmp {
char* name;
struct blk **blocks;
} CMP;

typedef struct blk {
char* name;
char* type;
} BLK;

struct O_FILE {
int i0[2];
char spc[2];
char name[14];
int type;
int i1;
int i2;
int i3;
long wf_ptr;
int i4;
} cp_rec, cmpnd_rec;

struct cmp *new_cmp(char* name ) {
struct cmp *c = malloc(sizeof(struct cmp *));
sprintf (c->name, "%s", name);
printf ("\tCreating CMPND %s [%d]\n", name, c);
return c;
}

struct sta *new_sta(char* name ) {
int ret;
int num_rec;
int gDebug=1;

FILE *cp;
char filepath[256];
struct sta *s = malloc(sizeof(struct sta *));

if (s == NULL) return NULL;
s->name = name;
printf("Creating Station [%s] [%d]\n", s->name, s);

strcpy (filepath,name);
strcat (filepath,"/");
strcat (filepath,name);
strcat (filepath,".O");

if ((cp=fopen(filepath,"rd"))==NULL) {
printf("Error opening %s\n", filepath);
exit(1);
}

fseek(cp, 0, SEEK_END);
num_rec = ftell(cp) / sizeof(cp_rec);
printf ("Mem: [%d] [%d] [%d]\n", num_rec, sizeof(struct cmp *),
num_rec * sizeof(struct cmp *));
s->cmpnds = (struct cmp**) malloc(num_rec * sizeof(struct cmp *));
fseek(cp, 0, 0);

s->num_cmpnds = 0;
while (ret=fread((char*)&cp_rec,sizeof(cp_rec),1,cp)) {
s->cmpnds[s->num_cmpnds]=new_cmp(cp_rec.name);
s->num_cmpnds++;
if (s->num_cmpnds > 10) {
break;
}
}

num_stations++;
ret=fclose(cp);
return s;
}

void print_stations(struct sta **stations) {
int s = 0;
int c = 0;
for (s=0; s<=(num_stations-1); s++) {
printf("%0d Station [%d] [%d] #[%d]\n", s, stations->name,
stations, stations->num_cmpnds);
for (c=0; c<=( stations->num_cmpnds - 1); c++) {
printf("\t%0d %0d Cmpnd [%s] [%d]\n", s, c,
stations->cmpnds[c]->name, stations->cmpnds[c]);
}
printf("END OF CMPND
----------------------------------------------------------------\n");
}
}

int main(int argc, char *argv[]) {
struct sta **stations = (struct sta **) malloc(10 * sizeof(struct sta
*));

stations[0]=new_sta("CDCU21");
print_stations(stations);
stations[1]=new_sta("6DCU23");
print_stations(stations);
stations[2]=new_sta("CDCU21");
print_stations(stations);
stations[3]=new_sta("6DCU23");
print_stations(stations);

return (0);
}

 

[Jack Klein]

Hi,
I am new to C and having problems with the following program.
Basically I am trying to read some files, loading data structures into
memory for latter searching.
I am trying to use structres and arrays of pointers to them.

I have gotten the program to compile with gcc on WinXP.

If the file i read doesnt have alot of records, it runs thru. But once
i add more, it dies. In this program i have 4 files setup to read. The
program dies on the second, the number of records it dies at varies
depending on which files i give it, usually around 30-40.

Seems like it is propably some kind of memoery issue. Sorry to post
such a big piece of code. If you guys could catch some problems i might
be having or even some suggestions for storing dynamic structures of
data. I have also tried linked list, but still seem to hit the same
problem. making me think the way i am allocating memory or using it is
causing th problems.

Thanks


#include "stdio.h"

The way to provide the appropriate prototypes and definitions for C's
standard input/output library functions, data types, and macros, is:

#include <stdio.h>

....what you have written produces undefined behavior if stdio.h is
included by reading an actual file of that name, and it finds another
file by that name first with the "stdio.h" form.

ALWAYS include standard C headers with the <name.h> form.

You also use quite a few string functions, but have failed to include
<string.h>. And you are calling malloc(), you need to include

int num_stations;

typedef struct sta {
char* name;
int num_cmpnds;
struct cmp **cmpnds;
} STA;

Get rid of the typedefs, they serve no purpose and you don't use the
aliases you create anyway.

typedef struct cmp {
char* name;
struct blk **blocks;
} CMP;

typedef struct blk {
char* name;
char* type;
} BLK;

struct O_FILE {
int i0[2];
char spc[2];
char name[14];
int type;
int i1;
int i2;
int i3;
long wf_ptr;
int i4;
} cp_rec, cmpnd_rec;

Defining a type and instances of that type in one declaration, as you
have done above, is error prone. Sooner or later it will cause you
problems.

struct cmp *new_cmp(char* name ) {

You don't modify the characters pointed to by name inside the
function, so you should change this to:

struct cpm *new_cmp(const char *name)

struct cmp *c = malloc(sizeof(struct cmp *));

You fail to check whether malloc() succeeded. Also you have a
terrible error here, one that you probably would not have made if you
had used the comp.lang.c recommended way of allocating memory.

Note the argument you are passing to malloc() above, namely the size
of a POINTER to a struct cmp. A pointer to a struct cmp is almost
certainly smaller than the size of an instance of struct cmp. So even
if your malloc() succeeds, you will eventually write beyond the end of
the allocated block when you access later members of the structure.

Change your memory allocations to look like this:

struct cmp *c = malloc(sizeof *c);

....or in the more general case where you want to allocate memory for
an array of N objects of type T:

T *t_pointer = malloc(N * sizeof *t_pointer);

sprintf (c->name, "%s", name);

Aside from your other problems, c->name is a pointer to char. It is
uninitialized, you haven't defined or allocated any memory for it to
point to. Passing an uninitialized pointer to sprintf() as a
destination is undefined behavior.

If you know that c->name points to a sufficiently sized array of
writeable characters, and you certainly don't in this case, you'd be
much better off using strcpy() than sprintf(), since you are not
actually performing any conversions.

printf ("\tCreating CMPND %s [%d]\n", name, c);

You are producing more undefined behavior by passing a pointer to
struct cmp to printf() with a "%d" conversion specifier. The only
portable way to use any of the *printf() functions to output a pointer
to any object type is to cast it to pointer to void and use the %p
conversion specifier:

printf ("\tCreating CMPND %s [%p]\n", name, (void *)c);

....and yes, in this case the cast is necessary.

return c;
}

struct sta *new_sta(char* name ) {

Again, should be const char *name.

int ret;
int num_rec;
int gDebug=1;

FILE *cp;
char filepath[256];
struct sta *s = malloc(sizeof(struct sta *));

Same problem here, you are only allocating enough memory to hold a
pointer to a struct sta, not enough memory to hold an instance of one.

if (s == NULL) return NULL;

Well at least you're checking to see if malloc() succeeded here.

s->name = name;
printf("Creating Station [%s] [%d]\n", s->name, s);

strcpy (filepath,name);
strcat (filepath,"/");
strcat (filepath,name);
strcat (filepath,".O");

Do you actually want to create a string like "name/name.O"? And you
don't check to see if the total of name twice plus the three
additional characters, plus a null terminator, will fit within a 256
byte array. If the string pointed to by name is too long, you will
overwrite the end of filepath, again producing undefined behavior.

if ((cp=fopen(filepath,"rd"))==NULL) {

The mode argument you are using, "rd", is not one defined by the C
standard. It might be an extension supported by your particular
compiler, but as far as C is concerned, it is undefined. Did you
perhaps mean "rb", which opens a file in binary mode?

printf("Error opening %s\n", filepath);
exit(1);
}

fseek(cp, 0, SEEK_END);

Assuming you really meant "rb", opening the file in binary mode, be
aware that fseek() is not guaranteed to work properly for binary
files.

num_rec = ftell(cp) / sizeof(cp_rec);

There is a hideous mix of types in the expression above. The sizeof
operator yields a size_t, and ftell() returns a signed long. Then you
assign the result to an int.

printf ("Mem: [%d] [%d] [%d]\n", num_rec, sizeof(struct cmp *),
num_rec * sizeof(struct cmp *));

More undefined behavior, passing two size_t values to printf with a
"%d" conversion specifier. size_t is an implementation-defined
unsigned integer type large enough to hold the size of an object. One
thing it is guaranteed NOT to be is a signed int.

If you have a C99 conforming compiler and library, you can print
size_t values with "%z". Otherwise, use "%lu" and cast them to
(unsigned long) in the printf() call.

s->cmpnds = (struct cmp**) malloc(num_rec * sizeof(struct cmp *));

For once you're using the correct size in a call to malloc, but
suddenly you are casting the return value. NEVER cast the return of
malloc() unless you are writing code that must also compile as C++.

fseek(cp, 0, 0);

The call above can be replaces with:

rewind(cp);

....which is much more self documenting of the intent.

s->num_cmpnds = 0;
while (ret=fread((char*)&cp_rec,sizeof(cp_rec),1,cp)) {

The return type of fread() is a size_t, not a signed int.

s->cmpnds[s->num_cmpnds]=new_cmp(cp_rec.name);
s->num_cmpnds++;
if (s->num_cmpnds > 10) {
break;
}
}

num_stations++;
ret=fclose(cp);

Why are you storing the return value of fclose() when you do nothing
with it?

return s;
}

void print_stations(struct sta **stations) {
int s = 0;
int c = 0;
for (s=0; s<=(num_stations-1); s++) {
printf("%0d Station [%d] [%d] #[%d]\n", s, stations->name,
stations, stations->num_cmpnds);
for (c=0; c<=( stations->num_cmpnds - 1); c++) {
printf("\t%0d %0d Cmpnd [%s] [%d]\n", s, c,
stations->cmpnds[c]->name, stations->cmpnds[c]);
}
printf("END OF CMPND
----------------------------------------------------------------\n");
}
}

int main(int argc, char *argv[]) {
struct sta **stations = (struct sta **) malloc(10 * sizeof(struct sta
*));

stations[0]=new_sta("CDCU21");
print_stations(stations);
stations[1]=new_sta("6DCU23");
print_stations(stations);
stations[2]=new_sta("CDCU21");
print_stations(stations);
stations[3]=new_sta("6DCU23");
print_stations(stations);

return (0);
}

There are more similar errors in the rest of the code, I just got
tired. I'm amazed it runs at all. It should die much more quickly,
the way you write past the end of allocated memory.

 

[Mike]

Thanks Jack,
For taking the time to redline my program, if it can be even called a
program, LOL!

I will review your comments and try to get things implemented better.
Some of the issues like not using my typedefs, were because i was
trying different things to limit possible errors. But as you pointed
out, i still was causing alot, LOL. I have about 10 different versions
for testing, trying different things, so each propably added a new
different bad programming technique.

Again i really appreciate you taking the time, i realize it was not a
fun task. But your comments have definitely pointed out alot of things!

Thanks
Mike

Hi,
I am new to C and having problems with the following program.
Basically I am trying to read some files, loading data structures into
memory for latter searching.
I am trying to use structres and arrays of pointers to them.

I have gotten the program to compile with gcc on WinXP.

If the file i read doesnt have alot of records, it runs thru. But once
i add more, it dies. In this program i have 4 files setup to read. The
program dies on the second, the number of records it dies at varies
depending on which files i give it, usually around 30-40.

Seems like it is propably some kind of memoery issue. Sorry to post
such a big piece of code. If you guys could catch some problems i might
be having or even some suggestions for storing dynamic structures of
data. I have also tried linked list, but still seem to hit the same
problem. making me think the way i am allocating memory or using it is
causing th problems.

Thanks


#include "stdio.h"

The way to provide the appropriate prototypes and definitions for C's
standard input/output library functions, data types, and macros, is:

#include <stdio.h>

...what you have written produces undefined behavior if stdio.h is
included by reading an actual file of that name, and it finds another
file by that name first with the "stdio.h" form.

ALWAYS include standard C headers with the <name.h> form.

You also use quite a few string functions, but have failed to include
<string.h>. And you are calling malloc(), you need to include

int num_stations;

typedef struct sta {
char* name;
int num_cmpnds;
struct cmp **cmpnds;
} STA;

Get rid of the typedefs, they serve no purpose and you don't use the
aliases you create anyway.

typedef struct cmp {
char* name;
struct blk **blocks;
} CMP;

typedef struct blk {
char* name;
char* type;
} BLK;

struct O_FILE {
int i0[2];
char spc[2];
char name[14];
int type;
int i1;
int i2;
int i3;
long wf_ptr;
int i4;
} cp_rec, cmpnd_rec;

Defining a type and instances of that type in one declaration, as you
have done above, is error prone. Sooner or later it will cause you
problems.

struct cmp *new_cmp(char* name ) {

You don't modify the characters pointed to by name inside the
function, so you should change this to:

struct cpm *new_cmp(const char *name)

struct cmp *c = malloc(sizeof(struct cmp *));

You fail to check whether malloc() succeeded. Also you have a
terrible error here, one that you probably would not have made if you
had used the comp.lang.c recommended way of allocating memory.

Note the argument you are passing to malloc() above, namely the size
of a POINTER to a struct cmp. A pointer to a struct cmp is almost
certainly smaller than the size of an instance of struct cmp. So even
if your malloc() succeeds, you will eventually write beyond the end of
the allocated block when you access later members of the structure.

Change your memory allocations to look like this:

struct cmp *c = malloc(sizeof *c);

...or in the more general case where you want to allocate memory for
an array of N objects of type T:

T *t_pointer = malloc(N * sizeof *t_pointer);

sprintf (c->name, "%s", name);

Aside from your other problems, c->name is a pointer to char. It is
uninitialized, you haven't defined or allocated any memory for it to
point to. Passing an uninitialized pointer to sprintf() as a
destination is undefined behavior.

If you know that c->name points to a sufficiently sized array of
writeable characters, and you certainly don't in this case, you'd be
much better off using strcpy() than sprintf(), since you are not
actually performing any conversions.

printf ("\tCreating CMPND %s [%d]\n", name, c);

You are producing more undefined behavior by passing a pointer to
struct cmp to printf() with a "%d" conversion specifier. The only
portable way to use any of the *printf() functions to output a pointer
to any object type is to cast it to pointer to void and use the %p
conversion specifier:

printf ("\tCreating CMPND %s [%p]\n", name, (void *)c);

...and yes, in this case the cast is necessary.

return c;
}

struct sta *new_sta(char* name ) {

Again, should be const char *name.

int ret;
int num_rec;
int gDebug=1;

FILE *cp;
char filepath[256];
struct sta *s = malloc(sizeof(struct sta *));

Same problem here, you are only allocating enough memory to hold a
pointer to a struct sta, not enough memory to hold an instance of one.

if (s == NULL) return NULL;

Well at least you're checking to see if malloc() succeeded here.

s->name = name;
printf("Creating Station [%s] [%d]\n", s->name, s);

strcpy (filepath,name);
strcat (filepath,"/");
strcat (filepath,name);
strcat (filepath,".O");

Do you actually want to create a string like "name/name.O"? And you
don't check to see if the total of name twice plus the three
additional characters, plus a null terminator, will fit within a 256
byte array. If the string pointed to by name is too long, you will
overwrite the end of filepath, again producing undefined behavior.

if ((cp=fopen(filepath,"rd"))==NULL) {

The mode argument you are using, "rd", is not one defined by the C
standard. It might be an extension supported by your particular
compiler, but as far as C is concerned, it is undefined. Did you
perhaps mean "rb", which opens a file in binary mode?

printf("Error opening %s\n", filepath);
exit(1);
}

fseek(cp, 0, SEEK_END);

Assuming you really meant "rb", opening the file in binary mode, be
aware that fseek() is not guaranteed to work properly for binary
files.

num_rec = ftell(cp) / sizeof(cp_rec);

There is a hideous mix of types in the expression above. The sizeof
operator yields a size_t, and ftell() returns a signed long. Then you
assign the result to an int.

printf ("Mem: [%d] [%d] [%d]\n", num_rec, sizeof(struct cmp *),
num_rec * sizeof(struct cmp *));

More undefined behavior, passing two size_t values to printf with a
"%d" conversion specifier. size_t is an implementation-defined
unsigned integer type large enough to hold the size of an object. One
thing it is guaranteed NOT to be is a signed int.

If you have a C99 conforming compiler and library, you can print
size_t values with "%z". Otherwise, use "%lu" and cast them to
(unsigned long) in the printf() call.

s->cmpnds = (struct cmp**) malloc(num_rec * sizeof(struct cmp *));

For once you're using the correct size in a call to malloc, but
suddenly you are casting the return value. NEVER cast the return of
malloc() unless you are writing code that must also compile as C++.

fseek(cp, 0, 0);

The call above can be replaces with:

rewind(cp);

...which is much more self documenting of the intent.

s->num_cmpnds = 0;
while (ret=fread((char*)&cp_rec,sizeof(cp_rec),1,cp)) {

The return type of fread() is a size_t, not a signed int.

s->cmpnds[s->num_cmpnds]=new_cmp(cp_rec.name);
s->num_cmpnds++;
if (s->num_cmpnds > 10) {
break;
}
}

num_stations++;
ret=fclose(cp);

Why are you storing the return value of fclose() when you do nothing
with it?

return s;
}

void print_stations(struct sta **stations) {
int s = 0;
int c = 0;
for (s=0; s<=(num_stations-1); s++) {
printf("%0d Station [%d] [%d] #[%d]\n", s, stations->name,
stations, stations->num_cmpnds);
for (c=0; c<=( stations->num_cmpnds - 1); c++) {
printf("\t%0d %0d Cmpnd [%s] [%d]\n", s, c,
stations->cmpnds[c]->name, stations->cmpnds[c]);
}
printf("END OF CMPND
----------------------------------------------------------------\n");
}
}

int main(int argc, char *argv[]) {
struct sta **stations = (struct sta **) malloc(10 * sizeof(struct sta
*));

stations[0]=new_sta("CDCU21");
print_stations(stations);
stations[1]=new_sta("6DCU23");
print_stations(stations);
stations[2]=new_sta("CDCU21");
print_stations(stations);
stations[3]=new_sta("6DCU23");
print_stations(stations);

return (0);
}

There are more similar errors in the rest of the code, I just got
tired. I'm amazed it runs at all. It should die much more quickly,
the way you write past the end of allocated memory.