Search code examples
boost

Boost asio: including <arpa/inet.h> causes socket to always output 0 bytes


I'm trying to include <arpa/inet.h> in a low-level library so that I have access to hton* and ntoh* functions in the library. The low-level library gets called into by higher-level code running a Boost asio socket. I'm aware Boost asio contains the hton* and ntoh* functions, but i'd like to avoid linking all of Boost asio to the library since hton*/ntoh* are all I need.

However, if I simply include <arpa/inet.h> in the low-level library, 0 bytes always will be sent from the Boost asio socket. Confirmed by Wireshark.

Here's the class where i'd like to include <arpa/inet.h> but not Boost. If <arpa/inet.h> is included, 0 bytes will be sent.

#pragma pack(push, 1)

#include "PduHeader.h"
#include <arpa/inet.h>

class ClientInfoPdu
{
public:
    ClientInfoPdu(const uint16_t _client_receiver_port)
    {
        set_client_receiver_port(_client_receiver_port);
    }

    PduHeader pdu_header{CLIENT_INFO_PDU, sizeof(client_receiver_port)};


    inline void set_client_receiver_port(const uint16_t _client_receiver_port)
    {
        //client_receiver_port = htons(_client_receiver_port);
        client_receiver_port = _client_receiver_port;
    }

    inline uint16_t get_client_receiver_port()
    {
        return client_receiver_port;
    }

    inline size_t get_total_size()
    {
        return sizeof(PduHeader) + pdu_header.get_pdu_payload_size();
    }

private:
    uint16_t client_receiver_port;
};

#pragma pack(pop)

Here's the higher level code that includes Boost and attempts to send the data via a socket. The printout indicates 5 bytes were sent, however 0 bytes were actually sent.

#include "ServerConnectionThread.h"
#include "config/ClientConfig.h"
#include "protocol_common/ClientInfoPdu.h"
#include <boost/asio.hpp>
#include <unistd.h>

using boost::asio::ip::udp;

void ServerConnectionThread::execute()
{
    boost::asio::io_service io_service;
    udp::endpoint remote_endpoint = 
    udp::endpoint(boost::asio::ip::address::from_string(SERVER_IP), SERVER_PORT);
    udp::socket socket(io_service);
    socket.open(udp::v4());

    ClientInfoPdu client_info_pdu = ClientInfoPdu(RECEIVE_PORT);

    while (true)
    {
        uint16_t total_size = client_info_pdu.get_total_size();
        socket.send_to(boost::asio::buffer(&client_info_pdu, total_size), remote_endpoint);
        printf("sent %u bytes\n", total_size);
        usleep(1000000);
    }
}

Again, simply removing "#include <arpa/inet.h>" will cause this code to function as expected and send 5 bytes per packet.


