C socket programming: Invalid argument error on connect()

Question

I'm writing a client side as part of a TCP client server program.

My code reaches the connect part and throws an Invalid argument error, I have gone through the code several times and I couldn't find the problem.

The code receives 3 arguments, first one is an IP address or a hostname, second one is port and the third is the maximum length of the message to be sent.

My code uses getaddrinfo in order to convert the ip address or hostname, creates the needed variables, starts a connection, read from file, send data and receive data.

I run the code with:

gcc -std=gnu99 -O3 -Wall -o pcc_client pcc_client.c
./pcc_client 127.0.0.1 2233 4

The output is:

sockaddr_in initialized
Error starting connection : Invalid argument

#include <stdlib.h>
#include <stdio.h>
#include <stdio.h>
#include <unistd.h>
#include <assert.h>
#include <errno.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <netdb.h>
#include <dirent.h>

#define FILE_ADDR   "/dev/urandom"

int main(int argc, char *argv[]) {
    if (argc != 4) {
        printf("should receive 3 arguments Received %d args\n", argc);
        exit(1);
    }

    //Get command line arguments
    unsigned int port = atoi(argv[2]);
    int length = atoi(argv[3]); //Number of bytes to read
    char* buffer = malloc(length * sizeof(char)); //Buffer to hold data read from file
    char* recvBuf = malloc(10 * sizeof(char)); // Buffer to hold response from server

    struct addrinfo hints, *servinfo, *p;
    struct sockaddr_in *serv_addr;
    int rv;
    char ip[100];

    memset(&hints, 0, sizeof hints);
    hints.ai_family = AF_INET;
    hints.ai_socktype = SOCK_STREAM;

    if ((rv = getaddrinfo(argv[1], argv[2], &hints, &servinfo)) != 0) {
        perror("getaddrinfo error\n");
        return 1;
    }
    for (p = servinfo; p != NULL; p = p->ai_next) {
        serv_addr = (struct sockaddr_in *) p->ai_addr;
        strcpy(ip, inet_ntoa(serv_addr->sin_addr));
    }
    //   inet_aton(ip, &h.sin_addr);
    freeaddrinfo(servinfo);

    //Initialize socket
    int sockfd;
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if (sockfd < 0) //Error creating socket
            {
        perror("Error creating socket \n");
        exit(1);
    }

    printf("socket created\n");

    //Initialize sockaddr_in structure
    memset((void*)serv_addr, 0,(size_t) sizeof(*serv_addr));
    serv_addr->sin_family = AF_INET;
    serv_addr->sin_port = htons(port);
    serv_addr->sin_addr.s_addr = inet_addr("127.0.0.1"); //change?


    //Initialize connection
    if (connect(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0) { //Error connecting
        perror("Error starting connection \n");
        exit(1);
    }
    printf("connect succesful\n");

    exit(0);

}

Indite

When creating a Minimal, Complete, and Verifiable Example, it's kind of important to remove irrelevant parts. Like the whole getaddrinfo call or most of the code after the connect call. Those parts are irrelevant for the problem at hand. – Ricardoricca

Also, present the actual output of the program -- in this case, the output written to stderr by perror(). A summary or paraphrase can be misleading. – Dabbs

Also, if you're using command line arguments, you need to show the command line that you're using to run the program. But even better is to skip the command line altogether, and hard code the inputs, e.g. unsigned int port = 1234; instead of unsigned int port = atoi(argv[2]); – Fruin

... and having made such simplifications, be sure that resulting program still reproduces the error. – Dabbs

@Someprogrammerdude I have edited the code as requested, I wouldn't remove the getaddrinfo part because I suspect it might be the probem. – Indite

Answer 1

You are using serv_addr all wrong.

You have declared serv_addr as a sockaddr_in* pointer. After getaddrinfo() exits successfully, you are looping through the output list, assigning serv_addr to point at every ai_addr in the list, and then you free the list, leaving serv_addr pointing at invalid memory. You then trash memory when you try to populate serv_addr with data. And then you end up not even passing a valid pointer to a sockaddr_in to connect() at all, you are actually passing a pointer to a pointer to a sockaddr_in, which is why it complains about an "invalid argument".

In fact, you are going about this situation all wrong in general. When using getaddrinfo(), since it returns a linked list of potentially multiple socket addresses, you need to loop through the list attempting to connect() to every address until one of them is successful. This is especially important if you ever want to upgrade the code to support both IPv4 and IPv6 (by setting hints.ai_family = AF_UNSPEC;).

Try something more like this instead:

int main(int argc, char *argv[])
{
    if (argc != 4)
    {
        printf("should receive 3 arguments Received %d args\n", argc);
        exit(1);
    }

    struct addrinfo hints, *servinfo, *p;
    int sockfd = -1;

    memset(&hints, 0, sizeof hints);
    hints.ai_family = AF_INET; // or AF_UNSPEC
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;

    int rv = getaddrinfo(argv[1], argv[2], &hints, &servinfo);
    if (rv != 0)
    {
        perror("getaddrinfo error\n");
        return 1;
    }

    for (p = servinfo; p != NULL; p = p->ai_next) {
        //Initialize socket
        sockfd = socket(p->ai_family, p->ai_socktype, p->ai_protocol);
        if (sockfd < 0) continue;
        //Initialize connection
        rv = connect(sockfd, p->ai_addr, (socklen_t) p->ai_addrlen);
        if (rv == 0) break;
        close(sockfd);
        sockfd = -1;
    }

    freeaddrinfo(servinfo);

    if (sockfd < 0) //Error creating/connecting socket
    {
        perror("Error creating/connecting socket \n");
        exit(1);
    }

    printf("connect successful\n");

    ...

    close(sockfd);
    exit(0);
}

Redo

Answer 2

You define serv_addr

struct sockaddr_in *serv_addr;

Then you use it

memset((void*)serv_addr, 0,(size_t) sizeof(*serv_addr));
serv_addr->sin_family = AF_INET;
serv_addr->sin_port = htons(port);
serv_addr->sin_addr.s_addr = inet_addr("127.0.0.1"); //change?

But nowhere in between those two places in the code do you initialize the pointer! That means serv_addr is uninitialized and its value is indeterminate and will point to some seemingly random location. Dereferencing the pointer will lead to undefined behavior.

The simple and natural and de facto standard solution is to make serv_addr not a pointer, but a structure object:

struct sockaddr_in serv_addr;

Then when you need a pointer you use the address-of operator &.

---

The issue above is further complicated by you actually using the & operator when calling connect. With serv_addr being a pointer, then &serv_addr is a pointer to the pointer. It will be of type struct sockaddr_in **. It is this issue, with the pointer to the pointer, that leads to the error message, since the pointer you send in is not a pointer to a sockaddr_in structure object.

By using a structure object as shown above will solve this problem as well.

Ricardoricca