Segmentation fault with queue in C

Question

I am getting a segmentation fault with the following code after adding structs to my queue.

The segmentation fault occurs when the MAX_QUEUE is set high but when I set it low (100 or 200), the error doesn't occur. It has been a while since I last programmed in C, so any help is appreciated.

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

#define MAX_QUEUE 1000

struct myInfo {
        char data[20];
};

struct myInfo* queue;
void push(struct myInfo);
int queue_head = 0;
int queue_size = 0;

int main(int argc, char *argv[])
{
        queue = (struct myInfo*) malloc(sizeof(struct myInfo) * MAX_QUEUE);

        struct myInfo info;
        char buf[10];
        strcpy(buf, "hello");

        while (1)
        {
                strcpy(info.data, buf);
                push(info);
        }
}

void push(struct myInfo info) {
        int next_index = sizeof(struct myInfo) * ((queue_size + queue_head) % MAX_QUEUE);
        printf("Pushing %s to %d\n", info.data, next_index);
        *(queue + (next_index)) = info;
        queue_size++;
}

Output:

Pushing hello to 0
Pushing hello to 20
...
Pushing hello to 7540
Pushing hello to 7560
Pushing hello to 7580
Segmentation fault

Since you are using a static size for the queue, it might be wiser to declare it as struct myInfo queue[MAXQUEUE] and reference elements with expressions like queue[next_index]. Though I haven't programmed in C in a while either. — Dave Costa
– Dave Costa, Commented May 11, 2010 at 15:19

paxdiablo · Accepted Answer · 2010-05-11 15:12:06Z

4

I think your problem lies here:

int next_index = sizeof(struct myInfo) * ...
*(queue + (next_index)) = info;

You're scaling next_index by the size of your structure but this is something done automatically by that second statement - *(queue + (next_index)) is equivalent to queue[next_index] and the latter is more readable to all but those of us who have been using C since K&R was first published :-)

In other words, next_index should be a value from 0 to MAX_QUEUE-1, so try changing the first statement to remove the multiplication by sizeof(struct myInfo):

void push(struct myInfo info) {
    int next_index = (queue_size + queue_head) % MAX_QUEUE;
    printf("Pushing %s to %d\n", info.data, next_index);
    queue[next_index] = info;
    queue_size++;
}

And keep in mind that you'll eventually overflow queue_size in that infinite loop of yours. You will presumably be checking to ensure that queue_size is not incremented beyond MAX_QUEUE in the final production-ready code, yes?

edited May 11, 2010 at 15:12

answered May 11, 2010 at 15:03

paxdiablo

888k243 gold badges1.6k silver badges2k bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Trevor Over a year ago

Thanks, that was the problem! And yes, I have other code for checking the queue_size and alerting when the queue is full. It wasn't playing a factor in this case, so I didn't include it. :)

JSBձոգչ · Accepted Answer · 2010-05-11 15:08:04Z

1

You're multiplying next_index by sizeof(struct myInfo), which isn't necessary. When you add to a pointer type, the offset is automatically calculated in terms of the size of the pointed-to object. Changing the first line of push() should be sufficient:

int next_index = (queue_size + queue_head) % MAX_QUEUE;

answered May 11, 2010 at 15:08

JSBձոգչ

41.6k19 gold badges106 silver badges173 bronze badges

Comments

Thomas · Accepted Answer · 2010-05-11 15:10:05Z

0

void push(struct myInfo info) {
        int next_index = (queue_size + queue_head) % MAX_QUEUE;
        printf("Pushing %s to %d\n", info.data, next_index);
        queue[next_index] = info;
        queue_size++;
}

Also, you don't need that temporary buf:

int main(int argc, char *argv[])
{
        queue = (struct myInfo*) malloc(sizeof(struct myInfo) * MAX_QUEUE);

        while (1)
        {
                struct myInfo info; /* Seems you're using C99 so we can declare here */
                strcpy(info.data, "hello");
                push(info);
        }
}

edited May 11, 2010 at 15:10

answered May 11, 2010 at 15:04

Thomas

183k57 gold badges383 silver badges510 bronze badges

Comments

tomlogic · Accepted Answer · 2010-05-11 15:10:11Z

0

*(queue + (next_index)) = info;

queue is a pointer to a struct myInfo. You only need to add 1 to it to get the next address -- you're treating it like a char *.

You can just do:

*(queue + queue_size++) = info;

answered May 11, 2010 at 15:10

tomlogic

11.7k3 gold badges36 silver badges60 bronze badges

1 Comment

Thomas Over a year ago

No you can't: the queue is circular and starts at index queue_head inside the array.

smountcastle · Accepted Answer · 2010-05-11 15:12:49Z

0

You can treat queue as an array and then it should be simple to push items:

void push(struct myInfo info) {
   if (queue_size < MAX_QUEUE) {
     printf("Pushing %s to %d\n", info.data, queue_size);
     queue[queue_size] = info;
     queue_size++;
   } else {
     printf("ERROR: Queue is full.\n");
     /* alternatively you could have a queue_insertion_point
        variable to keep track of where you are in the queue
        and use that as your index into your array. You'd then
        reset it to 0 (to wrap around) when it hit MAX_QUEUE. 
        You need to ensure you don't overwrite data currently
        in the queue by comparing it against queue_head */
   }
}

answered May 11, 2010 at 15:12

smountcastle

6205 silver badges14 bronze badges

Collectives™ on Stack Overflow

Segmentation fault with queue in C

5 Answers 5

1 Comment

Comments

Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

1 Comment

Comments

Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related