CODEWITHSUNDEEP
c
Jake
Jake

Reputation: 249

Initializing struct member of type struct pointer to null

I'm having an issue with assigning a null value to a pointer to a struct member (that is also a pointer to a struct). The following code correctly sets the headNode->right variable equal to null, but not the headNode->left variable.

typedef struct node {
    char *key;
    int frequency;
    struct node *left;
    struct node *right;
    struct node *parent;
} node;


void addKey(char key[]) {
    extern node *headNode;
    node *newNode;

    if (headNode != NULL) {
        printf("Head node is initialized\n");
        if (headNode->left != NULL) printf("    Left is not null\n");
        if (headNode->right != NULL) printf("    Right is not null\n");
    }


    newNode = malloc(sizeof(node*));
    newNode->key = malloc(sizeof(char) * (strlen(key) + 1));
    newNode->left = malloc(sizeof(node*));
    newNode->right = malloc(sizeof(node*));
    newNode->parent = malloc(sizeof(node*));


    newNode->left = newNode->right = NULL;
    newNode->frequency = 1;
    newNode->right = NULL;
    strcpy(newNode->key, key);

    // If this is the first node, assign it as the root
    if (headNode == NULL) {
        newNode->parent = NULL;
        headNode = newNode;
        return;
    }
}

However, if I add the following two lines right before the return statement, it works correctly. 

if (headNode->left == NULL) printf("L is null\n");
else printf("L is NOT null\n");

I don't understand how an if statement can make a difference. There is nowhere else that assigns or changes the value of this variable anywhere else in my code.

Upvotes: 0

Views: 81

Answers (2)

linguamachina
linguamachina

Reputation: 5803

Take a step back. There are a number of serious mistakes here.

You declare a pointer to a node:

node *newNode;

So far, so good. However, when you try to allocate memory for this new node, you only allocate enough to store a pointer, instead of the node itself:

newNode = malloc(sizeof(node*));

You already have storage for the pointer: newNode.

Change this line to:

newNode = malloc(sizeof(node));

A little later you make three more unnecessary calls to malloc:

newNode->left = malloc(sizeof(node*));
newNode->right = malloc(sizeof(node*));
newNode->parent = malloc(sizeof(node*));

If you had requested enough memory (i.e. sizeof(node)) in the previous call, there would be no need to allocate any more memory for these internal fields. You would already have memory for them; it would already exists at the address pointed to by newNode.

Worse still, you then go on to leak memory on the next line:

newNode->left = newNode->right = NULL;

Here you are throwing away the addresses of the memory you allocated unnecessarily.

I think you need to read up on memory management further before we can troubleshoot this problem for you.

Upvotes: 3

Barmar
Barmar

Reputation: 781058

All the malloc(sizeof(node*)) should be malloc(sizeof(node)). You're only alloocating enough space for a pointer, not the whole structure. This is causing undefined behavior with all the following code that indirects through these pointers.

Upvotes: 4

Related Questions