Trying to return a string from a queue in C/free problems

Question

I've been working on a lab for a CSC class for a while, and unfortunately I'm a bit rusty with C (as you'll probably notice from the code). I'm encountering two particular problems, both related to memory management.

1) In the dequeue operation, I'm attempting to return a string value from the node at the end of the queue. Since I'm also trying to use free() and kill off that node once I retrieve the data, I need to use a method like strcpy() to grab the data. The program segfaults whenever I try to use strcpy, and Valgrind claims invalid r/w.

2) dequeue also is not properly updating the stringQueue struct for reasons I cannot understand. I have similar code for stacks where the alterations persist, but I can keep running dequeue all day and it won't actually remove the end node.

The relevant code:

typedef struct node { 
  char data [strMax];
  struct node * next;
} queueNode;

typedef struct {
  queueNode * head;
  queueNode * tail;
} stringQueue;

char * dequeue(stringQueue *queue) {
  char * data = malloc(strMax * sizeof(char));
  if(empty(*queue)) {
    return "Null list!";
  }
  else if(!(queue->head)->next) { // One item in the queue.
    data = (queue->head)->data;
    //free(queue->head);
    queue->head = NULL;
    queue->tail = NULL;
  }
  else { // Multiple items in the queue.
    data = (queue->tail)->data;
    free(queue->tail);
    queueNode * trace = queue->head;
    while(trace->next) // Seek the last node in the queue.
      trace = trace->next;
    queue->tail = trace;
  }
  return data;
}

Answer 1

Your main problem is in lines like data = (queue->head)->data;. You can't assign array like this. you should memcpy. (strcpy is for null-terminated strings, and I guess that it's not so)

edit: you can also use strncpy, to avoid buffer-overflow.

Answer 2

You probably want to declare data as a char * = NULL at first. Then when you want to return it use data = asprintf("%s", (queue->tail)->data);. That will only do the string allocation and copying when needed, and only the required size. Then your calling code must take responsibility for freeing that data itself.

You currently have a char[] in your node struct in memory on the heap. Later on, you are setting a pointer to the data member of the struct, then freeing the struct in memory. You are left with a 'dangling pointer' that points to where the struct used to be. Trying to use that pointer will end in almost certain doom (or worse, unpredictable behaviour).

Answer 3

I see a few problems with your code...

First you do not test that your queue argument is not NULL. Then you haven't included your definition of empty() but probably testing that queue->head is NULL should tell you that the list is empty. Here you are dereferencing it prior testing it's a valid pointer, very dangerous.

Secondly, you are mallocing some data which is not used properly. When you do the affection data = (queue->head)->next; you are loosing the pointer to your allocated memory, you probably want to do a strncpy() here like strncpy(data, queue->head->data, strMax). After this you can uncomment your free(). The function calling your dequeue one will have to free() that string later when it's not used anymore. Why not allocate your data only when you are sure that the list is not empty? If you do not want to do this, you then have to free() that unsuded malloc'ed memory.

See the code below.

queueNode* find_before_tail(stringQueue* queue)
{
   queueNode* node = NULL;

   if (!queue || !queue->head)
      return NULL;

   node = queue->head;
   while (node->next != queue->tail && node->next)
      node = node->next;

   return node;
}

char * dequeue(stringQueue *queue) {
  char *data = NULL;
  queueNode* to_queue = NULL;

  if(!queue || !queue->head) {
    /* Nothing to dequeue here... */
    return NULL;
  }

  data = malloc(strMax * sizeof(char));
  if (!data) {
    printf("Error with malloc()...\n");
    return NULL;
  }

  /* Only one element */
  if(!(queue->head)->next == queue->head) {
    strncpy(data, queue->head->data, strMax);

    free(queue->head);

    queue->head = NULL;
    queue->tail = NULL;
  }
  else {
    strncpy(data, queue->tail->data, strMax);

    to_dequeue = queue->tail;

    queue->head = queue->head->next;

    queue->tail = find_before_tail(queue);
    if (!queue->tail)
       return NULL;
    queue->tail->next = NULL;

    free(to_dequeue);
  }

  data[strMax - 1] = 0;

  return data;
}

There are probably some other issues with the rest of your code, judging by this one but hopefully it gives you some basis.

EDIT WITH YOUR QUEUE CODE

Here again you are not testing the return value of malloc(). Here is a version with a non-cyclic linked list (I've also updated the dequeue() function above to work with this).

int enqueue(stringQueue *queue, char *item)
{
   queueNode * newNode = NULL;

   if (!queue || !item)
      return EINVAL;

   newNode = malloc(sizeof(queueNode));
   if (!newNode) {
      perror("malloc()");
      return errno;
   }

   strncpy(newNode->data, item, strMax);
   newNode->data[strMax - 1] = 0;

   if (!queue->head) {
       /* Element is queue and tail */
       queue->tail = newNode;
   }

   newNode->next = queue->head;
   queue->head = newNode;

   return 0; /* Everything was fine */
}

I have not tested the code but it should be very similar to this. In this scenario, when you have only one element, this_element->next is NULL and not pointing to itself.