Solution

  • How is ClientInfoPdu defined? This looks like it is likely UB:

    boost::asio::buffer(&client_info_pdu, total_size)
    

    The thing is total size is sizeof(PduHeader) + pdu_header.get_pdu_payload_size() (so sizeof(PduHeader) + 2);

    First problem is that you're mixing access modifiers, killing the POD/standard_layout properties of your types.

    #include <type_traits>
    static_assert(std::is_standard_layout_v<PduHeader> && std::is_trivial_v<PduHeader>);
    static_assert(std::is_standard_layout_v<ClientInfoPdu> && std::is_trivial_v<ClientInfoPdu>);
    

    This will fail to compile. Treating the types as POD (as you do) invokes Undefined Behaviour.

    This is likely the explanation for the fact that "it stops working" with some changes. It never worked: it might just accidentally have appeared to work, but it was undefined behaviour.

    It's not easy to achieve POD-ness while still getting the convenience of the constructors. In fact, I don't think that's possible. In short, if you want to treat your structs as C-style POD types, make them... C-style POD types.

    Another thing: a possible implementation of `PduHeader I can see working for you looks a bit like so:

    enum MsgId{CLIENT_INFO_PDU=0x123};
    struct PduHeader {
        MsgId id;
        size_t payload_size;
        size_t get_pdu_payload_size() const { return payload_size; }
    };
    

    Here, again you might have/need endianness conversions.

    Suggestion

    In short, if you want this to work, I'd say keep it simple.

    Instead of creating non-POD types all over the place that are responsible for endianness conversion by adding getters/setters for each value, why not create a simple user-defined-type that does this always, and use them instead?

    struct PduHeader {
        Short id; // or e.g. uint8_t
        Long payload_size;
    };
    
    struct ClientInfoPdu {
        PduHeader pdu_header; // or inheritance, same effect
        Short client_receiver_port;
    };
    

    Then just use it as a POD struct:

    while (true) {
        ClientInfoPdu client_info_pdu;
        init_pdu(client_info_pdu);
    
        auto n = socket.send_to(boost::asio::buffer(&client_info_pdu, sizeof(client_info_pdu)), remote_endpoint);
        printf("sent %lu bytes\n", n);
        std::this_thread::sleep_for(1s);
    }
    

    The function init_pdu can be implemented with overloads per submessage:

    void init_pdu(ClientInfoPdu& msg) {
        msg.pdu_header.id = CLIENT_INFO_PDU;
        msg.pdu_header.payload_size = sizeof(msg);
    }
    

    There are variations on this where it can become a template or take a PduHeder& (if your message inherits instead of aggregates). But the basic principle is the same.

    Endianness Conversion

    Now you'll noticed I avoided using uint32_t/uint16_t directly (though uint8_t is fine because it doesn't need byte ordering). Instead, you could define Long and Short as simple POD wrappers around them:

    struct Short {
        operator uint16_t() const { return ntohs(value); }
        Short& operator=(uint16_t v) { value = htons(v); return *this; }
      private:
        uint16_t value;
    };
    
    struct Long {
        operator uint32_t() const { return ntohl(value); }
        Long& operator=(uint32_t v) { value = htonl(v); return *this; }
      private:
        uint32_t value;
    };
    

    The assignment and conversions mean that you can use it as just another int32_t/int16_t except that the necessary conversions are always done.

    If you want to satnd on the shoulder of giants instead, you can use the better types from Boost Endian, which also has lots more advanced facilities

    DEMO

    Live On Coliru

    #include <type_traits>
    #include <cstdint>
    #include <thread>
    #include <arpa/inet.h>
    using namespace std::chrono_literals;
    
    #pragma pack(push, 1)
    
    enum MsgId{CLIENT_INFO_PDU=0x123};
    
    struct Short {
        operator uint16_t() const { return ntohs(value); }
        Short& operator=(uint16_t v) { value = htons(v); return *this; }
      private:
        uint16_t value;
    };
    
    struct Long {
        operator uint32_t() const { return ntohl(value); }
        Long& operator=(uint32_t v) { value = htonl(v); return *this; }
      private:
        uint32_t value;
    };
    
    static_assert(std::is_standard_layout_v<Short>);
    static_assert(std::is_trivial_v<Short>);
    
    static_assert(std::is_standard_layout_v<Long>);
    static_assert(std::is_trivial_v<Long>);
    
    struct PduHeader {
        Short id; // or e.g. uint8_t
        Long payload_size;
    };
    
    struct ClientInfoPdu {
        PduHeader pdu_header; // or inheritance, same effect
        Short client_receiver_port;
    };
    
    void init_pdu(ClientInfoPdu& msg) {
        msg.pdu_header.id = CLIENT_INFO_PDU;
        msg.pdu_header.payload_size = sizeof(msg);
    }
    
    static_assert(std::is_standard_layout_v<PduHeader> && std::is_trivial_v<PduHeader>);
    static_assert(std::is_standard_layout_v<ClientInfoPdu> && std::is_trivial_v<ClientInfoPdu>);
    
    #pragma pack(pop)
    
    #include <boost/asio.hpp>
    //#include <unistd.h>
    
    using boost::asio::ip::udp;
    
    #define SERVER_IP "127.0.0.1"
    #define SERVER_PORT 6767
    #define RECEIVE_PORT 6868
    
    struct ServerConnectionThread {
        void execute() {
            boost::asio::io_service io_service;
            udp::endpoint const remote_endpoint = 
                udp::endpoint(boost::asio::ip::address::from_string(SERVER_IP), SERVER_PORT);
            udp::socket socket(io_service);
            socket.open(udp::v4());
    
            while (true) {
                ClientInfoPdu client_info_pdu;
                init_pdu(client_info_pdu);
    
                auto n = socket.send_to(boost::asio::buffer(&client_info_pdu, sizeof(client_info_pdu)), remote_endpoint);
                printf("sent %lu bytes\n", n);
                std::this_thread::sleep_for(1s);
            }
        }
    };
    
    int main(){ }