Allocate memory to char * variables in structure

[dtschoepe]

Hi,

I have a homework project I am working on, so be forwarned, I'm new to
C programming. But anyway, having some trouble with a memory
allocation issue related to a char * that is a variable inside of a
structure. I keep getting segmentation fault errors and I am having
trouble understanding why. Here's the parts of the code in question...

This is part of the .h file where the struct us defined...

enum color { COLORS };
typedef enum color Color;

struct person
{
char *firstName;
char *lastName;
char *hobby;
Color favColor;
char sex;
int age;
};
typedef struct person Person;

struct node
{
struct node *next;
Person client;
};
typedef struct node Node;



Now here is the function that I am having trouble with... If you look
down to the section where this line is found...

member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));

The compiler stops there and generates the seg fault message. Here's
the full function. By the way I have tested the first part of the
function, the file pointer exists and the output to stdout from printf
is working ok until I get the line above. Then the program crashes.

Am I trying to allocate the memory in the wrong way?


Person *createMember(FILE *infile)
{
char fname[30];
char lname[30];
char sex[10];
char age[10];
char color[20];
char hobby[30];
char *colorS;
Person *member;

if (!feof(infile))
{
fgets(fname, 30, infile);
fgets(lname, 30, infile);
fgets(sex, 10, infile);
fgets(age, 10, infile);
fgets(color, 20, infile);
fgets(hobby, 30, infile);
}

else
return NULL;
printf("top of createMember\n");
/* read member attributes from file */
fgets(fname, sizeof fname, infile);
fname[strlen(fname)-1] = '\0';
fgets(lname, sizeof lname, infile);
lname[strlen(lname)-1] = '\0';
fgets(sex, sizeof sex, infile);
sex[strlen(sex)-1] = '\0';
fgets(age, sizeof age, infile);
age[strlen(age)-1] ='\0';
fgets(color, sizeof color, infile);
color[strlen(color)-1] = '\0';
fgets(hobby, sizeof hobby, infile);
hobby[strlen(hobby)-1] = '\0';
printf("got through fgets statements.\n");

/* create memory allocation for member attributes */
member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));
printf("calloc firstName\n");
member->lastName = (char *)malloc(sizeof(fname) * sizeof(char));
member->hobby = (char *)malloc(sizeof(hobby) * sizeof(char));
colorS = (char *)malloc(sizeof(color) * sizeof(char));
printf("got past malloc statements\n");

/* store member attributes */
strcpy(member->firstName, fname);
printf("fname processed\n");
sscanf(lname, "%s", member->lastName);
sscanf(sex, "%c", member->sex);
sscanf(age, "%d", member->age);
strncpy(colorS, color, sizeof(color));
member->favColor = readColor(colorS);
sscanf(hobby, "%s", member->hobby);
printf("bottom of createMember\n");

return member;
}

David

 

[Ian Collins]

Hi,

I have a homework project I am working on, so be forwarned, I'm new to
C programming. But anyway, having some trouble with a memory
allocation issue related to a char * that is a variable inside of a
structure. I keep getting segmentation fault errors and I am having
trouble understanding why. Here's the parts of the code in question...

Now here is the function that I am having trouble with... If you look
down to the section where this line is found...

member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));

Never cast the return of calloc/malloc/realloc. Also sizeof(char) is by

The compiler stops there and generates the seg fault message.

I assume you should have said the program crashes when you run it?

sscanf(sex, "%c", member->sex);
sscanf(age, "%d", member->age);

the above should use &member->sex and &ember->age.

 

[santosh]

Hi,

I have a homework project I am working on, so be forwarned, I'm new to
C programming. But anyway, having some trouble with a memory
allocation issue related to a char * that is a variable inside of a
structure. I keep getting segmentation fault errors and I am having
trouble understanding why. Here's the parts of the code in question...

This is part of the .h file where the struct us defined...

enum color { COLORS };

Only one enumeration constant?

typedef enum color Color;

