CODEWITHSUNDEEP
c
viethaihp291
viethaihp291

Reputation: 507

Segmentation error for linked list

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.

Upvotes: 0

Views: 104

Answers (2)

Vlad from Moscow
Vlad from Moscow

Reputation: 310910

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 );
    }
}

Upvotes: 1

gsamaras
gsamaras

Reputation: 73366

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?

Upvotes: 2

Related Questions