1

What am I doing wrong here?

I have a QueueElement struct containing a char* text and a pointer to the next element, so obviously a linked list: 

//QElement as in QueueElement
struct QElement {
    char* text;
    struct QElement* next;
};
typedef struct QElement QElement;

....

void enqueue (char* string){
QElement *new = (QElement *)malloc(sizeof(QElement)); 
//QElement cast is probably redundant but it doesn't hurt anyone
strcpy (new->text, string);

//some other stuff happening here, linking to other element in linked list, 
//but thats not of interest at the moment

}

....

If I try to enqueue a word in the main function, I keep getting segmentation faults and valgrind is telling me, that I'm doing something wrong when I'm using strcpy, so my malloc seems to be incorrect. How do I have to do it?

Yu Hao
  • 119,891
  • 44
  • 235
  • 294
harrisonfooord
  • 308
  • 3
  • 19
  • 1
    I know this is a C question, but even so it's generally not a good idea to use `new` as a variable name. This is a C++ keyword and will cause all kinds of problems if you ever try to compile your code as C++. – Graham Borland Jan 07 '15 at 12:29

4 Answers4

3

in this code

strcpy (new->text, string);

new->text is not allocated memory. It might contain some garbage value, or some write protected memory address or even NULL. Passing that pointer to strcpy() will lead you to Undefined behaviour and as a side effect you may experience the segmentation fault.

You can make use of strdup() or can allocate memory to new->text before strcpy()-ing to that pointer. Both the cases, you need to free() the allocated memory afterwards.

Also,

  1. Please do not cast the return value of malloc().
  2. Check for the success of dynamic memory allocation [malloc()/ calloc()] before using the pointer.
Community
  • 1
  • 1
Sourav Ghosh
  • 133,132
  • 16
  • 183
  • 261
3

strdup(.) recommended by other answers is non-standard. If you're not on a Unix platform it may not be available.

However the point is correct. You need to allocate memory to store your string.

Try:

const size_t sz=(strlen(string)+1)*sizeof(*string);//Space required. Including '\0' terminator.
new->text=malloc(sz);//space allocated.
if(new->text==NULL){//sz never 0 because +1.
   exit(EXIT_FAILURE);//Allocation failed in non-error handled function.
}
memcpy(new->text,string,sz); //typically fastest way to copy!

Instead of strdup(.).

My use of sizeof(*string) is actually unnecessary (as it is always 1) but the compiler will spot that and it's just good practice.

One day the world will move more uniformly to multi-byte characters and this code is ready for that glorious dawn!

Don't forget to free(.) when you've finished with the 'QElement'. You should probably write a function like this:

void QElementDestroy(QElement*const element){
    if(element==NULL){
        return;//Nothing to do!
    }
    free(element->text);//Needs NULL protection.
    free(element);//Does not need NULL protection. But we've done the test anyway!
    //NB: Order matters here. A lot.
}

And call it when you've finished with value returned by enqueue(.).

If you want the string to 'out live' the element set element->text=NULL before calling destroy. free(NULL) is required to do nothing and return normally.

PS: I think strdup(.) is a bit of a n00b trap. They take a while to get the hang of matching malloc(.) and free(.) and strdup(.) is a bit of a traitor because none of the other str... functions allocate space the caller is expected to free(.). It's also non-standard.

Persixty
  • 8,165
  • 2
  • 13
  • 35
1

Because you are allocating the memory for structure variable only. You have to allocate the memory in the structure member.

 new->text=malloc(10);//for example

After the allocation of memory you can use the strcpy function.

Karthikeyan.R.S
  • 3,991
  • 1
  • 19
  • 31
1

You have allocated the memory for new, but not for the text member variable. Allocate the memory for text and execute the code.

sharon
  • 734
  • 6
  • 15