Copy with string to linked list doesn't work

Question

I got a task to copy some inputs to linked lists but when I try to copy this with strncpy, it doesn't work and I got an error

Exception thrown at 0x0F4C0E15 (ucrtbased.dll) in 
ProjectA.exe: 0xC0000005: Access violation writing location 0xCDCDCDCD.

Code:

typedef struct Frame
{
    char*       name;
    unsigned int    duration;
    char*       path;  
} Frame;

typedef struct FrameNode
{
    Frame* frame;
    struct FrameNode* next;
} FrameNode;

FrameNode* createframe(char name[], char path[], int duration)
{
    Frame* list = (Frame*)malloc(sizeof(Frame));
    strncpy((list->name), name, STR_LEN);
    list->duration = duration;
    strncpy(list->path, path, STR_LEN);
    FrameNode* frame = list;
    frame->next = NULL;
    return list;
}

After the `malloc()`, `list->name` is an uninitialized pointer. You need to allocate space enough for the string and then copy into that space. Similarly with `path`. Don't forget to allow for the null bytes at the ends of the strings. — Jonathan Leffler, Jun 16 '19 at 12:45
you can show me that in code?, i dont understand what do you offer to me — d.z, Jun 16 '19 at 12:49
`malloc(sizeof(Frame));` allocates storage for a `Frame` but it doesn't allocate storage for the strings which you want to store in components `name` and `path`. Please, note, these are pointers but not arrays. Hence, `strncpy((list->name), name, STR_LEN);` is U.B. Neither you initialized component `list->name` nor you allocated storage for the data you copy into it. The same for `list->path`. — Scheff's Cat, Jun 16 '19 at 12:57
One solution could be to change `Frame` into `struct Frame { char name[STR_LEN]; unsigned int duration; char path[STR_LEN]; };` assuming that `STR_LEN` represents a sufficient constant number. The other solution would be to initialize `list->name` and `list->path` with `malloc(STR_LEN)`. — Scheff's Cat, Jun 16 '19 at 13:00

score 2 · Accepted Answer · edited Jun 16 '19 at 13:23

You need to allocate the space with malloc before copying a string in a destination. The first malloc allocates only the space for the Frame but not for its inner char *'s.

The code in your createframeshould be:

    Frame* list = malloc(sizeof(Frame));
    list->name = malloc(STR_LEN);
    strncpy((list->name), name, STR_LEN);
    list->duration = duration;
    list->path= malloc(STR_LEN);
    strncpy(list->path, path, STR_LEN);
    //FrameNode* frame = list; // <- nope. FrameNode* should point to a FrameNode not a Frame
    FrameNode* frame = malloc(sizeof(FrameNode)); 
    frame->frame = list;
    frame->next = NULL;
    return frame;

It would be also good to check that malloc has succeeded before using dynamic allocated variables, like this:

 Frame *list = malloc(sizeof(Frame));
 if(list==NULL){
     perror("problem allocating frame");
     return NULL;
 }
 list->name = malloc(STR_LEN);
 if(list->name==NULL){
     free(list);//free the already allocated memory
     perror("error message");
     return NULL;
  }
 strncpy((list->name), name, STR_LEN);
 ...
 return frame;
 }

When createframe returns you should check if it has returned NULL or not, and in case it returned NULL handle the error, usually by freeing allocated memory and terminating the program.

You should not cast the result of malloc

