Thread: Unexplainable Segvault in Constructor Method

Hey all out there!
I am a kind of new to C coming from Python and Bash
(if bash is a real programming language ) and have a header file to work on String arrays/lists with big size. It's working pretty fine, excepted that my constructor is'nt working. Somtimes it returns
a structure and somtime not.
If it returns no structure I get a segfault and the programs is killed by the Kernel.
I also asked some friends, but they also don't know what the problem is.
In the file in the Appendix are only the relevant parts.
By the way:

1. I am using gcc 4.8.x
2. On Ubuntu GNU/Linux 14.04 x86_64
3. I tried also on Arch (GNU) Linux, but it didn't work, too

Code:

    #include<stdlib.h>
#include<string.h>
#include<stdio.h>

/*
   the code is documentation enough
 */


void exitMemoryError(void)
{
    printf("\n\n\n\tWARN: ERROR: OUT OF MEMORY!\n\n\n");
    exit(-105);
}

/* contains a string, kind of data element */
struct String
{
    char *value;
    unsigned int len;
};



struct String * newString(char * str)
{
    struct String * string=malloc(sizeof(struct String));
    if(! string)
    {
        exitMemoryError();
    }
    string->value=str;
    string->len=strlen(str);
    return string;
}

Post the smallest and simplest compilable program that demonstrates your problem. You should post code within [code][/code] bbcode tags.

The problem is that you allocated space for the struct, but not the data you want the struct to store. You need to use malloc again and assign to the value field. You shouldn't just set value equal to the incoming pointer, because that pointer may not remain valid. You need to copy the incoming string into memory that you've allocated.

Ah, I see that you updated your post. That's good.

Look at struct String: you have a pointer member that presumably will point to the first character of a null terminated string. No problem there. Now look at your newString function: you allocate space for a struct String object, but you did not allocate any space for the string that it should store. Then, you assign str to string->value. This is not necessarily a Bad Thing, but it probably is. To know what went wrong, we need to see the code that calls newString, which is why I told you to post the smallest and simplest compilable program that demonstrates your problem.

By the way, the .h hints that this is a header file, and if so, it should have inclusion guards. Furthermore, you should not be defining functions in the header file, but rather forward declare them and define them in a source file.

Err... you still have a problem: you allocated memory for the string wrongly, and then you overwrote the pointer to the memory allocated, resulting in a memory leak. Rather, you need a +1 when allocating memory to account for the null character, then you need to use say, strcpy to copy over the string. Since you will be calling strlen for string->len, you might as well assign to string->len first then use string->len in the malloc call.

Originally Posted by MutantJohn

Or am I thinking about this wrong?

That would only copy the first character of the string.

Originally Posted by MutantJohn

Shouldn't it be :

Code:

*(string->value) = *str;

?

no, my code is right(I tested it).

Originally Posted by LittleByBlue

no, my code is right(I tested it).

No, the code that you posted in post #6 is wrong. As I mentioned in post #7, it results in a memory leak which probably does not show up in your testing. Furthermore, it should exhibit the same problem as your code in post #1. If it does not, then it is merely by chance.

That would be correct. However, I suggest:

Code:

string->len = strlen(str);
string->value = malloc(string->len + 1);
strcpy(string->value, str);

sizeof(char) == 1 will always be true, so there is no need to multiply by that. If you really want to, then write:

Code:

string->value = malloc(sizeof(string->value[0]) * (string->len + 1));

Also, since you check the result of malloc when allocating for string, you might as well check it when allocating for string->value.

Ok thanks. Final solution is

Code:

struct String * newString(char * str)
{
        struct String * string=malloc(sizeof(struct String));
        if(! string)
        {
                exitMemoryError();
        }

        string->len=strlen(str);
        string->value=malloc(sizeof(str[0])*((string->len)+1));
        if(!string->value)
        {
                exitMemoryError();
        }
        strcpy(string->value,str);
        return string;
}

There is no point calling strlen(str) twice when the string that str points to does not change, hence my suggestion to assign to string->len first. By assigning to string->len again in the malloc call, you merely make your code unnecessarily complicated and possibly less efficient. In fact, string->len is probably now wrong, because it includes the null character when it conventionally should not.

Next, this is wrong:

Code:

free(str);

By doing this, you make it unnecessarily illegal to call newString("hello") or newString(x) where x is an array of char that stores a string. Even if malloc was used, it could well be that the caller wants to use the string, not have it deallocated unexpectedly.