C language, where is the segmentation fault?

Question

here is the function where im getting the segmentation fault

void searchcity()
{
    struct city *ptr=citylist;
    printf("Which city would you like me to search?: ");
    scanf("%s",searchedcity);
    //  printf("%s",searchedcity);
    while(ptr)
    {
        if(!strcmp(searchedcity,ptr->name))
            printf("name= %s, statecode = %s,population = %s,region = %s,zipcode =     %s\n",ptr->name,ptr->statecode,ptr->population,ptr->region,ptr->zipcode);
        else
            printf("sorry, couldnt find that city");
        ptr=ptr->next;
    }   
}

not sure what can be causing this to happen.

There could be many reasons why, but without more code it is hard to tell — Shafik Yaghmour, Apr 15 '13 at 01:20
is there anything that causes it within the function though? — Alex, Apr 15 '13 at 01:21
@alex We need to see more code to figure this out without guess work. — Shafik Yaghmour, Apr 15 '13 at 01:27
Have you poked around this with a debugger to see if any pointers involved are `NULL` but shouldn't be? — millimoose, Apr 15 '13 at 01:33

score 0 · Answer 1 · edited May 23 '17 at 10:28

Based on that code ^(a), here's what you need to check at a bare minimum:

That searchedcity has enough space for the input ^(b).
That all the strings held in the citylist linked list are properly constructed (null-terminated).
That all those fields in the structure are actually character arrays (or equivalent pointers) rather than integers (population, for example).
That the list itself is properly built (no dangling or invalid pointers).

You do have one other problem, though nothing to do with the segfault.

Your code will print "sorry, couldnt find that city" for every node in the list that doesn't match your city so, if you have New York, Moscow and London, and you look for London, you'll get that message printed twice before it finds it.

A better solution (one of many variants) would be something like:

struct city *ptr = citylist;
while (ptr != NULL)
  if (strcmp (searchedcity, ptr->name) == 0)
    break;

if (ptr == NULL)
  printf ("Sorry, couldnt find that city.\n");
else
  printf ("%s, state = %s, pop = %s,r egion = %s, zip = %s\n",
    ptr->name, ptr->statecode, ptr->population, ptr->region, ptr->zipcode);

That way, the loop is responsible for either finding the correct pointer or setting it to NULL. After the loop is the correct time to decide what should be printed.

^(a) That code itself seems okay other than the dangerous scanf but it does depend on quite a lot of other stuff not shown.

^(b) In fact, scanf with an unbounded %s is a serious hole in your code that can easily lead to buffer overflow. See here for details and solution. In any case, scanf("%s") is not a good way to scan strings with spaces in them since something like Los Angeles would end up as Los :-)

score -1 · Answer 2 · edited Feb 11 '14 at 15:52

Code below works, I did minor changes you can walk through it. Few small mistakes you had was your ptr->next was never executed due to missing bracket. The rest I wrote in the code.

Thanks hope we helped.

#include <stdio.h>
struct city {
    char name[100];
    char  statecode[100];
    char  population[100];
    char region[100];
    char zipcode[100];
    struct city* next;
};

int main() // this is your searchcity function
{
    char searchedcity[100]; // make sure you initialize this. YOu haven't done it in the code you gave us.
        // Assume citylist is in your main function or initialized as a global var
        // I initialized it here for simplicity
    struct city* citylist = (struct city*) malloc(sizeof( struct city));
    strcpy(citylist->statecode,"statecode");
    strcpy(citylist->population,"population");
    strcpy(citylist->region,"region");
    strcpy(citylist->zipcode,"zipcode");
    citylist->next = NULL;
//end of citylist
    struct city *ptr = citylist;
    printf("Which city would you like me to search?: ");
    scanf("%s",searchedcity);
//  printf("%s",searchedcity);
    while(ptr)  {
        printf("while \n");
        if(!strcmp(searchedcity,ptr->name)){
               printf("name= %s, statecode = %s,population = %s,region = %s,zipcode =     %s\n",ptr->name,ptr->statecode,ptr->population,ptr->region,ptr->zipcode);
        }else{
                printf("sorry, couldnt find that city");
                ptr=ptr->next;
        }
    }
    return 0;
}

Please explain the contention that `ptr->next` wasn't executed in the original code. Moving it inside the braces has no effect on the loop since it wouldn't be _in_ the loop if it were NULL. — paxdiablo, Apr 15 '13 at 04:20
@paxdiablo Oh, it has an effect alright ... it produces an infinite loop when the city is found. The loop is wrong, but that sure isn't the fix. — Jim Balter, Apr 15 '13 at 04:26

C language, where is the segmentation fault?

2 Answers2