Segmentation. strcmp [C]

Question

I have a file with format: [name][number][amount] number is taken as a string. and im using it in a strcmp. Problem is that i get a segmentation fault. I know that on most cases when strcmp signs segmentation fault it means that one of the parameters is null or cant find its "end" ('\0'). I checked with gdb and i cant say if this is the problem.Take a look:

> (gdb) bt full
> #0  0x08048729 in lookup (hashtable=0x804b008, hashval=27, 
>     number=0x804b740 "6900101001") 
>         list = 0xffffffff
> #1  0x080487ac in add (hashtable=0x804b008, 
>     number=0x804b740 "9900101001", name=0x804b730 "Smithpolow",
> time=6943) 
>         new_elem = 0xffffffff
>         hashval = 27
> #2  0x08048b25 in main (argc=1, argv=0xbffff4b4) 
>         number = 0x804b740 "9900101001"
>         name = 0x804b730 "Smithpolow"
>         time = 6943
>         i = 2

Code:

        typedef struct  HashTable
        {
            int length;
            struct  List *head; 

        } HashTable;

        //(resolving collisions using chaining)
        typedef struct  List
        {
            char *number;
            char *name;
            int time;
            struct List *next;
        } List;

    int primes[]={17,29,51,79,163,331,673,1361,2729,5471,10949,21911,43853,87719,175447,350899};
    *int PrimesIndex=1;* **int PrimesIndex=0;**  **//changed.**


     HashTable *createHashTable(size)
    {
         HashTable *new_table = malloc(sizeof(*new_table)*size);

        if (new_table == NULL)
        {   return NULL;
        }

        int i=0;
        for(i; i<size; i++)
        {   new_table[i].length=0;
            new_table[i].head=NULL;
        }
        return new_table;
    }

    int hash ( HashTable *hashtable,char* number)
    {
        int hashval = 0;
        int i = 0;
        for ( i = 0; i < 10; i++)
        {   hashval = (hashval << 5)|(hashval >> 27);
            hashval += ( int)number[i];
        }

        return hashval % primes[PrimesIndex];
    }

         List *lookup ( HashTable *hashtable,int hashval,char number[10])
        {
         printf("NUMBER:%s\n",number);
          List *list=hashtable[hashval].head;
         for(list; list!=NULL; list=list->next){
           if (strcmp(number,list->number)==0)    
            return list;

         }
         return NULL;
        }


        int add ( HashTable* hashtable,char number[10],char* name,int time)
        {
             List *new_elem;
            int hashval=hash (hashtable,number);

            new_elem=hashtable[hashval].head;
            if(hashtable[hashval].length>0)
            {                   
                  if ((lookup (hashtable,hashval,number))!=NULL) {return 0;}    
            }

            if (!(new_elem=malloc(sizeof(struct  List)))){ return -1;}

            //insert values for the new elem
            new_elem->number=strdup(number);    
            new_elem->name=strdup(name);
            new_elem->time=time;

            hashtable[hashval].head=new_elem;
            new_elem->next=NULL;
            hashtable[hashval].length++;

            /* rehash existing entries if necessary */
            if(hashTableSize(hashtable)>= 2*primes[PrimesIndex])    
            {    
                 hashtable = expand(hashtable);
                 if (hashtable ==NULL){
                   return 0;
                 }

            }

            return 1;
        }

 HashTable* expand( HashTable* h )
{   printf("EXPAND \n");
     HashTable* new;
     List *temp;
    int n;
     List *node,*next;
    PrimesIndex++;
    int new_size= primes[PrimesIndex];      /* double the size,odd length */

    if (!(new=malloc((sizeof(  List*))*new_size))) return NULL;

    for(n=0; n< h->length; ++n) {
        for(node=h[n].head; node; node=next) {
            add (new, node->number, node->name,node->time);
            next=node->next;
            //free(node);
        }
    }
    free(h);
    return new;
}