struct person
{
char *firstName;
char *lastName;
char *hobby;
Color favColor;
char sex;
int age;
};
typedef struct person Person;

struct node
{
struct node *next;
Person client;
};
typedef struct node Node;



Now here is the function that I am having trouble with... If you look
down to the section where this line is found...

member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));

sizeof(char) is always one in C. Also no need to cast the return value
of calloc, (or malloc or realloc), as it can hide certain mistakes.

The compiler stops there and generates the seg fault message.

You mean compilation stops with a segmentation fault. If so the
compiler is broken.

Here's
the full function. By the way I have tested the first part of the
function, the file pointer exists and the output to stdout from printf
is working ok until I get the line above. Then the program crashes.

Am I trying to allocate the memory in the wrong way?


Person *createMember(FILE *infile)
{
char fname[30];
char lname[30];
char sex[10];
char age[10];
char color[20];
char hobby[30];
char *colorS;
Person *member;

if (!feof(infile))

In C, feof and ferror are used _after_ a call to one of the I/O
functions have reported failure.

{
fgets(fname, 30, infile);
fgets(lname, 30, infile);
fgets(sex, 10, infile);
fgets(age, 10, infile);
fgets(color, 20, infile);
fgets(hobby, 30, infile);

Check all these calls for failure. Are you also sure that the various
fields will be less than the array sizes?

}

else
return NULL;
printf("top of createMember\n");
/* read member attributes from file */
fgets(fname, sizeof fname, infile);

Why are you overwriting fname again?

fname[strlen(fname)-1] = '\0';

This will already be done for you by fgets.

fgets(lname, sizeof lname, infile);
lname[strlen(lname)-1] = '\0';
fgets(sex, sizeof sex, infile);
sex[strlen(sex)-1] = '\0';
fgets(age, sizeof age, infile);
age[strlen(age)-1] ='\0';
fgets(color, sizeof color, infile);
color[strlen(color)-1] = '\0';
fgets(hobby, sizeof hobby, infile);
hobby[strlen(hobby)-1] = '\0';
printf("got through fgets statements.\n");

All these statements are duplicates of the what you'd written earlier
within the if construct. The overwrite values read earlier with
possibly wrong values. Is this really what you want?

Also check the fgets statements for failure. Don't simply assume to
have got through safely.

/* create memory allocation for member attributes */
member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));

Here's your main error. member is pointer to a Person type, (which is
actually a struct of type person), but as yet it doesn't point to
valid instance of a Person object. You need to define a Person object,
set member to point to it and then do the memory allocation for the
fields like:

Person p1;
member = &p1;
/* ... */

printf("calloc firstName\n");
member->lastName = (char *)malloc(sizeof(fname) * sizeof(char));
member->hobby = (char *)malloc(sizeof(hobby) * sizeof(char));
colorS = (char *)malloc(sizeof(color) * sizeof(char));

Same as above. Also don't cast return value of *alloc. I'd write these
statements as:

printf("got past malloc statements\n");

Without error checking.

/* store member attributes */
strcpy(member->firstName, fname);
printf("fname processed\n");
sscanf(lname, "%s", member->lastName);

Why strcpy for firstName and sscanf for lastName?

sscanf(sex, "%c", member->sex);
sscanf(age, "%d", member->age);
strncpy(colorS, color, sizeof(color));
member->favColor = readColor(colorS);
sscanf(hobby, "%s", member->hobby);
printf("bottom of createMember\n");

return member;
}

Make sure your Person object is not a local one, since, if so, it'll
get destroyed when execution leaves this function. Either dynamically
allocate the object or make it a file scope one.

 

[dtschoepe]

Never cast the return of calloc/malloc/realloc. Also sizeof(char) is by


I assume you should have said the program crashes when you run it?


the above should use &member->sex and &ember->age.

OK, am I correct in understanding that I should be changing the code
as follows?

member->firstName = calloc(strlen(fname)+1, sizeof(char));

Notice I removed the (char *) from in front of calloc. Actually,
leaving it there is how any example in class had it. So I am confused.

