Segmentation Fault - aaaaaaaah!

This is a discussion on Segmentation Fault - aaaaaaaah! within the C Programming forums, part of the General Programming Boards category; I'm new to these forums (and pretty new to C), so first I would like to say Hi to everybody. ...

  1. #1
    Registered User
    Join Date
    Sep 2007
    Posts
    3

    Segmentation Fault - aaaaaaaah!

    I'm new to these forums (and pretty new to C), so first I would like to say Hi to everybody.

    I have come here seeking help with a linked list stack implementation. Basically I am trying to implement a program that generates a stack of regular playing cards ie: 52 cards hearts spades etc. It then shuffles these cards and then deals them out into two seperate piles of equal amounts. This is for a school assignment with focus on ADTs (something I am still trying to get my head around :S).

    The code compiles without any errors, however I get a segmentation fault whenever I try to run the executable. I have narrowed down when the fault ccurs and it seems to be caused by the line: strcpy(newCard->suit, suit); which is in the init_card function of deck.c. I understand that segmentation faults occur when the program attempts to write to memory when it does not have permission. My understanding of malloc() is that memory allocated by it remains until freed regardless of which function the program is in.

    I apologise for the amount of code included. If anybody can offer any help at all it would be much appreciated. Thanks.

    Code:
    [main.c]
    #include <stdlib.h>
    #include <stdio.h>
    #include "main.h"
    #include "deck.h"
    #include "stack.h"
    
    
    int main (int argc, char **argv)
    {
       int i;
       // Generate a deck of cards and shuffle
       char *suit[] = {"Diamonds", "Hearts", "Clubs", "Spades"};
       card deck[52];
    
       fill_deck (deck, suit);
       shuffle (deck);
    
       // Initialise stack pointers
       card *p1top, *p2top, *p1bottom, *p2bottom = NULL;
    
       stack_init (&p1top, &p1bottom);  // Initialise player 1 stack
       stack_init (&p2top, &p2bottom);  // Initialise player 2 stack
    
       deal (deck, &p1top, &p2top);     // Deal 26 cards to each player
    
       return (0);
    }
    [/main.c]
    
    [main.h]
    struct card {
       int value;
       char *suit;
    
       struct card *nextCard;
       struct card *prevCard;
    };
    
    typedef struct card card;
    [/main.h]
    
    [stack.c]
    #include <stdlib.h>
    #include <string.h>
    #include <stdio.h>
    #include "main.h"
    #include "deck.h"
    #include "stack.h"
    
    
    void stack_init (card **top, card **bottom)
    {
       *top = NULL;
       *bottom = NULL;
    }
    
    void push (card *new, card **top)
    {
       card *prev = *top;
    
       new->nextCard = NULL;
    
       if (*top == NULL)
       {
          new->prevCard = NULL;
       }
       else
       {
          prev->nextCard = new;
          new->prevCard = prev;
       }
    
       *top = new;
    }
    
    
    void queue (card *hold, card **bottom)
    {
       card *prev = *bottom;
       card *new;
    
       new->prevCard = NULL;
    
       prev->prevCard = new;
       new->nextCard = prev;
    
       *bottom = new;
    }
    
    
    card *pop (card **top)
    {
       return *top;
    }
    [/stack.c]
    
    [stack.h]
    /* Function Prototypes */
    void stack_init (card **top, card **bottom);
    void push (card *new, card **top);
    void queue (card *new, card **bottom);
    card *pop (card **top);
    [/stack.h]
    
    [deck.c]
    #include <stdlib.h>
    #include <stdio.h>
    #include <string.h>
    #include <time.h>
    #include "main.h"
    #include "deck.h"
    #include "stack.h"
    
    
    card *init_card (char *suit, int value)
    {
       card *newCard = (card *) malloc (sizeof(card));
    
       if (newCard == NULL)    // Check for successful malloc()
          return (NULL);
       else
       {
          strcpy(newCard->suit, suit);
          newCard->value = value;
          return (newCard);
       }
    }
    
    void fill_deck (card *deck, char *suit[])
    {
       int i;
    
       for (i = 0; i < 52; i++)
       {
          deck[i].value = (i % 13) + 2;
          deck[i].suit = suit[i / 13];
       }
    }
    
    void shuffle (card *deck)
    {
       srand (time(NULL));
    
       int i, j;
       card temp;
    
       for (i = 0; i < 52; i++)
       {
          j = rand() % 52;
          temp = deck[i];
          deck[i] = deck[j];
          deck[j] = temp;
       }
    }
    
    void deal (card *deck, card **p1top, card **p2top)
    {
       int i;
       card *hold;
    
       for (i = 0; i < 52; i++)
       {
          hold = init_card (deck[i].suit, deck[i].value);
    
          if (i % 2 == 0)
          {
             push (hold, p1top);
          }
          else
          {
             push (hold, p2top);
          }
       }
    }
    [/deck.c]
    
    [deck.h]
    card *init_card (char *suit, int value);
    void fill_deck(card *, char *suit[]);
    void shuffle(card *);
    void deal (card *, card **, card **);
    [/deck.h]

  2. #2
    int x = *((int *) NULL); Cactus_Hugger's Avatar
    Join Date
    Jul 2003
    Location
    Banks of the River Styx
    Posts
    902
    Code:
    strcpy(newCard->suit, suit);
    newCard->suit is never initialized, and because of such points to garbage. You then try to copy a string into that garbage space. You'll need to make card.suit either a char array, or allocate enough space for strcpy().

    Alternatively, why copy a string? Just use a number to identify what suit the card is. (ie, 0 = spades, 1 = heards, 2 = clubs, 3 = diamonds) This saves you from copying strings, and is more space efficient too.

    Edit: Your shuffle() algorithm isn't what I consider a shuffle, really... you're just swapping cards, so why only 52 swaps? Why not 100? or 1000? I usually use the algorithm here... see Wikipedia, specifically the Knuth Shuffle.
    Last edited by Cactus_Hugger; 09-30-2007 at 09:15 PM.
    long time; /* know C? */
    Unprecedented performance: Nothing ever ran this slow before.
    Any sufficiently advanced bug is indistinguishable from a feature.
    Real Programmers confuse Halloween and Christmas, because dec 25 == oct 31.
    The best way to accelerate an IBM is at 9.8 m/s/s.
    recursion (re - cur' - zhun) n. 1. (see recursion)

  3. #3
    Registered User
    Join Date
    Sep 2007
    Posts
    3
    Yea thanks, i think the numbering method may be a better idea. In regards to newCard->suit not being initialised: since I have defined card to be a structure containing the variable suit. I thought that the one line: card *newCard = (card *) malloc (sizeof(card)); would both assign memory space and initialise such a structure. I am probably mistaken. What would I need to add to initialise the variables inside the structure? so the program knows it isnt garbage space.

  4. #4
    Registered User
    Join Date
    Jun 2005
    Posts
    6,646
    Yes, you are mistaken, but it's a common mistake. One of the most common mistaken beliefs that beginners often have is to assume that creating a pointer also implicitly creates an object to be pointed at, and associates the pointer with that object. The solution is to create something to be pointed at, and initialise the pointer so it points at it.

    In your case, that means;
    Code:
      card *newCard = (card *) malloc (sizeof(card));
    
       if (newCard == NULL)    // Check for successful malloc()
          return (NULL);
       else
       {
          newCard->suit = malloc(strlen(suit) + 1);   /*  make newCard->suit point at a suitable block of memory */
          if (newCard->suit != NULL) strcpy(newCard->suit, suit);
          newCard->value = value;
          return (newCard);
       }
    And, yes, later on you will need to remember to free(newCard->suit) before free(NewCard) -- unless you like memory leaks.

    Incidentally, in C you do not need to do a conversion (or cast) of the return value from malloc(). If your compiler is forcing you to do that, it is a C++ compiler.
    Last edited by grumpy; 09-30-2007 at 09:19 PM.

  5. #5
    Registered User
    Join Date
    Sep 2007
    Posts
    3

    Talking Thank you

    Ah yes I see. That explanation really helped. Thanks a lot for the help guys, you've saved me many hours staring frustratedly at my code.

  6. #6
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Alternatively, if you want to continue using strings, you could just as easily just assign the address of the string in your "suits" list.
    Code:
    card *init_card (char *suit, int value)
    {
       card *newCard = (card *) malloc (sizeof(card));
    
       if (newCard == NULL)    // Check for successful malloc()
          return (NULL);
       else
       {
          newCard->suit = suit;
          newCard->value = value;
          return (newCard);
       }
    }
    The above wors on the premise that you are not going modify the string for suit - which is somehting I find unlikely - a suit is either Hearts, Clubs, Diamonds or Spades, and once the card is a certain suit, it's not going to change to "Blades" or some such... [One hopes!]

    Of course, using a numeric (enum) would be a better suggestion, since a numeric comparison is simpler than the string compare [although, with some clever use of the array suits as a global variable, you could of course just compare the strings directly - but you can't make "rank" comparisons, such as "if (onecard->suit > secondcard->suit) ... ", which is certainly the case when using a numeric value.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  7. #7
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,834
    > card *newCard = (card *) malloc (sizeof(card));
    *shudder*
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Re: Segmentation fault
    By turkish_van in forum C Programming
    Replies: 8
    Last Post: 01-20-2007, 05:50 PM
  2. Segmentation fault
    By bennyandthejets in forum C++ Programming
    Replies: 7
    Last Post: 09-07-2005, 06:04 PM
  3. Segmentation fault
    By NoUse in forum C Programming
    Replies: 4
    Last Post: 03-26-2005, 03:29 PM
  4. Locating A Segmentation Fault
    By Stack Overflow in forum C Programming
    Replies: 12
    Last Post: 12-14-2004, 01:33 PM
  5. Segmentation fault...
    By alvifarooq in forum C++ Programming
    Replies: 14
    Last Post: 09-26-2004, 01:53 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21