0

seg fault again!! Please see this and correct me

typedef struct entry_s
{
    int key;
    int value;
    struct entry_s *next1;
} entry_t;

entry_t *next2;
next2=malloc(sizeof(entry_t));
next2->key=key;  // this is giving seg fault now...

Thanks

Meghan
  • 303
  • 2
  • 6
  • 17
  • 5
    Did you allocate memory with `malloc` for `next2`? – pzaenger Mar 06 '14 at 11:00
  • 2
    not wrong if you have allocated memory for `next2` prior to it – mangusta Mar 06 '14 at 11:00
  • Always remember that in C if you create a pointer like in your case entry_t *next2; next2 will be given a place in memory just to store the address of variable of type entry_t. You need to either malloc it as told by others in the above comments or assign an address of the structure to it which you would have already created. Without doing any of the things you will get unexpected results – Kranthi Kumar Mar 06 '14 at 11:16
  • Never mind the segfault: Why are you trying to test the value of `next2->next1` immediately after calling `malloc`? It's just going to be uninitialized rubbish. – Roddy Mar 06 '14 at 11:32

4 Answers4

2

It's wrong as written, since there's nothing assigning a valid pointer value to next2, so you can't de-reference it: it doesn't point at a valid entry_t.

If you had some initialization, like:

const entry_t *next2 = get_entry_with_key(4711);
if(next2 != NULL)
{
  printf("got an entry with %d", next2->key);
  if(next2->next != NULL)
    printf(" and the next one has %d\n", next2->next1->key);
}

then it would be perfectly fine.

unwind
  • 391,730
  • 64
  • 469
  • 606
2

sizeof(entry_t *) gives you 4 bytes which is nothing but a size of pointer. You need to change your malloc statement to this:

next2=malloc(sizeof(entry_t ));

After malloc add the following statement. 

memset(next2, 0x00, sizeof(entry_t))

now you will not get segfault.

Kranthi Kumar
  • 1,184
  • 3
  • 13
  • 26
  • 1
    Yes to the `malloc` fix : And clearing the struct is usually a good idea - But after you've cleared it there's no need to test `next2->next1` anyway... – Roddy Mar 06 '14 at 11:34
  • Yes Roddy I agree with you. Since the code is not complete I thought there might be a code in between. So gave that it is always good to clear the structure after mallocing it. Correct me If I am wrong – Kranthi Kumar Mar 06 '14 at 11:40
  • @Kranthi Kumar: you can't say it is *always* a good idea. Sometimes zeroing might be redundand and sometimes too expensive. – jacekmigacz Mar 06 '14 at 12:39
  • @jacekmigacz but if you do not zero it there might be a possibility that we try to access the data which is not ours(when there are pointers inside the structure). When you have a very large code and when these structures are passed as params to functions and all it might create a problem. So I feel even if it is costly it is good do it to avoid problems at later stage. This is my opinion. Correct me if I am wrong. – Kranthi Kumar Mar 07 '14 at 06:01
  • 1
    @Kranthi Kumar: Sure it's desired to avoid operations on garbage. Modern languages like Objective-C are zeroing ivars by default. In C++ it's recommended to initialize auto variable at the same time it's introduced. It all depends. In embedded more often speed and saving CPU cycles has the highest priority. The "size" of source code is irrelevant here. The problems You see are usually easy to trace by using exceptions and/or debuggers. – jacekmigacz Mar 07 '14 at 14:09
  • @KranthiKumar yeah. That was the issue. Thanks..:) – Meghan Mar 10 '14 at 06:23
1

change:

next2=malloc(sizeof(entry_t *));

to

next2=malloc(sizeof(entry_t));
jacekmigacz
  • 789
  • 12
  • 22
0

The alloction size is wrong: sizeof(entry_t *) is teh size of a pointer, not the size of the ponted value (the structure). When you are accessing the next1 field of the structure, you are reading data right after the place you have allocated.

I would recommand to use the idiomatic pointer initialization form:

next2=malloc(sizeof(*next2));

It allows to change the pointed type without impact on allocation statement:

You should also test the next2 pointer righ after to check if the allocation worked well.

Additionnaly, you are testing a value which have not been initialized (next2->next1). The value of the field is unpredictable just after allocation. The most probable here is that the next1 pointer should be initilized to NULL as no memory has been allocated to store an other "struct entry_s".

jag
  • 880
  • 5
  • 6