I did also mean the program crashes, not the compiler stops. I am too
tired to think clearly tonight.

By the way, I just recompiled and ran the program by removing the
(char *) cast, but the same seg fault error is still there. I will
need to review my code more.

Also, I am including the stdlib.h library in my header.

David

 

[dtschoepe]

Why are you overwriting fname again?

fname[strlen(fname)-1] = '\0';

This will already be done for you by fgets.

Well, according to my handy dandy textbook, Programming in C by
Stephen Kochan, the fgets function will read everything into the array
including the newline character (\n), so I am overwriting the newline
character with the \0 character so I can eliminate the newline
character.

I am reading all the data from a file and each set of data ends with
new line character.

David

 

[Ian Collins]

OK, am I correct in understanding that I should be changing the code
as follows?

member->firstName = calloc(strlen(fname)+1, sizeof(char));

Or just

member->firstName = calloc(strlen(fname)+1, 1);

And take note of the critical bug santosh spotted that I didn't - you
have not allocated any memory for member.

Notice I removed the (char *) from in front of calloc. Actually,
leaving it there is how any example in class had it. So I am confused.

This is a common mistake. All the cast does is suppress important error
messages if you forget to include the header, nothing else.

 

[dtschoepe]

Why are you overwriting fname again?

You got me there. I wrote that code at two different times and
obviously made a boo-boo. I have deleted the second set of fgets()
statements. Although in the test this didn't hurt anything, I have
enough data in the file to cover the mistake.

I'm working on your other suggestions but to this point I'm still
getting the segfault when the program runs. I think this has something
to do with your reference to me not creating a proper Person.

If I define member using...

Person member;

What is the difference if I use member or p1? In your example, you
didn't associate member with a type. So I'm not sure what relevance
your example has? That being said, your point is a valid one, and I
would like to learn the correct way this is done. Hopefully I can
figure it out, but I just need to figure how the syntax of where I'm
wrong.

David

 

[dtschoepe]

Santos and Ian,

Thanks for your help. I did review your comments again and see where
my problems were. I am now making progress with the program.

David

 

[Ian Collins]

Santos and Ian,

Thanks for your help. I did review your comments again and see where
my problems were. I am now making progress with the program.

Good luck!

 

[santosh]

On Mar 4, 10:04 pm, "santosh" <[email protected]> wrote:

I'm working on your other suggestions but to this point I'm still
getting the segfault when the program runs. I think this has something
to do with your reference to me not creating a proper Person.


If I define member using...

Person member;

From your first post to this thread:

Person *createMember(FILE *infile)
{
char fname[30];
char lname[30];
char sex[10];
char age[10];
char color[20];
char hobby[30];
char *colorS;
Person *member;

if (!feof(infile))
{
<<<<<<<<<<<<<

As you can see, you've declared member as a _pointer_ to an object of
type Person. Now you're saying member is a Person object, not a
pointer. If so, all your accesses to the fields of member by using the
structure indirection operator, (->), is wrong. It can only be used
with pointers to structures. If member is an actual struct object then
use the dot operator to access fields like:

member.firstName = /* whatever */

What is the difference if I use member or p1?

No difference. But there *is* a difference between Person *member and
Person member. The former is a pointer to a Person object, while the
latter *is* a Person object.

In your example, you
didn't associate member with a type. So I'm not sure what relevance
your example has? That being said, your point is a valid one, and I
would like to learn the correct way this is done. Hopefully I can
figure it out, but I just need to figure how the syntax of where I'm
wrong.

This is basic structure stuff. Your textbook should have the
information. If not, then it isn't a good C textbook.

 

[Flash Gordon]

I have a homework project I am working on,

Thank you for being honest about it being homework.

> so be forwarned, I'm new to
C programming. But anyway, having some trouble with a memory
allocation issue related to a char * that is a variable inside of a
structure. I keep getting segmentation fault errors and I am having
trouble understanding why. Here's the parts of the code in question...

This is part of the .h file where the struct us defined...

enum color { COLORS };
typedef enum color Color;

struct person
{
char *firstName;
char *lastName;
char *hobby;
Color favColor;
char sex;
int age;
};
typedef struct person Person;

struct node
{
struct node *next;
Person client;
};
typedef struct node Node;



Now here is the function that I am having trouble with... If you look
down to the section where this line is found...

member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));

