0

I'm trying to populate a linked list of structs from a global variable source and I get a BAD_ACCESS at the strcpy() line. Using C. Wondering if anyone could point out the issue.

The global struct is declared as so:

#include "data_structs.h"
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define LINE_LEN   256
#define NUL        '\0'

table_entry_t reg_list[]=
{
{"R0",0},{"PC",0},{"R1",1},{"SP",1},{"R2",2},{"SR",2},{"R3",3},
{"R4",4},{"R5",5},{"R6",6},{"R7",7},{"R8",8},{"R9",9},{"R10",10},
{"R11",11},{"R12",12},{"R13",13},{"R14",14},{"R15",15},{"R16",16}
};

... The structs looks like so (below) and are defined in a .h file.

typedef struct
{
    char label[20];
    int address;

}table_entry_t;

typedef struct
{
    table_entry_t *data;
    void *next;
} List_node_t;

typedef struct
{
    List_node_t *head;
}list_t;

The linked list is initialized using: (below). The EXC_BAD_ACCESS occurs at the line "strcpy(new_node->data->label,reg_list[i].label);"

boolean List_init (list_t *list)
{
int all_ok = False;
int i=0;
char* temp[3];

if (list != NULL) {
    list->head = NULL;

    //Add Register Labels

    while(i<20)   // 20 register labels
    {
        List_node_t *new_node = NULL;    
        new_node = (List_node_t *) malloc( sizeof( List_node_t));
        strcpy(new_node->data->label,reg_list[i].label); <---BAD ACCESS
        new_node->data->address = reg_list[i].address; 
        new_node->next = list->head->next;
        list->head->next = new_node;
    }
    all_ok = True;
}
return all_ok;
}

Appreciate the fresh set(s) of eyes. Regards.

Darrell
  • 139
  • 7
  • not allocated for `new_node->data` – BLUEPIXY May 24 '16 at 02:23
  • Allocation is properly `new_node = malloc (sizeof *new_node);` See: [**Do I cast the result of malloc?**](http://stackoverflow.com/q/605845/995714) for thorough explanation. You will need to follow it by `new_node->data = malloc (sizeof *(new_node->data));` One way to never forget an allocation is to write a `create_node (...)` function where you allocate the `node`, and any pointers in `data` requiring allocation. (you pass the values needed to initialize each as parameters to `list_init` and through to `create_node`). A `free_node` and `delete_list` can work the same way. – David C. Rankin May 24 '16 at 02:43
  • Note, `list_init` must be `list_init (list_t **list, ...)` unless you return the first node address and assign that as the list address back in the calling function. Why? The address of the first node is the `list` address. Unless you pass the *address of* the list (i.e. `list_init (&list, ...)`) the address returned from `malloc` will never be reflected in `main` (or unless your *type* for `list_init` is `list *` and your return and assign the first node address back in `main`) – David C. Rankin May 24 '16 at 02:51
  • @DavidC.Rankin, thank you for the advice. It did indeed fix the issue. – Darrell May 26 '16 at 16:27
  • Glad it helped. That is one of the primary *gotchas* with people learing lists. If you change the 1st node, you are changing the *list* address and must make sure your changes to the first node are reflected/available in the calling function (usually `main()`). When you create your 1st node -- the list address changes, when you delete the 1st node, your list address changes. That is why you must pass the *address of* the list to any function that does either (or return and assign the new 1st node as the list address) But, if you delete the whole list, you don't have to worry about the address. – David C. Rankin May 27 '16 at 15:49

1 Answers1

1

Stylistic note: The rest of the C programming world typically uses either list_node_t or ListNode (snake notation and CamelCase respectively). Your notation is somewhat confusing.

You are allocating List_node_t, but the data pointer in it is left uninitialized. You lucked out, and it was NULL, and you knew there was a problem. It could also have been a random address inside your program, after which any program behavior is acceptable, including if your program displayed a 3D window playing Tetris.

You could allocate that too, but this is, likely, not what you want to do. A much better approach is to change the definition of List_node_t to:

typedef struct List_node_t
{
    table_entry_t data;
    struct List_node_t *next;
} List_node_t;

I've done two changes. The first and important one is that I changed data to be inlined, so one allocation allocates both the data and the node. You, obviously, also need to adjust the syntax for accessing it in the rest of the code.

The second is also kinda stylistic, but I highly recommend it. The pointer to next should be typed correctly. You cannot access the typedef from within the struct definition, but you can access the struct itself by name. To that end, I gave the struct a name (which is the same as the typedef), and used that to type the next pointer.

Shachar Shemesh
  • 8,193
  • 6
  • 25
  • 57
  • thank you for the pointers. I had always followed my profs notation "list_node_t", but had no idea that it was standardized. My final solution ended up being a hybrid of both suggestions. – Darrell May 27 '16 at 19:13
  • @Darrell, if you believe this answered your question, please accept the answer by clicking the checkbox next to it. – Shachar Shemesh May 29 '16 at 04:34