Thread: Segmentation fault!

Please do have a look at the attachment to get an idea of how it's supposed to look.

After opening a file, each line (containing <TITLE><space><PRICE><comma>) is read, and then a node is created which is to be inserted at a suitable (sorted) place. The relevant part of the code is:

Code:

typedef struct books {
    char title[TITLE_LEN];
    double price;
    struct books *nxt_price;
    struct books *nxt_title;
} booksT;


    // Each line is scanned, then 'insert' is called.
    for (i = 0, pos = 0; (feof(f) == 0); i++) {
        
        fseek(f, pos, SEEK_CUR);
        fscanf(f, "%s %lf", temp_title, &temp_price);
        insert(titles, prices, temp_title, temp_price);
        pos++;
    }

void insert (booksT *titles[26], booksT *prices[10], char tmp_title[], double tmp_price) {
    booksT *newnode, *runner;
    int index;
    
    newnode = (booksT*)malloc(sizeof(booksT*));
    if (newnode == NULL) {
        printf("Memory allocation error.\n");
        exit(1);
    }
    
    strcpy(newnode->title, tmp_title);
    newnode->price = tmp_price;
    
    // Placement according to title.
    index = newnode->title[0] - 'A';
    
    if (titles[index] == NULL) {
        titles[index] = newnode;
    }
    else {
        runner = (booksT*)malloc(sizeof(booksT*));
        if (runner == NULL) {
            printf("Memory allocation error.\n");
            exit(1);
        }
        
        strcpy(runner->title, newnode->title);
        runner = titles[index];
        
        for (runner = titles[index]; (runner->nxt_title != NULL) && (strcmp(runner->title, runner->nxt_title->title) < 0); runner = runner->nxt_title);
        
        runner->nxt_title = newnode;
        free(runner);
    }
    
    // Placement according to price.
    index = (int)newnode->price / 10;
    
    if (prices[index] == NULL) {
        prices[index] = newnode;
    }
    else {
        runner = (booksT*)malloc(sizeof(booksT*));
        if (runner == NULL) {
            printf("Memory allocation error.\n");
            exit(1);
        }
        
        runner->price = newnode->price;
        runner = prices[index];
        
        for (runner = prices[index]; (runner->nxt_price != NULL) && (runner->price < runner->nxt_price->price); runner = runner->nxt_price);
        
        runner->nxt_price = newnode;
        free(runner);
    }
}

Line 69 seems to be giving me a segmentation fault. Why would that be? Am I trying to insert the nodes incorrectly?

Code:

for (i = 0, pos = 0; (feof(f) == 0); i++)

additionally to the above - you have to read FAQ > Why it's bad to use feof() to control a loop - Cprogramming.com


line 45 gives you a memory leak - you do not need to allocate memory for runner and free it afterwards...

Originally Posted by Xpl0ReRChR

So what should I do instead? I can't think of a different way to find where the new node has to be inserted unless I do that.

just use the pointer to traverse the list. The pointer will "walk" over existing nodes - so no allocation is needed for it.

Why do you think the line below always returns a valid index value?

Code:

index = (int)newnode->price / 10;

I think this would always return a valid index. Note: Use of magic numbers is poor style. http://en.wikipedia.org/wiki/Magic_n...ical_constants

Code:

index = (int)newnode->price % 10;

Tim S.

Originally Posted by Xpl0ReRChR

Prices are of type double, and range from 0 to 100. So I figured, if a number is eg. 9.12, it corresponds to the index of 0 as it is a value less than 10. And since 9 (as an int) / 10 = 0, it would work correctly. I sense it will probably work for any value.

100/10 is 10; which is NOT a valid index.

Tim S.

Code:

        runner = (booksT*)malloc(sizeof(booksT*));
        if (runner == NULL) {
            printf("Memory allocation error.\n");
            exit(1);
        }
         
        runner->price = newnode->price;
        runner = prices[index];
         
        for (runner = prices[index]; (runner->nxt_price != NULL) && (runner->price < runner->nxt_price->price); runner = runner->nxt_price);
         
        runner->nxt_price = newnode;
        free(runner);

You change the location runner is pointing by the time you free it; do you mean to free a location you did not allocate in this code block?

Note: I suggest posting the new code you are having problems with since at least two mistakes were pointed out in past posts.
The main ones are mentioned in anduril462 post number 2.

Tim S.

Originally Posted by Xpl0ReRChR

You're definitely right about this.
But are you sure about the use of '%'? If I'm not mistaken, 100%10=0.

It is better than the program crashing; but, it would not insert then in order. Neither does your code in some cases.
Note: I consider over writing prior data not inserting it right in order.

Oops, relooking at your design; it does seem to be good enough to insert new prices without over writing prior prices.
Your code is hard to follow; so, I am not sure if it does a good job on that or not.

Tim S.

Code:

typedef struct books {
    char title[TITLE_LEN];
    double price;
    struct books *nxt_price;
    struct books *nxt_title;
} booksT;


     // Each line is scanned, then 'insert' is called.     
      for (i = 0, pos = 0; (feof(f) == 0); i++) {
        
        fseek(f, pos, SEEK_CUR);
        fscanf(f, "%s %lf", temp_title, &temp_price);
        insert(titles, prices, temp_title, temp_price);
        pos++;
    }

    //New node is created and inserted.
void insert (booksT *titles[26], booksT *prices[10], char tmp_title[], double tmp_price) {
    booksT *newnode, *curr, *prev;
    int index;
    
    newnode = (booksT*)malloc(sizeof(booksT));
    if (newnode == NULL) {
        printf("Memory allocation error.\n");
        exit(1);
    }
    
    strcpy(newnode->title, tmp_title);
    newnode->price = tmp_price;
    
    // Placement according to title.   
    index = newnode->title[0] - 'A';
    if (titles[index] == NULL) {
        titles[index] = newnode;
    }
    else {
        curr = (booksT*)malloc(sizeof(booksT));
        if (curr == NULL) {
            printf("Memory allocation error.\n");
            exit(1);
        }
        
        prev = (booksT*)malloc(sizeof(booksT));
        if (prev == NULL) {
            printf("Memory allocation error.\n");
            exit(1);
        }
        
        strcpy(curr->title, newnode->title);
        
        for (prev = NULL, curr = titles[index]; (curr->nxt_title != NULL) && (strcmp(curr->title, curr->nxt_title->title) < 0); prev = curr, curr = curr->nxt_title);
        
        curr->nxt_title = newnode;
        free(curr);
    }
    
    // Placement according to price.
    index = (int)newnode->price / 10;
    
    if (prices[index] == NULL) {
        prices[index] = newnode;
    }
    else {
        curr = (booksT*)malloc(sizeof(booksT));
        if (curr == NULL) {
            printf("Memory allocation error.\n");
            exit(1);
        }
        
        prev = (booksT*)malloc(sizeof(booksT));
        if (prev == NULL) {
            printf("Memory allocation error.\n");
            exit(1);
        }
        
        curr->price = newnode->price;
        
        for (prev = NULL, curr = prices[index]; (curr->nxt_price != NULL) && (curr->price < curr->nxt_price->price); prev = curr, curr = curr->nxt_price);
        
        curr->nxt_price = newnode;
        free(curr);
    }
}

I talked to my Programming teacher today and he still insisted on casting malloc so I'm going to go with that. This is the current progress of the code (the relevant part of it).
"curr" (ex-"runner") is supposed to find exactly where the new node is supposed to be placed, and then with the use of "prev" which will point to the node before "curr", the new node must be inserted. I seem to be getting it wrong.