0

I'm having some issues with my linked list. I have struct dListNode used as the nodes for the list with a pointer to struct data, which is used as the data storage.

struct data{
    int payload;
};

struct dListNode{
    struct dListNode *next;
    struct dListNode *prev;
    struct data *val;
}*dHead, *dTail;

My program compiles fine, but I get a segmentation fault at the line indicated below. What is going on?

newDNode = (struct dListNode *)malloc(sizeof(struct dListNode)+sizeof(struct data));
printf("newnode created\n"); // this prints
newDNode->val->payload = rand() % 1000; //error here?
printf("newnode payload: %i\n", newDNode->val->payload); //seg fault before this is printed

Also, I have already ran this line in the program: srand((unsigned)time(NULL))

Improve this question

4 Answers 4

Reset to default
3

NewDNode doesn't have an associated memory allocation to it. So when you do 

newDNode = (struct dListNode *)malloc(sizeof(struct dListNode)+sizeof(struct data));

This just allocates memory to newDnode and not to newDnode->val. Since newDNode->val just contains whatever was leftover in the memory at that location (or maybe even 0 (NULL pointer)), and you try to assign a value to the memory location which is neither on the stack nor on the heap, the program complains because you are trying to access unassigned part of memory.

Here's what you should do:

newDNode = malloc(sizeof(struct dListNode));
newDnode->val = malloc(sizeof(struct data));
printf("newnode created\n");
newDNode->val->payload = rand() % 1000;
printf("newnode payload: %i\n", newDNode->val->payload);

And as a tip, always try to not cast the result returned by malloc (or any other memory allocation function). Its considered bad practice.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Oh, that makes sense. Thanks for the help. I was under the impression that it is bad to NOT cast the result of malloc...
Basically, casting is very, very powerful...but use it only when you have to. Sometimes, casting (in general, casting malloc is almost always bad) is very useful and makes sense but most of the time, its misused and leads to confusion for the person who reads your code. Remember, "Code is read more that it is written".
2

You problem is that you never initialized the pointer val:

newDNode->val->payload = rand() % 1000;

newDNode is allocated, but none of the fields are initialized, so dereferencing val will likely cause that segmentation fault.

So you will need to allocate something for val before you access it.

newDNode = malloc(sizeof(struct dListNode));   //  Allocate "dListNode"
newDNode->val = malloc(sizeof(struct data));   //  Allocate "data"
newDNode->val->payload = rand() % 1000;

You have a slight misunderstanding of how the allocation works. You need to allocate each pointer separately.

EDIT : And alternate approach is just to not use a pointer for val in the first place:

//  Declare struct as:
struct dListNode{
    struct dListNode *next;
    struct dListNode *prev;
    struct data val;
}*dHead, *dTail;


//  Build object like this:
newDNode = malloc(sizeof(struct dListNode));
newDNode->val.payload = rand() % 1000;
Improve this answer

2 Comments

This newDNode = malloc(sizeof(struct dListNode)+sizeof(struct data)); is wrong. You don't need the extra space, that was due to the OP's misunderstanding of how to allocate the structure.
@EdS.: I was fixing that as you were commenting. Thanks for pointing out though.
1

val does not point to a valid data structure. Sure, you malloc enough size, but that doesn't mean that val is now all of a sudden a valid pointer. You should initialize newDNode with just the size of a dListNode and then separately initialize newDNode->val to point to some valid chunk of memory large enough for a data structure.

On a side note, you don't need to cast the return value of malloc. That's a C++ thing; in C a void* can be implicitly converted to any other pointer type.

Second, if you typedef your struct types you don't have to write struct all over the place when using them.

Improve this answer

2 Comments

But its not good practice to use void *s too much unless it makes sense right?
@mtahmed: You're not using void*'s; the return value of malloc is a void* which is implicitly being cast to a dListNode*. You should use void*'s when needed, but the cast is not required here.
0

You're never setting newDNode->val to point to anything. So when you try to set newDNode->val->payload, you're dereferencing either a null pointer or some random address (i forget which). Neither case is what you want.

I don't really like the idea of malloc'ing both the structs in the same statement. But if you insist on doing it, you'll need to do something like

newDNode->val = (struct data*)((char*) newDNode + sizeof(struct dListNode));

A better way, though, is to change the struct so that val is a struct rather than just a pointer to one. Then sizeof(struct dListNode) includes the size of struct data, and you can access it like newDListNode->val.payload without having to malloc the data part separately or do any tricky pointer math. (Drawback being, you'd have to copy the structs to store them in an array, swap them around, etc.)

Improve this answer

3 Comments

I don't see any advantage (or even a reason) to do what the OP might be trying to do...
Good point. I was pointing out how it could be done with minimal changes, but the best solution (if he wants to assume the data struct will be there immediately) is to put the data struct directly into the node struct rather than using a pointer.
Yes, that would be the best solution because he is always going to have data from the looks of it.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.