segmentation fault.

[Vandana]

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!

 

[Ben Bacarisse]

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>

 

[Hamiral]

#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 )

 

[bartc]

pete wrote:

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.

 

[Vandana]

Thanks to all.

 

[Ben Bacarisse]

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>

 

[Seebs]

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

 

[Hamiral]

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

 

[Hamiral]

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