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;
}
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 createframe
should 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.
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.
Frame
and the FrameNode
.NULL
, which can be safely passed to free()
.memmove()
(or memcpy()
) to copy the data.sizeof(*variable)
notation instead of sizeof(VariableType)
.FrameNode *
instead of the Frame *
.strncpy()
because that does not guarantee the the copied string is null terminated, which leads to problems elsewhere.STR_LEN
— it isn't clear what value that has.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;
STR_LEN
plus the null terminator.STR_LEN
.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.
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.
User contributions licensed under CC BY-SA 3.0