How is this for a "Hello World" of socket programming?

Question

I was trying out the Beej's guide to socket Programming and wrote this. Is there anything that I am obviously doing wrong? Any potential for buffer overflows? Segmentation faults? Any possible errors that I am not checking for?

Or anything that I am overdoing?

Thanks a lot.

/*
 * sk2.c
 *
 *  Created on: May 26, 2013
 *      Author: bharath
 */


#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <string.h>
#include <regex.h>
#define BUFSIZE 1025

int main(int argC, char *argv[]){
    struct addrinfo hints, *results, *p;
    int status, s,con1, rret;
    char ipstr[INET6_ADDRSTRLEN];
    printf("Bharath's Socket Program\n");
    memset(&hints,0,sizeof hints);
    hints.ai_family=AF_UNSPEC;
    hints.ai_socktype=SOCK_STREAM;
    if (argC<2){
        fprintf(stderr,"No URI given.\n");
        return -1;
    }
    regex_t fqdnParser, resourceParser;
    regmatch_t m;
    char *fqdn, *resource;

    if (!regcomp(&fqdnParser,"^[a-zA-Z0-9.]*",0)){
        char *buff = argv[1];
        if (!regexec(&fqdnParser, argv[1], 1, &m, 0)){
            fqdn=(char*)malloc(m.rm_eo-m.rm_so+1);
            snprintf (fqdn,m.rm_eo-m.rm_so+1,"%.*s \n", m.rm_eo, buff+m.rm_so);
            *(fqdn+m.rm_eo-m.rm_so)=0;
        }else{
            fprintf(stderr,"Bad URI FQDN.\n");
            return -1;
        }
        if (!regcomp(&resourceParser,"/.*$",0)){
            if (!regexec(&resourceParser, argv[1], 1, &m, 0)){
                resource=(char*)malloc(m.rm_eo-m.rm_so+1);
                snprintf (resource,m.rm_eo-m.rm_so+1,"%.*s \n", m.rm_eo, buff+m.rm_so);
                *(resource+m.rm_eo-m.rm_so)=0;
            }
            else{
                resource="/";
            }
            printf("Parsed: [ %s ][ %s ]\n",fqdn,resource);
        }
    }else{
        fprintf(stderr,"RegEx not compiled.\n");
    }
    regfree(&fqdnParser);
    regfree(&resourceParser);
    if ((status=getaddrinfo(fqdn,"http",&hints,&results))!=0){
        fprintf(stderr,"Error: %s\n",gai_strerror(status));
        return -2;
    }
    for(p=results; p!=NULL; p=p->ai_next){
        char *ipver;
        void *addr;
        //ipver=malloc(5);
        if (p->ai_family==AF_INET){
            ipver="IPv4";
            struct sockaddr_in *ipv4=(struct sockaddr_in*)p->ai_addr;
            addr=&(ipv4->sin_addr);
        }else if(p->ai_family==AF_INET6){
            ipver="IPv6";
            struct sockaddr_in6 *ipv6=(struct sockaddr_in6*)p->ai_addr;
            addr=&(ipv6->sin6_addr);
        }
        inet_ntop(p->ai_family,addr,ipstr,sizeof ipstr);
        printf("%s %s\n",ipver, ipstr);
        if ((s=socket(p->ai_family,p->ai_socktype,p->ai_protocol))!=-1){
            if (connect(s,p->ai_addr,p->ai_addrlen) == -1){
                fprintf(stderr,"Error Connecting.");
            }else{
                char *req;
                req=(char*)malloc(25+strlen(resource)+strlen(fqdn));
                strcpy(req,"GET ");
                strcat(req,resource);
                strcat(req," HTTP/1.0\nHost: ");
                strcat(req,fqdn);
                strcat(req,"\n\n");
                //printf("%d | %s",strlen(resource),req);

                void *buf;
                int len,b;
                long int bc=0;
                buf=malloc(BUFSIZE);
                *(char*)(buf+BUFSIZE)=0;
                len=strlen(req);
                b=send(s,req,len,0);
                printf("%d bytes sent\n--------------\n",b);
                printf("%s\n-----------------\n",req);
                b=0;
                do{
                    if(b<BUFSIZE-1){
                        *(char*)(buf+b)=0;
                    }
                    printf("%s",(char*)buf);
                    bc+=b;
                }while ((b=recv(s, buf, BUFSIZE-1, 0))>0);
                close(s);
                printf("\n--------------\n%ld bytes received.",bc);
                free(buf);
                break;
            }
        }else{
            fprintf(stderr,"Socket not created: %d\n",s);
            return -3;
        }
    }
    return 0;
}

Answer 1

I don't know much about sockets, but here are some general C programming comments:

Bugs

• You never free() the malloc'd memory of resource. And if you did, which you should, then resource="/"; would crash the program when you attempted to free() that memory.

Possible bugs

• char ipstr[INET6_ADDRSTRLEN]; To declare an array with the size of a "string length" is fishy. Are you sure that this allocates enough room for a string null termination character?

• (char*)malloc There is never a reason to cast the result of malloc in C. It is even potentially harmful in C90. You can find countless topics about this on Stack Overflow.

Picky details and coding style

• argC is most often spelled with lower case, argc.

• It is always best to declare every variable on a line on its own, to make the code more readable and to avoid bugs like int* ptr1, ptr2; // ptr2 is not a pointer as intended.

• memset(&hints,0,sizeof hints); etc should be replaced with

  struct addrinfo hints = { .ai_family=AF_UNSPEC; .ai_socktype=SOCK_STREAM; };

  This makes the code easier to read and possibly slightly faster as well.

• return -1 should ideally be replaced with EXIT_FAILURE from stdlib.h. Similarly, return 0 should ideally be replaced with EXIT_SUCCESS.
• Your code could do with a lot more empty rows, to separate parts of the program from each other. Or even better, split up that huge main() into separate functions. I think that would improve the program a lot.
• Use verbose variable naming. Names like s, m, a, b, c make the program hard to read and understand.
• Avoid assignment inside if statements, it is hard to read and there's a slight potential for bugs in case compiler warnings aren't properly enabled (= when intending ==).
• malloc(25+ .... The magic number 25 could be replaced with a macro such as #define CONST_STRLEN (sizeof("GET ")-1 + sizeof(" HTTP/1.0\nHost: ")-1 + ... + 1), so that one actually understands what's going on.

Answer 2

In other C like languages, it is customary to use more whitespace for readability, but maybe C is an exception; I will refrain from editing your example. I like to put spaces around infix operators and after commas, and use blank lines before function definitions.

I think

*(char*)(buf+BUFSIZE)=0;

deserves a comment, but then again, my C is rusty.