Segmentation fault 11 in following code. How to avoid overflow?

Question

void main(int argc, char* argv[]) {

    char* hostname = (char*)malloc(sizeof(char)*1024);
    hostname = getClientHostName("122.205.26.34");
    printf("%s\n", hostname);
    free(hostname);
}

char* getClientHostName(char* client_ip) {

    char hostnames[5][2];
    hostnames[0][0] = "122.205.26.34";
    hostnames[0][1] = "aaaaa";
    hostnames[1][0] = "120.205.36.30";
    hostnames[1][1] = "bbbbb";
    hostnames[2][0] = "120.205.16.36";
    hostnames[2][1] = "ccccc";
    hostnames[3][0] = "149.205.36.46";
    hostnames[3][1] = "dddddd";
    hostnames[4][0] = "169.205.36.33";
    hostnames[4][1] = "eeeeee";
    for(int i = 0; i<5; i++) {
        if(!strcmp(hostnames[i][0], client_ip))
            return (char*)hostnames[i][1];
    }
    return NULL;
}

Beginner in C.

I am not sure if there would be a better way to implement what I am trying to implement. The code is self-explanatory. Is there any way that I can predefine the size of hostname, using some general size of IP addresses, to avoid seg fault? Is there a even better way where I don't have to hardcode the size?

Your code should not compile cleanly. Compile it with `gcc -Wall -Wextra -g` with [GCC](http://gcc.gnu.org/) — Basile Starynkevitch, Oct 07 '17 at 08:26

score 2 · Answer 1 · answered Oct 07 '17 at 08:30

After fixing the compiler errors and warnings you get:

const char* getClientHostName(const char* client_ip) {

    const char * hostnames[5][2];
    hostnames[0][0] = "122.205.26.34";
    hostnames[0][1] = "aaaaa";
    hostnames[1][0] = "120.205.36.30";
    hostnames[1][1] = "bbbbb";
    hostnames[2][0] = "120.205.16.36";
    hostnames[2][1] = "ccccc";
    hostnames[3][0] = "149.205.36.46";
    hostnames[3][1] = "dddddd";
    hostnames[4][0] = "169.205.36.33";
    hostnames[4][1] = "eeeeee";
    for(int i = 0; i<5; i++) {
        if(!strcmp(hostnames[i][0], client_ip))
            return hostnames[i][1];
    }
    return NULL;
}

int main(int argc, char* argv[]) {
    const char * hostname = getClientHostName("128.205.36.34");
    printf("%s\n", hostname);
}

Basile Starynkevitch · Answer 2 · 2017-10-07T10:37:15.433

Is there a even better way where I don't have to hardcode the size?

Take the habit to compile with all warnings and debug info: gcc -Wall -Wextra -g with GCC. Improve the code to get no warnings at all.

If you want to get genuine IP addresses, this is operating system specific (since standard C11 don't know about IP addresses; check by reading n1570). On Linux you would use name service routines such as getaddrinfo(3) & getnameinfo(3) or the obsolete gethostbyname(3).

If this is just an exercise without actual relationship to TCP/IP sockets (see tcp(7), ip(7), socket(7)) you could store the table in some global array:

 struct myipentry_st {
   const char* myip_hostname;
   const char* myip_address;
 };

then define a global array containing them, with the convention of terminating it by some {NULL, NULL} entry:

const struct myipentry_st mytable[] = {
    {"aaaaa", "122.205.26.34"},
    {"bbbb", "120.205.36.30"},
    /// etc
    {NULL, NULL} // end marker
};

You'll better have a global or static variable (not an automatic one sitting on the call stack) because you don't want to fill it on every call to your getClientHostName.

Then your lookup routine (inefficient, since in linear time) would be:

const char* getClientHostName(char* client_ip) {
   for (const struct myipentry_st* ent = mytable;
        ent->myip_hostname != NULL;
        ent++)
   // the if below is the only statement of the body of `for` loop
     if (!strcmp(ent->myip_address, client_ip))
         return ent->myip_hostname;
   // this happens after the `for` when nothing was found
   return NULL;
}

You could even declare that table as a heap allocated pointer:

const struct myipentry_st**mytable;

then use calloc to allocate it and read its data from some text file.

Read the documentation of every standard or external function that you are using. Don't forget to check against failure (e.g. of calloc, like here). Avoid memory leaks by appropriate calls to free. Use the debugger gdb and valgrind. Beware of undefined behavior.

In the real world, you would have perhaps thousands of entries and you would perform the lookup many times (perhaps millions of times, e.g. once per every HTTP request in a web server or client). Then choose a better data structure (hash table or red-black tree perhaps). Read some Introduction to Algorithms.

Thanks for the inputs. But the thing is, I definitely could have implemented much better ways, and I can do that. But not in C. This is the first time I am coding in C. The very first time. Like, FIRST TIME. And I had a week to do this assignment. So I didn't get enough time to learn C. I am more comfortable with Java and to some extent Python, but C is very different. So right now I am just trying to make it work somehow. — Manic, Oct 07 '17 at 15:46

score 1 · Accepted Answer · answered Oct 07 '17 at 08:27

Add * to type definition char * hostnames[5][2]. This must be array of pointers, not simple chars. Another necessary change is strcpy instead of = in strcpy( hostname, getClientHostName("122.205.26.34") );.

PS: Always try to compile with 0 compiler warnings, not only 0 errors!

Segmentation fault 11 in following code. How to avoid overflow?

3 Answers3