TCP Socket Wrapper

Question

I'm trying to build a simple server software for training purpose, most likely a IRC server, but I'm not there yet.

I'm currently implementing a TCP socket class, to ease the use of the C socket API. It uses the addrinfo struct and getaddrinfo() (which add some constraints).

TcpSocket.hpp

#ifndef TCPSOCKET_H
#define TCPSOCKET_H
#include <netdb.h>
#include <string>
#include <memory>
namespace Socket {
 /**
 * TcpSocket class implementation to facilitate the use of sockets
 */
 class TcpSocket
 {
 public:
 TcpSocket();
 TcpSocket(int family, int flags);
 TcpSocket(int socket, addrinfo info, bool connected, bool bound);
 virtual ~TcpSocket();
 //Avoiding copy
 TcpSocket(const TcpSocket &socket) = delete;
 TcpSocket &operator=(const TcpSocket &socket) = delete;
 void bind(int port);
 void connect(std::string adress, int port);
 void listen(int maxQueue);
 std::shared_ptr<TcpSocket> accept();
 void send(const char *data, unsigned int length, int flags);
 /**
 * Receive data (blocking)
 *@return true if socket is still open, false otherwise
 */
 bool receive(char* msg, int len, int flags);
 void close();
 private:
 void setInfo(int port);
 void setInfo(std::string adress, int port);
 void openSocket(addrinfo *info);
 addrinfo * mInfo;
 int mSock = -1;
 bool mSockCreated = false;
 bool mBound = false;
 bool mConnected = false;
 bool mClosed = false;
 };
}
#endif

