CODEWITHSUNDEEP
csegmentation-fault
Ronnie Slack
Ronnie Slack

Reputation: 17

Seg Fault when copying a pointer

How come the following code result in seg fault? Basically after I copy the head pointer to temp, the head pointer gone.

typedef struct address * paddress; // defines struct pointer

void addAddressToList(paddress head, int addr[])
{
  if (head == NULL) {
    //head->addrArray = addr; // if list is initially empty
  } else {
    paddress temp;
    temp = head;
    while (temp->right != NULL) {
      temp = temp->right;  // go to end of the list
    }
    paddress newAddress = (paddress)malloc(sizeof(paddress*));
    newAddress->intAddr = addr;
    newAddress->right = NULL;    
    newAddress->left = temp;  // connect the new address
    temp->right = newAddress;
  }
}

main() {
  paddress addressListHead;
  addressListHead = (paddress)malloc(sizeof(paddress*));
  int intAddr1[] = {1,2,3,4,5,6,7};
  char hexAddr1[] = "123456";
  int intAddr2[] = {16,14,13,12,11};
  char hexAddr2[] = "fedcb";

  addressListHead->intAddr = intAddr1;
  addressListHead->hexAddr = hexAddr1;
  addAddressToList(addressListHead, intAddr2);
}

Upvotes: 0

Views: 443

Answers (4)

AnT stands with Russia
AnT stands with Russia

Reputation: 320531

There is more than one problem in your code.

Firstly, the usual advice: stop using sizeof with type names (as much as possible). Use sizeof with expressions, not types. Type names belong in declarations and nowhere else.

Your problem with memory allocation could have been prevented if you used this malloc idiom

T *p = malloc(n * sizeof *p);

i.e. sizeof should be applied to *p, where p is the pointer to the array you are allocating and n is the total number of elements in that array. That way you never have to guess what type name you should specify under sizeof (an that way your code becomes type-independent).

In your case you are allocating just one object, so the code should look as

paddress newAddress = malloc(sizeof *newAddress);

(And don't cast the result of malloc - there's absolutely no point in doing that).

Secondly, when you the head element of the list, you need to initialize all the fields. Yet you never initialize right (or left) in the head element. Hence the crash even when the correct amount of memory is allocated.

Upvotes: 1

varunl
varunl

Reputation: 20239

First error:

addressListHead = (paddress)malloc(sizeof(paddress*));

paddress* means a pointer to paddress which itself is a pointer to struct address. Hence paddress* is a pointer to a pointer to struct address. You would want to do:

addressListHead = (paddress)malloc(sizeof(struct address));

Also, I see that you made a similar mistake yesterday. Why do I get a seg fault? I want to put a char array pointer inside a struct

It's important to understand the concept of pointers properly. I would definitely recommend you to go through some tutorials on pointers. If you need help with that, let me know.

Upvotes: 0

Mahesh
Mahesh

Reputation: 34625

paddress addressListHead;
addressListHead = (paddress)malloc(sizeof(paddress*));

It seems to get rid of the compilation error, you have type casted what malloc is returning to paddress. addressListHead is a pointer, which means it can hold the address of an object but not the address of a pointer. The malloc here statement doesn't create an object. You need to change this -

addressListHead = (paddress)malloc(sizeof(paddress*));

to 

addressListHead = (paddress)malloc(sizeof(struct address));

in main and addAddressToList functions.

Segmentation fault :

else {
  paddress temp;
  temp = head;
  while (temp->right != NULL) {
    temp = temp->right;  // go to end of the list
}

I understand paddress::right is a pointer with the fact you are comparing it to NULL. But what is temp::right is initialized to. It is pointing to some garbage address and so you cannot ask for it to compare with NULL. Make it point to a valid memory location.

Upvotes: 2

Lou Franco
Lou Franco

Reputation: 89172

In main(), you want

addressListHead = (paddress)malloc(sizeof(address));

That makes sure you get enough bytes to hold an address.

Upvotes: 1

Related Questions