«

Hello All,

Can anyone please explain why I ma getting segmentation fault in the
following program?


struct employee {
char *name;
struct {
int apmt;
int zip;
} addr;
};

struct employee * populate_employee(char *name, int apt, int zip) {
struct employee *emp;
emp->name = name;
emp->addr.apmt = apt;
emp->addr.zip = zip;

return emp;
}

int main() {

struct employee *emp1;

printf("From the main\n");
emp1 = populate_employee("tom", 5, 90);
printf("address of emp1 is %u\n", emp1);

printf("Values from emp1 %s, %d %d\n", emp1->name, emp1->addr.apmt,
emp1->addr.zip);
return 0;
}

If I remove the printf "From the main" then the program seg faults,
otherwise, the program executes but ends with seg fault.


Thanks for your help!

Vandana <> writes:

> Can anyone please explain why I ma getting segmentation fault in the
> following program?
>
>
> struct employee {
> char *name;
> struct {
> int apmt;
> int zip;
> } addr;
> };
>
> struct employee * populate_employee(char *name, int apt, int zip) {
> struct employee *emp;
> emp->name = name;
> emp->addr.apmt = apt;
> emp->addr.zip = zip;

emp is a pointer but where does it point? Leaving emp uninitialised
means that you can't use emp->anything. To fix this, you need to
decide how you want to manage these objects. The function could be
passed a pointer to a struct to fill in, or you could choose to
dynamically allocate a struct employee using malloc.

> return emp;
> }

<snip>
--
Ben.

pete wrote:
> #include <stdio.h>
>
> struct employee {
> char *name;
> struct {
> int apmt;
> int zip;
> } addr;
> };
>
> void populate_employee
> (struct employee *emp, char *name, int apt, int zip)
> {
> emp->name = name;
> emp->addr.apmt = apt;
> emp->addr.zip = zip;
> }

I think this is still dangerous.
What happens if the value passed to parameter name isn't a literal
string, but a dynamically allocated pointer to char, and gets freed
after the call to populate_employee() ?

I would do something like this :

#define NAME_MAX_LEN 50
struct employee {
char name[NAME_MAX_LEN];
struct {
int apmt;
int zip;
} addr;
};

void populate_employee
(struct employee *emp, char *name, int apt, int zip)
{
strncpy(emp->name, name, strlen(name), NAME_MAX_LEN);
emp->name[NAME_MAX_LEN - 1] = '\0';
emp->addr.apmt = apt;
emp->addr.zip = zip;
}

Assuming name is a properly null terminated string, and adding proper
#include's.

Ham (hoping he didn't make any mistake )

"Hamiral" <> wrote in message
news:4ba2c69c$0$21807$...
> pete wrote:

>> void populate_employee
>> (struct employee *emp, char *name, int apt, int zip)
>> {
>> emp->name = name;
>> emp->addr.apmt = apt;
>> emp->addr.zip = zip;
>> }
>
> I think this is still dangerous.
> What happens if the value passed to parameter name isn't a literal string,
> but a dynamically allocated pointer to char, and gets freed after the call
> to populate_employee() ?
>
> I would do something like this :
>
> #define NAME_MAX_LEN 50

The 50 might be a problem. Most people's names will be less than 50, so
you're wasting space. Then every so often there will be someone with a name
longer than 50, and it will be truncated.

Maybe better to allocate the space locally, of the exact length needed, and
store a copy of the name.

Or insist name is suitable for storing in the record. So the caller
allocates the space, or points to where the name is currently stored.

--
Bartc

Thanks to all.

Hamiral <> writes:
<snip>
> I would do something like this :
>
> #define NAME_MAX_LEN 50
> struct employee {
> char name[NAME_MAX_LEN];
> struct {
> int apmt;
> int zip;
> } addr;
> };
>
> void populate_employee
> (struct employee *emp, char *name, int apt, int zip)
> {
> strncpy(emp->name, name, strlen(name), NAME_MAX_LEN);

Some thing is wrong here. strncpy takes three arguments. I think you
intended to NAME_MAX_LEN as the third and last argument, though I
slightly prefer:

strncpy(emp->name, name, sizeof emp->name);

> emp->name[NAME_MAX_LEN - 1] = '\0';
> emp->addr.apmt = apt;
> emp->addr.zip = zip;
> }

<snip>
--
Ben.

On 2010-03-18, Vandana <> wrote:
> Can anyone please explain why I ma getting segmentation fault in the
> following program?

Yes.

> struct employee * populate_employee(char *name, int apt, int zip) {
> struct employee *emp;
> emp->name = name;

Where is "emp" pointing when you execute this line? Did you, for
instance, point it *at* anything?

Why, no, you didn't. It's garbage. It may or may not point anywhere
valid, and what it points at may or may not be important.

You might want to look into malloc().

-s
--
Copyright 2010, all wrongs reversed. Peter Seebach / usenet-
http://www.seebs.net/log/ <-- lawsuits, religion, and funny pictures
http://en.wikipedia.org/wiki/Fair_Game_(Scientology) <-- get educated!

Ben Bacarisse wrote:
> Some thing is wrong here. strncpy takes three arguments. I think you
> intended to NAME_MAX_LEN as the third and last argument, though I
> slightly prefer:
>
> strncpy(emp->name, name, sizeof emp->name);

You're right, I completely messed up the call to strncpy and your
version is more elegant.

Ham

bartc wrote:
> Maybe better to allocate the space locally, of the exact length needed,
> and store a copy of the name.

I considered this option, but I opted for simplicity and security,
though I messed up the call to strncpy. It would have failed on
compilation

Ham