Firstly you do not need to cast the result of calloc/malloc/realloc. If
the compiler complains without the cast then that means you have done
something else wrong, either compiling the code as C++ (where the cast
would be needed but you should not really use malloc) or failing to

The compiler stops there and generates the seg fault message. Here's

I don't think it is the compiler that is stopping with a seg fault,
rather it is your program stopping with a seg fault when you run it.
This is a very important distinction since the compiler seg faulting
would mean either a compiler bug or a hardware fault rather than a bug
in your program.

Precision is important in programming and in reporting problems.

the full function. By the way I have tested the first part of the
function, the file pointer exists and the output to stdout from printf
is working ok until I get the line above. Then the program crashes.

Am I trying to allocate the memory in the wrong way?

Yes, but not quite wrong in the way you think.

Person *createMember(FILE *infile)
{
char fname[30];
char lname[30];
char sex[10];
char age[10];
char color[20];
char hobby[30];
char *colorS;
Person *member;

member is a pointer to a struct, but currently it does not point anywhere.

if (!feof(infile))

This is almost certainly incorrect use of feof. In C feof does not tell
you that you are at the end of the file, it tells you if your failure to
read from the file was due to being at the end of file. I.e. it only
helps after the event, not before.

{
fgets(fname, 30, infile);
fgets(lname, 30, infile);
fgets(sex, 10, infile);
fgets(age, 10, infile);
fgets(color, 20, infile);
fgets(hobby, 30, infile);

You should check whether any of the above fail. One possible "simple"
method would be getting rid of the if above and using:
if (!(fgets(...) &&
fgets(...) && ... )
return NULL;

This relies on:
1) fgets is defined as returning a null pointer on failure doe to either
EOF or some other error condition and a non-null pointer otherwise
2) A null pointer in a boolean context is treated as "false" and a
non-null pointer as "true"
3) It saves needless calls to fgets because as soon as one fails the
"short-circuit" rules of && will means that the rest are not done.

All of the above are guaranteed by the C standard.

}

else
return NULL;
printf("top of createMember\n");
/* read member attributes from file */
fgets(fname, sizeof fname, infile);

Why are you reading over fname again? Where is your error checking?

fname[strlen(fname)-1] = '\0';

What if the line was too long? You need to check that as well.

fgets(lname, sizeof lname, infile);
lname[strlen(lname)-1] = '\0';
fgets(sex, sizeof sex, infile);
sex[strlen(sex)-1] = '\0';
fgets(age, sizeof age, infile);
age[strlen(age)-1] ='\0';
fgets(color, sizeof color, infile);
color[strlen(color)-1] = '\0';
fgets(hobby, sizeof hobby, infile);
hobby[strlen(hobby)-1] = '\0';
printf("got through fgets statements.\n");

/* create memory allocation for member attributes */
member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));

Remember I said that member did not point anywhere yet? Well, it still
does not. You need to allocate space for your struct and assign the
pointer to that space to member first.

You also need to check if malloc/calloc/realloc failed.

Finally, since you are about to write a string in to the allocated space
there is no point using calloc, you might as well use malloc.

printf("calloc firstName\n");
member->lastName = (char *)malloc(sizeof(fname) * sizeof(char));
member->hobby = (char *)malloc(sizeof(hobby) * sizeof(char));
colorS = (char *)malloc(sizeof(color) * sizeof(char));
printf("got past malloc statements\n");

/* store member attributes */
strcpy(member->firstName, fname);
printf("fname processed\n");
sscanf(lname, "%s", member->lastName);
sscanf(sex, "%c", member->sex);
sscanf(age, "%d", member->age);