and the main:

  int main(int argc, char *argv[])  
    {
        char **token;
        FILE *delimitedFile;
        /*Here's an example of tokenizing lines from an actual file*/
        /*Open file for reading ("r"), and take a FILE pointer, 
          which you can use to fetch lines using fgets()*/

        my_hash_table = createHashTable(17);
        if(my_hash_table==NULL)
        {   return 1;
        }

        FILE * File2;
            if ( ( File2=fopen(" File.txt","r")) !=NULL ) 
            { // File.txt format:  [name number time]
                int li = 0;
                char *lin = (char *) malloc(MAX_LINE * sizeof(char));

                while(fgets(lin, MAX_LINE, File2) != NULL)
                {
                    token = my_linetok(lin, " ");
                    if(token != NULL)
                    {
          char* number ;
          char* name;
          int time;
          int i;
                        for(i = 0; token[i] != NULL; i++)
                        {
           name=strdup(token[0]);
           number=strdup(token[1]);
           time=atoi(token[2]);

           if (i==2)
           { int insertDone=0;
                 insertDone =add(my_hash_table,number,name,time);   

           } 
          }
          free(name); 
          free(number);
          free(token);

                    }
                    else 
             {
                        printf("Error reading line %s\n", lin);
                        exit(1);   
                    }
                }

            } 
            else 
            {
                printf("Error opening file \nEXIT!");
         exit(0);
            }

        return 1;
    }

Answer 1

The underlying problem here is that you create a hashtable with 17 buckets:

my_hash_table = createHashTable(17);

But C arrays are 0-based, and PrimesIndex starts out at 1, not 0, so inside add(), the call to hash():

int hashval=hash (hashtable,number);

will return a number between 0 and 28, not a number between 0 and 16. So at some point, an out-of-range value will be assigned to hashval, and one of the subsequent accesses indexed by hashval, e.g.

new_elem=hashtable[hashval].head;

will be reading uninitialised memory, leading ultimately to crazy pointer values like 0xffffffff surfacing later on.

Solution: Change int PrimesIndex = 1; to int PrimesIndex = 0;.

But honestly, I think there could well be other issues that I'm missing. There are:

• Issues with the for loop inside the while loop in main() that I've pointed out in comments;
• The dubious declaration for the number parameter to lookup_on_Clients();
• The fact that sometimes the function is called lookup() and sometimes lookup_on_Clients() (as noticed by Oli);
• And I don't trust that my_linetok() (which you don't show source for) works properly -- at the very least, unless it uses a static buffer, it must be allocating an array of char * in order to hold the pointers to the individual tokens, which is never freed -- a memory leak.

Answer 2

You don't have a room for null terminator in number. You set size of number to be equal to 10 chars, but you have 10 digits in your number and no space for \0.

EDIT:

I looked your updated code. You created hashtable of initial size = 17, but your hasval = 27. But you don't have code to extend the size of hashtable properly.

new_elem=hashtable[hashval].head;
if(hashtable[hashval].length>0) // <-- when hashval is out of array 
                                // hashtable[hashval] can have any value of length and head (not NULL)

Answer 3

You don't actually show the source for add() which presumably calls lookup_on_Clients(), and the backtrace mentions lookup() instead of lookup_on_Clients(), so I can't be sure, but here's my diagnosis:

• The backtrace says list = 0xffffffff -- that's definitely not a valid address, so it's probably the list->name access that is causing the SIGSEGV.
• I'm also bothered by the fact that the number parameter to lookup_on_Clients() is declared as char number[10] and yet gdb shows it contains a 10-digit number -- that suggests that the variable holding the argument for this is declared the same way, meaning that there's no room for a terminating 0 byte. And the fact that you're calling strcmp() on it means that you are treating number as nul-terminated string, so the variable holding the argument that gets passed to lookup_on_Clients() as number (possibly a local variable declared in add()?) should be declared as an array with size at least 11 to avoid crashes. You're safe if add() just passes its own number argument straight through, since that is dynamically allocated to be large enough via strdup() in main(), but I would nevertheless change the declaration on lookup_on_Clients().