populating struct data causes a segfault when freed

Question

I have some simple code which creates a new struct (containing a string and a length), and deletes that struct.

/* string/proc.c */

#include "proc.h" /* String */
#include <malloc.h>
#include <assert.h>
#include <stddef.h> /* NULL */

struct string {
        char* str;
        int   len;
};


String new_string (int length)
{
        String S;
        S = (String) malloc (sizeof (String));
        S ->str = (char*) malloc (sizeof (char*) * length + 1);
        S ->str [length] = '\0';
        S ->len = length;

        return S;
}


void delete_string (String *Sp)
{
        free ((*Sp) ->str);
        free (*Sp);
        *Sp = NULL;
}

/* end of file */

These functions are exposed through a header file, and the struct is typedef'd.

/* string/proc.h */
#ifndef string_proc_h
#define string_proc_h

typedef struct string* String;

String new_string (int length);
void delete_string (String*);

#endif
/* end of file */

I also have a test file which #include's that header file and tests the new and delete functions:

/* string/proc_test.c */

#include "proc.h"
#include <assert.h>
#include <stddef.h> /* NULL */

int test_new_string ()
{
        int ok = 0;

        String S = new_string (10);
        assert (S);
        ok ++;

        delete_string (&S);

        return ok;
}


int test_delete_string ()
{
        int ok = 0;

        String S = new_string (10);
        delete_string (&S);
        assert (S == NULL);
        ok ++;

        return ok;
}

/* end of file */

PROBLEM: When I run this program I get a Segmentation Fault (Core Dumped).

I can trace it using dbg to the proc.c file at this line:

*Sp = NULL;

HOWEVER:

When I remove this line from the proc.c file:

S ->len = length;

... both tests pass perfectly!

Why is it that the program works perfectly fine, passing the tests, but when I attempt to make a change to a struct that is in scope, it causes a seemly unrelated part of my code to segfault?

I'm not seeing how these are related... can you help me?

Answer 1

The lines

        S = (String) malloc (sizeof (String));
        S ->str = (char*) malloc (sizeof (char*) * length + 1);

are wrong. It should be:

        S = malloc (sizeof (*S));
        S ->str = malloc (sizeof (*(S->str)) * length + 1);

or

        S = malloc (sizeof (struct string));
        S ->str = malloc (sizeof (char) * length + 1);

The first line is fatal. It allocates for only one pointer while allocating for the structure is required. The second line is not fatal, but it will allocate some extra memory because it is allocating for a pointer as each element while only room for one character is required.

Also note that casting results of malloc() family is considered as a bad practice.

Another advice is that you shouldn't use typedef to hide pointer like this because it makes it more confusing.