The above calls to sscanf are wrong, and in any case you do not need the
flexibility of sscanf. Read up on scanf, and also on the strto*
functions and remember that a string is an array of characters so you
can access an individual character using array access.

strncpy(colorS, color, sizeof(color));

ColorS does not point anywhere yet so this is a problem. In any case,
strncpy is almost certainly not what you want.

member->favColor = readColor(colorS);
sscanf(hobby, "%s", member->hobby);
printf("bottom of createMember\n");

return member;
}

I suggest you read the comp.lang.c FAQ at http://c-faq.com/
Start with sections 6, 7 and 12 which address some of your
misconceptions, then go through the rest as and when you have time. Also
keep it bookmarked as you should go back to it to look for solutions to
your problems before posting here. If you can't find, or can't
understand, the answer then ask for help.

 

[Flash Gordon]

santosh wrote, On 05/03/07 06:04:

(e-mail address removed) wrote:


Here's your main error. member is pointer to a Person type, (which is
actually a struct of type person), but as yet it doesn't point to
valid instance of a Person object. You need to define a Person object,
set member to point to it and then do the memory allocation for the
fields like:

Person p1;
member = &p1;

<snip>

You might confuse the OP my showing this method.

Make sure your Person object is not a local one, since, if so, it'll
get destroyed when execution leaves this function. Either dynamically
allocate the object or make it a file scope one.

Making it file scope is incredibly bad advice, please don't suggest to
newbies using "globals", they are needed sometimes, but definitely not
in most situations where a newbie might use them. Making it a static
(which would also would) would be bad advice as well.

Either it should be dynamically allocated or a pointer to a structure
provided by the calling routine should be passed in, depending on
exactly what the OP is doing.

 

[santosh]

santosh wrote, On 05/03/07 06:04:

<snip>

You might confuse the OP my showing this method.
Why?


Making it file scope is incredibly bad advice, please don't suggest to
newbies using "globals", they are needed sometimes, but definitely not
in most situations where a newbie might use them. Making it a static
(which would also would) would be bad advice as well.

Point taken.

 

[Flash Gordon]

santosh wrote, On 05/03/07 08:12:

Why?

<snip>

Because it is not the correct solution to the problem as you correctly
stated further down but at this point in the post (and for a fair bit
their after) the OP would probably be assuming that it was the correct
solution.

My experience with inexperienced people is that if you present an
incorrect solution and then a fair time later get around to saying it is
incorrect you confuse them, since they have been assuming it to be the
correct solution to their problem.

 

[CBFalconer]

Why are you overwriting fname again?

fname[strlen(fname)-1] = '\0';

This will already be done for you by fgets.

Well, according to my handy dandy textbook, Programming in C by
Stephen Kochan, the fgets function will read everything into the
array including the newline character (\n), so I am overwriting
the newline character with the \0 character so I can eliminate
the newline character.

I am reading all the data from a file and each set of data ends
with new line character.

But you don't handle the case where those lines are overlong. This
is an ideal place to use ggets (but not gets), which will handle
allocation and consistently suppress the terminal '\n'. You can
find it at:

<http://cbfalconer.home.att.net/download/>

--
<http://www.cs.auckland.ac.nz/~pgut001/pubs/vista_cost.txt>
<http://www.securityfocus.com/columnists/423>

"A man who is right every time is not likely to do very much."
-- Francis Crick, co-discover of DNA
"There is nothing more amazing than stupidity in action."
-- Thomas Matthews

 

[Daniel Rudy]

At about the time of 3/4/2007 9:35 PM, (e-mail address removed) stated the
following:

Hi,

I have a homework project I am working on, so be forwarned, I'm new to
C programming. But anyway, having some trouble with a memory
allocation issue related to a char * that is a variable inside of a
structure. I keep getting segmentation fault errors and I am having
trouble understanding why. Here's the parts of the code in question...

This is part of the .h file where the struct us defined...

enum color { COLORS };
typedef enum color Color;

struct person
{
char *firstName;
char *lastName;
char *hobby;
Color favColor;
char sex;
int age;
};
typedef struct person Person;

