Char Pointer Segmentation Fault

Question

Okay so here is the gist of what's happening:

I am passing in a character array (char[x]) into a function, whose argument is defined to be a character pointer (char *). Once inside the function I assign another character pointer (which is a part of a struct I have). I get a segmentation fault as soon as I assign the incoming argument, to the structure's character pointer; as such.

temp->name = (char*)malloc(sizeof(char));
temp->name = name;

This is how I've been utilizing the function previously:

char *name = "HEY";
Function(name);

Here's how I am utilizing it with errors:

char name[3] = "HEY";
Function(name);

with the same statement above, and it works fine. I made sure it wasn't anything else, by changing name to a constant "HEY", with the same input and everything went smoothly.

If anybody can think of a reason why off the top of their head, I would greatly appreciate the help. Thank you!

Here's the function in full:

• openList is the pointer to the beginning of a linked list of structs
• tempOpen is a temporary pointer we can utilize to scour through the list, without altering the openList position
• findOpenSetSID/findItem -> looks for a struct in the linked list via the SID/key
• answerOpen/answerItem -> 1 == first node, 2 == any other node, 0 = not found

Here is a breif summary of the structures involved. The open structure is a linked list of pointers to another structure (called set structures) A set structure is a linked list of names, sid's, and a item structure An item structure is a linked list of data and keys

Error_t WRITE(Sid_t sid, char *key, char *data){

 Open_p_t tempOpen = openList;      //setting a pointer to a struct 
 int answerOpen = findOpenSetSID(sid, &tempOpen);   
 if(answerOpen > 0){
    Set_p_t targetNode;             
    if(answerOpen == 1){
        targetNode = tempOpen->file;        
    }
    else{
        targetNode= tempOpen->next->file;   
    }
    Item_p_t tempItem = targetNode->items;      
    int answerItem = findItem(key, &tempItem);  
    Item_p_t targetItem;                
    targetItem = (Item_p_t)malloc(sizeof(Item_t));
    if(answerItem > 0){
        if(answerItem == 1){
            targetItem = targetNode->items;
        }
        else{
            targetItem = targetNode->items->next;
        }
        targetItem->data = data;        
    }
    else{
        **targetItem->data = data;**      <<<The problem line.
                                                      basically I am just adding   
                                                      items to my sets. But this line 
                                                      freaks out when the input changes 
                                                      from char* to char[]
        targetItem->key = key;

        targetItem->next = targetNode->items;
        targetNode->items = targetItem;
    }
    return 0;
}
return 1;
}

Here's the input segment:

char key[32], data[64]; // reads in data using fscanf(fd, "%s %s", key data) then calls WRITE(setId, key, data);

Answer 1

So let's start at the beginning. You read key,data pairs of strings from a file. You build a linked list of those pairs in the order you've read them. Then what?

/*
compile with:
gcc -std=c89 -pedantic -Wall -O2 -o so_10185705 so_10185705.c
test with:
valgrind ./so_10185705
*/
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

struct node_t {
  char key[32];
  char value[64];
  struct node_t *next;
};

static void print_list(const struct node_t *curr)
{
  while (curr) {
    printf("%s, %s\n", curr->key, curr->value);
    curr = curr->next;
  }
}

static void free_list(struct node_t **currp)
{
  struct node_t *curr = *currp;
  *currp = NULL; /* don't leave dangling pointers */
  while (curr) {
    struct node_t *next = curr->next;
    curr->next = NULL;
    free((void *)curr);
    curr = next;
  }
}

/* O(n) but useful */
static const struct node_t *
find_in_list(const struct node_t *curr, const char *key)
{
  while (curr) {
    if (!strncmp(curr->key, key, sizeof(curr->key)))
      return curr;
    curr = curr->next;
  }
  return NULL;
}

/* Same as find_in_list but less useful */
static int is_in_list(const struct node_t *curr, const char *key)
{
  while (curr) {
    if (!strncmp(curr->key, key, sizeof(curr->key)))
      return 1;
    curr = curr->next;
  }
  return 0;
}