TcpSocket.cpp (Please don't mind the DEBUG function, this is going away)

#include "TcpSocket.hpp"
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <errno.h>
#include <unistd.h>
#include <cstring>
#include <string>
#include "exceptions.hpp"
//DEBUG
#include <iostream>
#define DEBUG_ACT true
namespace Socket {
 void DEBUG(std::string message)
 {
 if(DEBUG_ACT)
 std::cout << "DEBUG : " << message << std::endl;
 }
// Public :
 TcpSocket::TcpSocket()
 {
 mInfo = new addrinfo;
 memset(mInfo, 0, sizeof *mInfo);
 mInfo->ai_family = AF_UNSPEC;
 mInfo->ai_socktype = SOCK_STREAM;
 //Can't create socket now.
 }
 TcpSocket::TcpSocket(int family, int flags)
 {
 mInfo = new addrinfo;
 memset(mInfo, 0, sizeof *mInfo);
 mInfo->ai_family = family;
 mInfo->ai_socktype = SOCK_STREAM;
 mInfo->ai_flags = flags;
 if(family == AF_UNSPEC)
 //Can't create socket now
 return;
 mSock = socket(mInfo->ai_family, mInfo->ai_socktype, 0);
 if(mSock == -1)
 {
 SocketCreationException except(std::string(strerror(errno)));
 throw except;
 }
 //Socket succesfully "opened".
 mSockCreated = true;
 }
 TcpSocket::TcpSocket(int socket, addrinfo info, bool bound, bool connected)
 : mSock(socket), mBound(bound), mConnected(connected)
 {
 mInfo = new addrinfo(info);
 }
 TcpSocket::~TcpSocket()
 {
 if(!mClosed)
 close();
 freeaddrinfo(mInfo);
 }
 void TcpSocket::bind(int port)
 {
 if(mBound && mConnected)
 throw SocketBindingException("Already bound");
 setInfo(port);
 addrinfo * result;
 for(result = mInfo; result != NULL; result = mInfo->ai_next)
 {
 if(!mSockCreated)
 {
 try {
 openSocket(result);
 } catch(SocketCreationException &e) {
 continue;
 }
 }
 //Socket sucessfully opened from here
 if( ::bind(mSock, result->ai_addr, result->ai_addrlen) == 0)
 {
 mBound = true;
 return;
 }
 }
 //Couldn't bind, throw
 throw SocketBindingException("Can't bind to port");
 }
 void TcpSocket::connect(std::string address, int port)
 {
 if(mConnected)
 throw SocketConnectException("Already connected");
 setInfo(address, htons(port));
 addrinfo * result;
 for(result = mInfo; result != NULL; result = mInfo->ai_next)
 {
 if(!mSockCreated)
 {
 try {
 openSocket(result);
 } catch(SocketCreationException &e) {
 continue;
 }
 }
 //Socket sucessfully opened from here
 if( ::connect(mSock, result->ai_addr, result->ai_addrlen) == 0)
 {
 mConnected = true;
 return;
 }
 }
 //Couldn't connect, throw
 throw SocketConnectException("Can't connect to host");
 }
 void TcpSocket::listen(int maxQueue)
 {
 if( ::listen(mSock, maxQueue) != 0)
 throw SocketListenException(std::string(strerror(errno)));
 DEBUG("Listening...");
 }
 std::shared_ptr<TcpSocket> TcpSocket::accept()
 {
 DEBUG("Starting to accept");
 union
 {
 sockaddr addr;
 sockaddr_in in;
 sockaddr_in6 in6;
 sockaddr_storage s;
 } address;
 socklen_t addressSize = sizeof (sockaddr_storage);
 int newSock;
 if( (newSock = ::accept(mSock, (sockaddr*)&address.s, &addressSize)) == -1)
 throw SocketAcceptException(std::string(strerror(errno)));
 DEBUG("1 client accepted");
 addrinfo info;
 memset(&info, 0, sizeof info);
 if(address.s.ss_family == AF_INET)
 {
 info.ai_family = AF_INET;
 info.ai_addr = new sockaddr(address.addr);
 }
 else
 {
 info.ai_family = AF_INET6;
 info.ai_addr = new sockaddr(address.addr);
 }
 return std::shared_ptr<TcpSocket>(new TcpSocket(newSock, info, true, false));
 }
 void TcpSocket::send(const char *data, unsigned int length, int flags)
 {
 const char * buff = data;
 int status = 0;
 int total_sent = 0;
 int left_to_send = length;
 while(total_sent < length)
 {
 status = ::send(mSock, buff + total_sent, left_to_send, flags);
 if(status == -1)
 {
 throw SocketSendException(std::string(strerror(errno)));
 }
 else
 {
 total_sent += status;
 left_to_send -= status;
 }
 }
 }
 bool TcpSocket::receive(char* msg, int len, int flags)
 {
 int status;
 if( (status = ::recv(mSock, msg, len, flags)) == -1)
 throw SocketReceiveException(std::string(strerror(errno)));
 else if(status == 0)
 return false;
 return true;
 }
 void TcpSocket::close()
 {
 if( ::close(mSock) == -1)
 throw SocketCloseException(std::string(strerror(errno)));
 else
 mClosed = true;
 }
// Private :
 void TcpSocket::setInfo(int port)
 {
 setInfo("null", port);
 }
 void TcpSocket::setInfo(std::string address, int port)
 {
 const char *charAddress;
 if(address == "null")
 charAddress = NULL;
 else
 charAddress = address.c_str();
 addrinfo hints = *mInfo;
 int status;
 if( (status = getaddrinfo(charAddress, std::to_string(port).c_str(), &hints, &mInfo)) != 0)
 {
 delete charAddress;
 throw SocketException("getaddrinfo returned non-zero : " + std::string(gai_strerror(status)));
 }
 delete charAddress;
 }
 void TcpSocket::openSocket(addrinfo *info)
 {
 mSock = socket(info->ai_family, info->ai_socktype, info->ai_protocol);
 if(mSock == -1)
 {
 SocketCreationException except(std::string(strerror(errno)));
 throw except;
 }
 }
}

main.cpp (usage example, you don't need to review this file)

#include "TcpSocket.hpp"
#include <iostream>
#include <exception>
#include <cstring>
#include <memory>
using namespace Socket;
using Socket_p = std::shared_ptr<TcpSocket>;
int main(int argc, char *argv[])
{
 Socket_p sock(new TcpSocket);
 Socket_p client;
 try {
 sock->bind(1170);
 sock->listen(5);
 client = sock->accept();
 }
 catch( std::exception &e)
 {
 std::cout << e.what() << std::endl;
 }
 //Welcoming the new user.
 client->send("Welcome !\n\f", 15, 0);
 //Closing the listening soket, we want nobody else.
 sock->close();
 char data[512];
 memset(&data, 0, 512);
 while( client->receive(data, sizeof data, 0) )
 {
 client->send(data, sizeof data, 0);
 memset(&data, 0, 512);
 }
 client->close();
 return 0;
}

What is good is that it's all-purpose, and it seems to function great. However, the use of so many booleans look a bit sloppy to me, but it seemed like the best way to avoid common problems.

I'm also wondering if this all-purposeness is that good, because it seems like this class can't be extended or specialized.

What do you think of my approach? I'm also interested by any security concerns you may have as this is the first time I work with the C standard lib.

• Mast: Welcome to Code Review! Don't worry about your question, it looks good :-)
• Zeks: Well, you could replace those booleans with enum and use it as flags as in : stackoverflow.com/questions/1448396/… or you could use a bitset.
• glampert: I have a similar question you might take a look: codereview.stackexchange.com/q/68240/39810
• florian-s: @Zeks Oh, nice trick. But redefining every useful operators would complicate the class more. Or I can create a special type Socket::StateFlags rather than Socket::TcpSocket::StateFlags.

Answer 1

The code is generally clear, readable and easy to understand, which is great. However, you can still improve a few things.

• Generally speaking, you don't want to manage memory manually like you're currently doing for mInfo. There is an easy way to make it managed effortless. Create the following type:

  struct addrinfo_delete
  {
   void operator()(addrinfo* ptr) const
   {
   freeaddrinfo(ptr);
   }
  };
  

  Now, instead of storing an addrinfo* field in your TcpSocket class, you can make it store and std::unique_ptr<addrinfo, addrinfo_delete> field instead and let it memory by itself. Call it addrinfo_ptr and you will probably want to use it in some other places where you need an addrinfo*.

  Moreover, having an std::unique_ptr pointer member won't make you lose anything fature-wise since you already explicity made your class uncopyable in the first place.

• When possible, use std::make_shared<Foo>(bar); instead of std::shared_ptr<Foo> foo(new Foo{bar});. It will prevent the new/delete visual mismatch and it can also save some reference counting. Therefore, instead of defining a Socket_p class, I would be as explicit as possible and use std::shared_ptr<TcpSocket> directly.

• @Zeks advice is really good: you can pack your boolean flags into an std::bitset<4> to make your class more compact.

• You shouldn't have to write client->close(); at the end of your main since your class already does that in its destructor. Let the destructor do its job and it will be easier for everyone.

• It is good practice to always fully qualify every component from the standard library. For example use std::memset instead of memset. Not only can it avoid some name clashes, but it will also make it easier to look for std:: if you need to know which components from the standard library you're using.

• Whenever you use control flow statements like if, try to always use curly braces, even when you have a single statement following them. It will make it easier to add more statements (debug statements for example) and avoid problems of the Apple's goto fail kind.

• Another good practice is to always use nullptr instead of NULL. While it doesn't matter most of the time, when it does you're glad you used nullptr everywhere.

• florian-s: Those smart pointers start to make sense when you create a custom deleter. Thank you for your help ! The only thing that isn't clear is how I can use a std::bitset, wouldn't it make my code less semantic ? Doesn't it remove the possibility to name the flags ?
• Morwenn: @yhoyhoj If you want to pack things, you'll need a single variable anyway. Create getters and setters for the bitset to get the small size and the semantics.