I don't know why OP didn't simply store `char name[STR_LEN]` and `char path[STR_LEN]` in `struct Frame`. IMHO, this would simplify things... — Scheff's Cat, Jun 16 '19 at 13:02
Note that `FrameNode *frame = list;` is a type mismatch. It probably isn't what's intended. You probably need another `malloc()` and error check, and then return `frame` not `list` (because otherwise there's another type mismatch). — Jonathan Leffler, Jun 16 '19 at 13:04

Jonathan Leffler · Answer 2 · 2019-06-16T13:18:53.433

Basic diagnosis and prescription

After the malloc(), list->name is an uninitialized pointer. You need to allocate space enough for the string and then copy into that space. Similarly with path. Don't forget to allow for the null bytes at the ends of the strings. You don't allocate space for the FrameNode; you don't return it, either.

FrameNode *createframe(char name[], char path[], int duration)
{
    Frame *list = (Frame*)malloc(sizeof(*list));
    size_t name_len = strlen(name) + 1;
    char  *name_cpy = malloc(name_len); 
    size_t path_len = strlen(path) + 1;
    char  *path_cpy = malloc(path_len);
    FrameNode *frame = malloc(sizeof(*frame));
    if (list == NULL || name_cpy == NULL || path_cpy == NULL || frame == NULL)
    {
        free(name_cpy);
        free(path_cpy);
        free(frame);
        free(list);
        return NULL;
    }
    list->duration = duration;
    memmove(path_cpy, path, path_len);
    memmove(name_cpy, name, name_len);
    list->name = name_cpy;
    list->path = path_cpy;
    frame->frame = list;
    frame->next = NULL;
    return frame;
}

There are lots of fixes in there.

It allocates space for the Frame and the FrameNode.
The code checks for allocation failures.
It tries to allocate all the memory before checking for failures, which simplifies the error handling. There's only one error return. If the first allocation fails, the chances are the rest will too, initializing the variables to NULL, which can be safely passed to free().
It calculates the length of the strings.
It allocates space for the strings and their null terminator.
Since it knows how long the strings are, it can use memmove() (or memcpy()) to copy the data.
It uses the sizeof(*variable) notation instead of sizeof(VariableType).
It returns the FrameNode * instead of the Frame *.
It avoids using strncpy() because that does not guarantee the the copied string is null terminated, which leads to problems elsewhere.
It does not pay attention to STR_LEN — it isn't clear what value that has.

Alternative designs for the structures

If you do have a fixed upper limit on the size of name and path, you'd do better with a structure like:

typedef struct Frame
{
    unsigned int    duration;
    char            name[STR_LEN + 1];
    char            path[STR_LEN + 1];  
} Frame;

Use fixed size members to save allocations.
Allow for strings of length STR_LEN plus the null terminator.
Place the character arrays at the end of the structure to minimize padding, regardless of the value of STR_LEN.
You'd then need to use strncpy() and ensure that you set list->name[STR_LEN] = '\0'; and list->path[STR_LEN] = '\0'; — and that is why there's a + 1 in the member definitions.

This variation reduces the number of allocations from 4 to 2. You might even include the next pointer in the Frame structure and do away with the FrameNode structure altogether — again reducing the amount of memory management needed and therefore simplifying the code. There may also be good reasons to keep the FrameNode separate from the Frame, but that isn't clear from the information in the question — and doesn't need to be; it is something for you to consider, that's all.

Vlad from Moscow · Answer 3 · 2019-06-16T14:51:05.687

For starters the function should be declared like

FrameNode * createframe( const char name[], const char path[], int duration );

because neither name nor path is changed in the function.

You did not allocate memory for list->name, list->path and frame.

And moreover instead of frame of the type FrameNode * the function returns list of the type Frame *.

The named constant STR_LEN does not make great sense when a character array is allocated dynamically as in your case with data members name and path of the structure Frame.

You should at first allocate dynamically memory for an object of the type FrameNode.

Then you should allocate memory for an object of the type Frame and its data members name and path.

Thus the function definition can look the following way.

FrameNode * createframe( const char name[], const char path[], int duration )
{
    FrameNode *frame = malloc( sizeof( FrameNode ) );

    if ( frame != NULL )
    {
        char *name_data = NULL;
        char *path_data = NULL;

        size_t n = strlen( name );

        name_data = malloc( n + 1 );

        if ( name_data != NULL ) strcpy( name_data, name );

        if ( name_data != NULL )
        {
            n = strlen( path );

            path_data = malloc( n + 1 );

            if ( path_data != NULL ) strcpy( path_data, path );   
        }

        Frame *list = NULL;

        if ( name_data != NULL && path_data != NULL )
        {
            list = malloc( sizeof( Frame ) );

            if ( list != NULL )
            {
                list->name     = name_data;
                list->duration = duration;
                list->path     = path_data;
            }
        }

        if ( list == NULL )
        {
            free( name_data );
            free( path_data );
            free( frame );
        }
        else
        {
            frame->frame = list;
            frame->next  = NULL; 
        }
    }        

    return frame;
}

If you indeed need to restrict the length of the dynamically allocated strings then these statements

size_t n = strlen( name );

name_data = malloc( n + 1 );

if ( name_data != NULL ) strcpy( name_data, name );

and

n = strlen( path );

path_data = malloc( n + 1 );

if ( path_data != NULL ) strcpy( path_data, path );

should be replaced with

name_data = malloc( STR_LEN );

if ( name_data != NULL ) 
{
    strncpy( name_data, name, STR_LEN );
    name_data[STR_LEN - 1] = '\0';
}

and

path_data = malloc( STR_LEN );

if ( path_data != NULL ) 
{
    strncpy( path_data, path, STR_LEN );   
    path_data[STR_LEN - 1] = '\0';
}

Otherwise there will be an inconsistence relative to stored data: some nodes will store strings while other will contain non-strings.

Copy with string to linked list doesn't work

3 Answers3

Basic diagnosis and prescription

Alternative designs for the structures