int main()
{
  struct node_t *head = NULL;
  FILE *f = fopen("foo", "r");
  if (!f) exit(1);
  while (!feof(f)) {
    struct node_t *new_node = malloc(sizeof(struct node_t));
    fscanf(f, "%s %s", new_node->key, new_node->value);
    new_node->next = head;
    head = new_node;
  }
  fclose(f);
  print_list(head);
  const struct node_t *node = find_in_list(head, "abcd2");
  if (node) {
    printf("found! key = %s, value = %s\n", node->key, node->value);
  } else {
    printf("not found!\n");
  }
  if (is_in_list(head, "abcd3")) {
    printf("found key in list but now I don't know the value associated with this key.\n");
  } else {
    printf("not found!\n");
  }
  free_list(&head);
  return 0;
}

/* explanation of bugs in OP's code */

struct node_t_2 {
  char *key;
  char *value;
  struct node_t_2 *next;
};

void build_list(FILE *f, struct node_t_2 *curr)
{
  while (!feof(f)) {
    /*
    These variable are allocated on the stack.
    Their lifetime is limited to the current scope.
    At the closing curly brace of the block in which they are declared,
    they die, the information in them is lost and pointer to them become
    invalid garbage.
    */
    key char[32];
    value char[64];
    /*
    Of course, you can only read keys up to 31 bytes long and values up to 63 bytes long.
    Because you need an extra byte for the string's NUL terminator.
    fscanf puts that NUL terminator for you.
    If it didn't, you would not be able to use the data:
     you would not know the lenth of the string.
    If you need 32-byte long keys, declare the variable key to be 33 bytes long.
    */
    fscanf(f, "%s %s", key, value);
    /* You can use key and value here */
    struct node_t_2 bad_new_node;
    /*
    You cannot add bad_new_node to the list, because as soon as you
    reach '}' it will die.
    You need a place for the node that will not disappear
     (be reused on the next iteration of the loop).
    So it must be on the heap.
    How many bytes do you need for the node? Enough to hold the three pointers:
     12 bytes on 32bit, 24 bytes on 64bit.
    The compiler knows how many bytes a struct needs.
    */
    struct node_t_2 *new_node = malloc(sizeof(struct node_t_2));
    /*
    You can add new_node to the list, because it is on the heap and will
     exist until either passed to free() or the process (program) exits.
    */
    new_node->key = key;
    new_node->value = value;
    /*
    That was a bug, because we stored pointers to garbage.
    Now new_node has a pointer to a place that will cease to exist
    as soon as we reach '}'.
    */
    new_node->key = strdup(key);
    new_node->value = strdup(value);
    /*
    strdup is a standard function that can be implemented like this:
    char * strdup(const char *s)
    {
      int len = strlen(s)
      char *p = malloc(len);
      memcpy(p, s, len);
      return p;
    }
    Now new_node has pointers to memory on the heap that will continue to
    exist until passed to free or the process terminates.
    */
    new_node->next = curr;
    curr = new_node;
    /*
    At the next line, memory for key and value is disappears and
     is re-used if we re-enter the loop.
    */
  }
}

/*
If your list nodes have pointers instead of arrays, you need to free
the strings pointed to by those pointers manually, becuause freeing
the list node wont free stuff it points to.
*/
free_list(struct node_t_2 **currp)
{
  struct node_t_2 *curr = *currp;
  *currp = NULL;
  while (curr) {
    struct node_t_2 *next = curr->next;
    free((void *)curr->key);
    curr->key = NULL;
    free((void *)curr->value);
    curr->value = NULL;
    curr->next = NULL;
    free((void *)curr);
    curr = next;
  }
}

Answer 2

Not sure from the sniplet as to how temp->name is declared, however the problem is probably here;

 name = (char*)malloc(sizeof(char));

As size of char is only one byte, and depending on what you want to store, you either need the space for a full pointer (4 or 8 bytes), or a sequence of chars if you expect to copy content into the allocated space;

So

 name = (char*)malloc(sizeof(char *));

Or

 name = (char*)malloc(sizeof(char) * 80 ); // for 80 bytes char array

Answer 3

Firstly, these two lines:

temp->name = (char*)malloc(sizeof(char));
temp->name = name;

The top line is useless and will cause a memory leak.

Secondly, these two lines:

char *name = "HEY";
char name[3] = "HEY";

are close but not identical. The first results in name pointing to a 4-byte chunk of data with the string "HEY" and a null terminator (value 0 or '\0') at the end. The second results in name pointing to a 3-byte chunk of memory with the bytes "HEY" and no null terminator.

If your function assumes that it is getting a null-terminated string (more than likely), then the second variant will probably cause a segfault.