Segmentation error for linked list

Question

My execution's name is test4.

Input:

$ ./test4 cccc zz abcdef0123456789 aaabbbcccddd

I expect to create a linked list of type char* as follow:

 ---> 12:aaabbbcccddd ---> 12:abcdef0123456789 ---> 2:zz ---> 4:cccc

each node has the form of "n:a" (n is the length of string a, n <= 12, if the length of a is more than 12 then n = 12)

Below is my code:

struct Node *prepend(struct Node *list, char *s)
{
    struct Node *node = (struct Node *)malloc(sizeof(struct Node));
    if (node == NULL)
        return NULL;

    int length_s = strlen(s);
    if (length_s > 12)
        length_s = 12;

    char* temp = NULL;
    sprintf(temp,"%d:%s", length_s, s);

    strcpy(node->data, temp);
    node->next = list;

    return node;
}

prepend function links a new node to list

struct Node {
    struct Node *next;
    char *data;
};

    int main(int argc, char **argv)
    {
        struct Node *list = NULL;

        argv++;
        while (*argv)
        {
            list = prepend(list,*argv);
            argv++;
        }
        return 0;
    }

Assume all necessary libraries and struct are included, I keep getting a segmentation error when running the code. How do I fix it? I believe the problem is in sprintf but can't figure out why.

score 2 · Answer 1 · edited May 23 '17 at 10:25

2

You don't allocate memory for temp here:

char* temp = NULL;
sprintf(temp,"%d:%s", length_s, s);

You could use either a static array of chars or dynamically allocate the memory for it.

In order to copy what you want, you should so this: If data of Node is a char*,

node->data = malloc((length_s + 1) + 1 + digits_of_length);
sprintf(node->data,"%d:%s", length_s, s);

If data of Node is an array of chars, you should do this:

sprintf(node->data,"%d:%s", ((length_s + 1) + 1 + digits_of_length), s);

As you see this gets a bit nasty, because you have to find the digits of the length too, in order to allocate memory.

Why not augmenting your Node with an extra field called str_length, which will keep track of the length of the string the current node holds. That way, you could modify your function to this:

struct Node *prepend(struct Node *list, char *s)
{
    struct Node *node = (struct Node *)malloc(sizeof(struct Node));
    if (node == NULL)
        return NULL;

    int length_s = strlen(s);

    node->data = malloc(length_s + 1);
    strcpy(node->data, temp);
    node->str_length = length_s + 1;
    node->next = list;

    return node;
}

When you go trhough this, don't forget to write a free_list(), which will de-allocates the whole list.

Also, don't cast what malloc returns. Why?

edited May 23 '17 at 10:25

Community

1
1

answered Oct 31 '14 at 04:35

gsamaras

71,951
46
188
305

So how do I copy the string "%d:%s" to node->data? this is actually the ultimate purpose of this function... – viethaihp291 Oct 31 '14 at 04:44
@viethaihp291 show the definition of `Node` *in your question post*. The answer is dependent on that definition. – WhozCraig Oct 31 '14 at 04:45
@viethaihp291, as WhozCraig said, this depends on the definition of `Node`. See my update. – gsamaras Oct 31 '14 at 04:48
@viethaihp291 check my update. I also came up with an easier way to implement this. :) – gsamaras Oct 31 '14 at 05:00
@G.Samaras can I do something like: node->data = malloc(strlen(s)+10); since "n:" can never exceed 10 in length? Also, if I malloc node->data then at the end of the function I would have to free it. But I still need the string in node->data to run in main? :) – viethaihp291 Oct 31 '14 at 05:11
Yes you could do that @viethaihp291. Yes you should free `node->data` when you free the list or delete a node. – gsamaras Oct 31 '14 at 16:57

Vlad from Moscow · Accepted Answer · 2014-10-31T05:15:21.930

1

The problem is in these statements

char* temp = NULL;
sprintf(temp,"%d:%s", length_s, s);

You did not allocate memory that will be pointed to by poointer temp. Sp the program has undefined begaviour.

You could make your life simpler if instead of the pointer in your structure you would use an array of size 12 + 1 because the length of copied strings is limited by 12 characters.

For example

enum { size = 13 };

struct Node
{
    char data[size];
    Node *next;
};

//...

struct Node *prepend( struct Node *list, const char *s )
{
    struct Node *node = ( struct Node * )malloc( sizeof( struct Node ) );
    if ( node == NULL ) return list;

    strnspy( node->data, s, size );
    node->data[size - 1] = '\0';

    node->next = list;

    return node;
}

If you need to use a pointer instead of a character array and sprintft instead of strncpy hen you can write

struct Node *prepend( struct Node *list, const char *s )
{
    const size_t N = 12;

    struct Node *node = ( struct Node * )malloc( sizeof( struct Node ) );
    if ( node == NULL ) return list;

    node->data = malloc( ( N + 1 ) * sizeof( char ) );
    sprintf( node->data, "%*.*s", N, N, s );

    node->next = list;

    return node;
}

When the list will not be needed any more you have to delete it. For example

void delete( struct Node *list )
{
    while ( list )
    {
        Node *tmp = list;
        list = list->next;

        free( tmp->data );
        free( tmp );
    }
}

edited Oct 31 '14 at 05:15

answered Oct 31 '14 at 04:39

Vlad from Moscow

301,070
26
186
335

Actually the copied string can be as long as possible but the value of n can only be up to 12. What should I do to copy whatever temp is pointing to to node->data? – viethaihp291 Oct 31 '14 at 04:43
I just updated the definition of node, it unfortunately can't be modified in this situation – viethaihp291 Oct 31 '14 at 04:52
@viethaihp291 Then you have to allocate data. For example node->data = malloc( 13 * sizeof( char ) ); And then use function strncpy. – Vlad from Moscow Oct 31 '14 at 04:54
but didn't I allocate memory to node already? also, if I malloc node->data then at the end of the function I would have to free it. But I still need the string in node->data to run in main? – viethaihp291 Oct 31 '14 at 05:09
@viethaihp291 You allocated memory only for data members of the structure. But you did not allocate memory where the string will be stored. You need to free the memory when you will delete the list. For each element of the list you need at first to free data and the element itself. – Vlad from Moscow Oct 31 '14 at 05:11
@@viethaihp291 I have shown in my post how to delete the list. – Vlad from Moscow Oct 31 '14 at 05:16

Segmentation error for linked list

2 Answers2