struct node
{
struct node *next;
Person client;
};
typedef struct node Node;



Now here is the function that I am having trouble with... If you look
down to the section where this line is found...

member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));

The compiler stops there and generates the seg fault message. Here's
the full function. By the way I have tested the first part of the
function, the file pointer exists and the output to stdout from printf
is working ok until I get the line above. Then the program crashes.

Am I trying to allocate the memory in the wrong way?

Yep. Just use malloc(3).

Person *createMember(FILE *infile)
{
char fname[30];
char lname[30];
char sex[10];
char age[10];
char color[20];
char hobby[30];
char *colorS;
Person *member;

if (!feof(infile))
{
fgets(fname, 30, infile);
fgets(lname, 30, infile);
fgets(sex, 10, infile);
fgets(age, 10, infile);
fgets(color, 20, infile);
fgets(hobby, 30, infile);
}

else
return NULL;
printf("top of createMember\n");
/* read member attributes from file */
fgets(fname, sizeof fname, infile);
fname[strlen(fname)-1] = '\0';
fgets(lname, sizeof lname, infile);
lname[strlen(lname)-1] = '\0';
fgets(sex, sizeof sex, infile);
sex[strlen(sex)-1] = '\0';
fgets(age, sizeof age, infile);
age[strlen(age)-1] ='\0';
fgets(color, sizeof color, infile);
color[strlen(color)-1] = '\0';
fgets(hobby, sizeof hobby, infile);
hobby[strlen(hobby)-1] = '\0';
printf("got through fgets statements.\n");

/* create memory allocation for member attributes */
member->firstName = (char *)calloc(strlen(fname)+1, sizeof(char));
printf("calloc firstName\n");
member->lastName = (char *)malloc(sizeof(fname) * sizeof(char));
member->hobby = (char *)malloc(sizeof(hobby) * sizeof(char));
colorS = (char *)malloc(sizeof(color) * sizeof(char));
printf("got past malloc statements\n");

/* store member attributes */
strcpy(member->firstName, fname);
printf("fname processed\n");
sscanf(lname, "%s", member->lastName);
sscanf(sex, "%c", member->sex);
sscanf(age, "%d", member->age);
strncpy(colorS, color, sizeof(color));
member->favColor = readColor(colorS);
sscanf(hobby, "%s", member->hobby);
printf("bottom of createMember\n");

return member;
}

David

Your statements like malloc(sizeof(fname) * sizeof(char)); can be
reduced to malloc(sizeof(fname)); since char is garunteed to be size 1.

--
Daniel Rudy

Email address has been base64 encoded to reduce spam
Decode email address using b64decode or uudecode -m

Why geeks like computers: look chat date touch grep make unzip
strip view finger mount fcsk more fcsk yes spray umount sleep

 

[dtschoepe]

Why are you overwriting fname again?
fname[strlen(fname)-1] = '\0';
This will already be done for you by fgets.

Well, according to my handy dandy textbook, Programming in C by
Stephen Kochan, the fgets function will read everything into the
array including the newline character (\n), so I am overwriting
the newline character with the \0 character so I can eliminate
the newline character.

I am reading all the data from a file and each set of data ends
with new line character.

But you don't handle the case where those lines are overlong. This
is an ideal place to use ggets (but not gets), which will handle
allocation and consistently suppress the terminal '\n'. You can
find it at:

<http://cbfalconer.home.att.net/download/>

--

In this case the instructor is guaranteeing our input will meet
certain specifications. I agree normally I would need to check for
oversized input.

David

 

[Flash Gordon]

(e-mail address removed) wrote, On 06/03/07 05:45:

In this case the instructor is guaranteeing our input will meet
certain specifications. I agree normally I would need to check for
oversized input.

My experience is that whenever one is given such a guarantee you end up
receiving input that through some error does not meet the specification.
Therefore it is best to at least do a check a check and some rudimentary
handling of bad data. As this is just homework, something as simple as
aborting the program with a suitable error message if the data is out of
spec would do. You might even get extra points for it!