Thread: Segmentation fault!

  1. #1
    Registered User
    Join Date
    Nov 2011
    Posts
    69

    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?
    Attached Images Attached Images Segmentation fault!-hw4eng-jpg 
    Last edited by Xpl0ReRChR; 01-10-2012 at 12:37 PM.

  2. #2
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,907
    Code:
    newnode = (booksT*)malloc(sizeof(booksT*));
    First, read the FAQ on why FAQ > Casting malloc - Cprogramming.com is not such a good idea.

    Then, realize that, with sizeof(booksT*), you're only allocating enough space for a pointer to a booksT object, not an actual booksT object. You do this on all of your malloc calls. The preferred idiom for malloc'ing an object is:
    Code:
    newnode = malloc(sizeof(*newnode));
    That way you always allocate enough space for an actual newnode object, and if the type changes for any reason, you only need to change the declaration, and not the malloc statement as well.

  3. #3
    Hurry Slowly vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,793
    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...
    All problems in computer science can be solved by another level of indirection,
    except for the problem of too many layers of indirection.
    David J. Wheeler

  4. #4
    Registered User
    Join Date
    Nov 2011
    Posts
    69
    Quote Originally Posted by vart View Post
    line 45 gives you a memory leak - you do not need to allocate memory for runner and free it afterwards...
    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.

    Thanks to both of you by the way; I read both FAQ's and they're very helpful, though I don't see why we've been instructed to typecast malloc.

  5. #5
    Hurry Slowly vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,793
    Quote Originally Posted by Xpl0ReRChR View Post
    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.
    All problems in computer science can be solved by another level of indirection,
    except for the problem of too many layers of indirection.
    David J. Wheeler

  6. #6
    Registered User
    Join Date
    Nov 2011
    Posts
    69
    Yes, but for each "step" it takes, a comparison will have to be made. Therefore, I'm guessing the runner will have to contain both a temporary title and a temporary price - meaning it will have to be malloc'd.

    Can someone help me as to why Line 69 seems to get me a segmentation fault? Also, is the logic of the for loops correct? It looks like I will need to know the runner's previous node too, in order to place a new node between two existing nodes.

  7. #7
    Registered User
    Join Date
    May 2009
    Posts
    3,824
    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.
    Last edited by stahta01; 01-11-2012 at 10:38 AM.
    "...a computer is a stupid machine with the ability to do incredibly smart things, while computer programmers are smart people with the ability to do incredibly stupid things. They are,in short, a perfect match.." Bill Bryson

  8. #8
    Registered User
    Join Date
    Nov 2011
    Posts
    69
    Quote Originally Posted by stahta01 View Post
    Why do you think the line below always returns a valid index value?
    Code:
     index = (int)newnode->price / 10;
    Tim S.
    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.

  9. #9
    Registered User
    Join Date
    May 2009
    Posts
    3,824
    Quote Originally Posted by Xpl0ReRChR View Post
    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.
    "...a computer is a stupid machine with the ability to do incredibly smart things, while computer programmers are smart people with the ability to do incredibly stupid things. They are,in short, a perfect match.." Bill Bryson

  10. #10
    Registered User
    Join Date
    Nov 2011
    Posts
    69
    You're definitely right about this.
    But are you sure about the use of '%'? If I'm not mistaken, 100%10=0.

  11. #11
    Registered User
    Join Date
    May 2009
    Posts
    3,824
    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.
    Last edited by stahta01; 01-11-2012 at 10:58 AM.
    "...a computer is a stupid machine with the ability to do incredibly smart things, while computer programmers are smart people with the ability to do incredibly stupid things. They are,in short, a perfect match.." Bill Bryson

  12. #12
    Registered User
    Join Date
    May 2009
    Posts
    3,824
    Quote Originally Posted by Xpl0ReRChR View Post
    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.
    Last edited by stahta01; 01-11-2012 at 11:04 AM.
    "...a computer is a stupid machine with the ability to do incredibly smart things, while computer programmers are smart people with the ability to do incredibly stupid things. They are,in short, a perfect match.." Bill Bryson

  13. #13
    Registered User
    Join Date
    Nov 2011
    Posts
    69
    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.
    Last edited by Xpl0ReRChR; 01-11-2012 at 11:07 AM.

  14. #14
    Registered User
    Join Date
    Nov 2011
    Posts
    69
    Code:
    for (prev = NULL, curr = titles[index]; (curr->nxt_title != NULL) && (strcmp(curr->title, curr->nxt_title->title) < 0); prev = curr, curr = curr->nxt_title);
    This causes a segmentation fault. Would someone have an idea as to why?

  15. #15
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,907
    You initialize curr with titles[index], but don't check if curr is NULL before doing curr->nxt_title. Also, you're comparing titles for two nodes already in the list, each node with the node after it. I would assume, if your insert was correct, they would already be in order. I would think you want to compare your new node with each node in the list.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Segmentation fault
    By sirsmilealot in forum C Programming
    Replies: 12
    Last Post: 02-10-2010, 01:26 PM
  2. segmentation fault??
    By snappleapple in forum C Programming
    Replies: 9
    Last Post: 04-27-2007, 11:56 PM
  3. Segmentation Fault
    By warfang in forum C++ Programming
    Replies: 9
    Last Post: 04-23-2007, 01:42 AM
  4. segmentation fault when using gcc
    By stodd04 in forum C Programming
    Replies: 6
    Last Post: 02-14-2005, 07:34 PM
  5. segmentation fault and memory fault
    By Unregistered in forum C Programming
    Replies: 12
    Last Post: 04-02-2002, 11:09 PM