I'm new to cpp, trying to write an echo server. I don't know where the segmentation fault is and I have been trying to find it for an hour.

When I run this server and client, the server crashed after client sent message. I'm confused, where the problem is.

There is a commenting in the code, where is the vscode stoped and pointed out with a yellow tag.

Is the msgBuffer went wrong or something ?

Anyone can help me?

#define SERV_PORT 1234

#define MAXLINE 1024

int sockfd, clientfd;

void stopServer(int p) {
    close(clientfd);
    close(sockfd);
    std::cout << "Server offline" << std::endl;
    exit(0);
}

void reconnect(int p) {
    close(clientfd);
    std::cout << "Client close. " << std::endl;
    std::cout << "Server listening..." << std::endl;
    clientfd = accept(sockfd, NULL, NULL);
}

class Server {
   public:
   public:
    int doBind(int argc, char const** argv) {
        memset(&servaddr, 0, sizeof(servaddr));
        this->servaddr.sin_family = AF_INET;
        if (argc == 3) {
            inet_pton(AF_INET, argv[1], &this->servaddr.sin_addr);
            this->servaddr.sin_port = htons(atoi(argv[2]));
        } else {
            
            this->servaddr.sin_addr.s_addr = htonl(INADDR_ANY);
            this->servaddr.sin_port = htons(SERV_PORT);
        }

        if (-1 == bind(sockfd, (sockaddr*)&this->servaddr, sizeof(servaddr))) {
            std::cout << "Bind error" << strerror(errno) << std::endl;
            return -1;
        }
        return 0;
    }

   private:
    sockaddr_in servaddr;
};

class Client {
   public:
   public:
    void doListen(int q) {
        listen(sockfd, q);
        std::cout << "Listening for request..." << std::endl;
    }

    void doAccept() { clientfd = accept(sockfd, NULL, NULL); }

>     void msgProcess(char* msgBuffer) {
>         while (1) {
>             signal(SIGINT, stopServer);
>     
>             signal(SIGPIPE, reconnect);
>             
>             memset(&msgBuffer, 0, sizeof(msgBuffer));
>     
>             recv(clientfd, &msgBuffer, MAXLINE, 0);
>     
>             // vscode stoped, and yellow tag pointed here 
>             std::cout << msgBuffer << std::endl;
>     
>             send(clientfd, &msgBuffer, sizeof(msgBuffer), 0);
>         }
>     }

   private:
    sockaddr_in clientaddr;
};

int main(int argc, char const** argv) {
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    char msgBuffer[MAXLINE];

    Server server;
    Client client;

    if (sockfd == -1) {
        std::cout << "Create socket error " << strerror(errno) << std::endl;
    }

    server.doBind(argc, argv);

    client.doListen(4);

    client.doAccept();

    client.msgProcess(msgBuffer);

    return 0;
}

CodePudding user response：

The statement

memset(&msgBuffer, 0, sizeof(msgBuffer));

have two problems:

1. &msgBuffer is a pointer to the variable msgBuffer, it's the wrong pointer to pass if you want to clear the memory where msgBuffer is pointing.

2. sizeof(msgBuffer) is the size of the pointer itself, not whatever array or other data it might point to. You need to pass the size of the array as an argument to the msgProcess function.

The first problem you also have with the call to recv.

---

You also have plenty of other problems. Like the global variables, setting the signal handlers in a loop inside a function, and possible more.

CodePudding user response：

         memset(&msgBuffer, 0, sizeof(msgBuffer));

The above line sets the local pointer msgBuffer to all 0, essentially a nullptr.

         recv(clientfd, &msgBuffer, MAXLINE, 0);

Then this tries to read data into the local pointer variable (not where it points to, but the variable itself). The problem is, pointer size is probably 8 bytes, while MAXLINE as well as your message is probably longer, resulting in buffer overflow.

What you need to do is this:

         // zero memory pointed to by msgBuffer, assume it is this size
         memset(msgBuffer, 0, MAXLINE); 

         // receive into memory pointed to by msgBuffer
         recv(clientfd, msgBuffer, MAXLINE, 0);

Additionally, you must check return value of recv both to detect errors and to know how many bytes were actually received! Store it into a variable so you can examine it for both. After you do this, the above memset is kind of redundant, you don't need to care what the "unused" bytes are, but zeroing out the buffer as matter of principle is ok as a means to have more robust code.

CodePudding user response：

Note that msgBuffer is not a null terminated char array, so if you receive 1024 bytes you will run into segfault even after you zero the array correctly because cout is expecting a null at the end.

a better option is to use write std::cout.write(msgBuffer,MAXLINE)