Cannot send datagrams from server to client

[a-andre]

Hello,

I'm trying to send datagram packets between client and server. Currently, datagrams can be send from client to server, but not from server to client.
The problem seems to be that for cnx on server side remote_parameters.max_datagram_frame_size is zero.

How can remote_parameters.max_datagram_frame_size be set to a none-zero value on server side?

Source code of test application (save as "application.cpp")

#include <chrono>
#include <cstdint>
#include <cstring>
#include <iostream>
#include <string>
#include <thread>

#include <picoquic.h>
#include <picoquic_packet_loop.h>

#define PICOQUIC_SAMPLE_ALPN "sample_alpn"

bool g_is_server = false;

int sample_default_callback(picoquic_cnx_t* cnx,
    uint64_t stream_id, uint8_t* bytes, size_t length,
    picoquic_call_back_event_t fin_or_event, void* callback_ctx, void* v_stream_ctx)
{
    switch (fin_or_event) {
    case picoquic_callback_stateless_reset:
        std::cout << "picoquic_callback_stateless_reset" << std::endl;
        break;
    case picoquic_callback_close:
        std::cout << "picoquic_callback_close" << std::endl;
        break;
    case picoquic_callback_almost_ready:
        std::cout << "Connection  completed, almost ready." << std::endl;
        break;
    case picoquic_callback_ready:
        std::cout << "Connection confirmed." << std::endl;
        break;
    case picoquic_callback_datagram:
        std::cout << "picoquic_callback_datagram: received data: " << bytes << std::endl;
        break;
    case picoquic_callback_prepare_datagram:
    {
        std::cout << "picoquic_callback_prepare_datagram" << std::endl;
        auto buffer = picoquic_provide_datagram_buffer_ex(bytes, length, picoquic_datagram_not_active);
        if (buffer == nullptr) {
            std::cerr << "error" << std::endl;
            return -1;
        }
        std::string msg = "Hello ";
        if (g_is_server)
            msg.append("from Server");
        else
            msg.append("from Client");
        memcpy(buffer, msg.c_str(), msg.length());
        break;
    }
    case picoquic_callback_datagram_acked:
        std::cout << "picoquic_callback_datagram_acked" << std::endl;
        break;
    case picoquic_callback_datagram_lost:
        std::cout << "picoquic_callback_datagram_lost" << std::endl;
        break;
    case picoquic_callback_datagram_spurious:
        std::cout << "picoquic_callback_datagram_spurious" << std::endl;
        break;
    default:
        std::cout << "default " << fin_or_event << std::endl;
    }
    return 0;
}

static int sample_loop_cb(picoquic_quic_t* quic, picoquic_packet_loop_cb_enum cb_mode, 
    void* callback_ctx, void * callback_arg)
{
    if (callback_ctx == nullptr) {
        return PICOQUIC_ERROR_UNEXPECTED_ERROR;
    }

    return 0;
}

void sample_sleep()
{
    while (true)
        std::this_thread::sleep_for(std::chrono::milliseconds(100));
}

int main(int argc, char** argv)
{
    bool is_server = false;

    std::string server_name = "127.0.0.1";
    int server_port = 4443;

    std::string sni = "test.example.com";
    
    for (int i = 1; i < argc; i++) {
        std::string arg(argv[i]);

        if (arg == "-s") {
            is_server = true;
            g_is_server = true;
        } else if (arg == "--server") {
            server_name = argv[i + 1];
        } else if (arg == "--port") {
            server_port = atoi(argv[i + 1]);
        }
    }

    uint32_t max_nb_connections = is_server ? 8 : 1;
    char const* cert_file_name = is_server ? "ca-cert.pem" : nullptr;
    char const* key_file_name = is_server ? "ca-key.pem" : nullptr;
    uint64_t current_time = picoquic_current_time();
    picoquic_quic_t *quic = picoquic_create(max_nb_connections, cert_file_name, key_file_name, nullptr,
        PICOQUIC_SAMPLE_ALPN, sample_default_callback, nullptr,
        nullptr, nullptr, nullptr, current_time, nullptr,
        nullptr, nullptr, 0);

    if (quic == nullptr) {
        std::cerr << "Could not create quic context" << std::endl;
        return -1;
    }

    picoquic_enable_sslkeylog(quic, 1);
    picoquic_set_key_log_file_from_env(quic);

    auto inital_tansport_data = const_cast<picoquic_tp_t *>(picoquic_get_default_tp(quic));
    inital_tansport_data->max_datagram_frame_size = 65535;
    picoquic_set_default_tp(quic, inital_tansport_data);

    picoquic_cnx_t* cnx = nullptr;
    picoquic_packet_loop_param_t param = { 0 };

    int ret = 0;
    struct sockaddr_storage server_address;
    if (is_server) {
        param.local_af = AF_INET;
        param.local_port = server_port;
    } else {
        int is_name = 0;
        ret = picoquic_get_server_address(server_name.c_str(), server_port, &server_address, &is_name);
        param.local_af = server_address.ss_family;
    }

    cnx = picoquic_create_cnx(quic, picoquic_null_connection_id, picoquic_null_connection_id,
            (struct sockaddr*)&server_address, current_time, 0, sni.c_str(), PICOQUIC_SAMPLE_ALPN, 1);

    if (cnx == nullptr) {
        std::cerr << "Could not create connection context" << std::endl;
        return -1;
    }

    if (!is_server) {
        ret = picoquic_start_client_cnx(cnx);
    }

    auto th = picoquic_start_network_thread(quic, &param, sample_loop_cb, &cnx, &ret);
    if (th == nullptr) {
        std::cerr << "Could not start network thread" << std::endl;
        if (ret == 0)
            return -1;
        return ret;
    }

    for (int i = 0; i < 2000; i++) {
        if (th->thread_is_ready) {
            break;
        }
        else {
            usleep(1000);
        }
    }

    picoquic_mark_datagram_ready(cnx, 1);

    std::thread sample_wait(sample_sleep);
    sample_wait.join();

    picoquic_delete_network_thread(th);
    picoquic_delete_cnx(cnx);

    return 0;
}

Source code of "CMakeLists.txt"

cmake_minimum_required(VERSION 3.18)

project(application VERSION 0.0 LANGUAGES CXX)

include(ExternalProject)
ExternalProject_Add(picoquic
    GIT_REPOSITORY  https://github.com/private-octopus/picoquic.git
    GIT_TAG         master
    CMAKE_ARGS "-DPICOQUIC_FETCH_PTLS:BOOL=ON"
               "-DCMAKE_INSTALL_PREFIX:PATH=<INSTALL_DIR>"
               "-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}"
)
ExternalProject_Get_Property(picoquic install_dir)
file(MAKE_DIRECTORY ${install_dir}/include)

add_library(pico_core STATIC IMPORTED)
set_property(TARGET pico_core PROPERTY IMPORTED_LOCATION ${install_dir}/lib/libpicoquic-core.a)
target_include_directories(pico_core INTERFACE ${install_dir}/include)
add_dependencies(pico_core picoquic)

add_library(pico_tls_core STATIC IMPORTED)
set_property(TARGET pico_tls_core PROPERTY IMPORTED_LOCATION ${install_dir}/lib/libpicotls-core.a)
target_include_directories(pico_tls_core INTERFACE ${install_dir}/include)
add_dependencies(pico_tls_core picoquic)

add_library(pico_tls_fusion STATIC IMPORTED)
set_property(TARGET pico_tls_fusion PROPERTY IMPORTED_LOCATION ${install_dir}/lib/libpicotls-fusion.a)
target_include_directories(pico_tls_fusion INTERFACE ${install_dir}/include)
add_dependencies(pico_tls_fusion picoquic)

add_library(pico_tls_crypto STATIC IMPORTED)
set_property(TARGET pico_tls_crypto PROPERTY IMPORTED_LOCATION ${install_dir}/lib/libpicotls-minicrypto.a)
target_include_directories(pico_tls_crypto INTERFACE ${install_dir}/include)
add_dependencies(pico_tls_crypto picoquic)

add_library(pico_tls_ssl STATIC IMPORTED)
set_property(TARGET pico_tls_ssl PROPERTY IMPORTED_LOCATION ${install_dir}/lib/libpicotls-openssl.a)
target_include_directories(pico_tls_ssl INTERFACE ${install_dir}/include)
target_link_libraries(pico_tls_ssl INTERFACE OpenSSL::SSL)
add_dependencies(pico_tls_ssl picoquic)

find_package(OpenSSL REQUIRED)

add_executable(application application.cpp)
target_link_libraries(application PUBLIC pico_core pico_tls_core pico_tls_crypto pico_tls_fusion pico_tls_ssl)

For the server to start ca-key.pem and ca-cert.pem need to be available.

To start the server run ./application -s and to start the client ./application.

[huitema]

Once you have created the quic context, i.e., after the call to quic_create, you need to set the default transport parameter values that will be used for server side connections.

The API picoquic_tp_t const* picoquic_get_default_tp(picoquic_quic_t* quic) returns a pointer to the current value of the transport parameters (the structure picoquic_tp_t is defined in picoquic.h). It returns a const pointer. Because of that const restriction, you need to do a three-step:

/* copy the current TP value to a local structure */
picoquic_tp_t quic_tp = *picoquic_get_default_tp(quic);
/* update the local copy to set the parameters that we want to modify */
quic_tp.max_datagram_frame_size  = <maximum datagram size for the app>;
/* copy the local value of transport parameters to the quic context */
picoquic_set_default_tp(quic, &quic_tp);

Section 3 of RFC 9221 suggests setting the maximum datagram size to 65535, indicating that the application will accept any datagram size, as long as it fits inside a packet. Of course, that does not mean that you can send datagrams of any size: they have to fit inside the packet size, which is constrained by the path MTU. The queuing API picoquic_queue_datagram_frame will not accept datagram longer than the conservative value PICOQUIC_DATAGRAM_QUEUE_MAX_LENGTH (1200 bytes), which will work even if the path MTU is the lowest value acceptable for QUIC. The just in time API will indicate how many bytes are available in the packet.

The three step process is a legacy of an earlier design, which required that picoquic_get_default_tp returns a const parameter. Maybe we should change that, and just let applications write directly into the default_tp field of the QUIC context. Please open a bug if you feel strongly about that.

[a-andre]

I basically did the same in the code provided in my initial question

    auto inital_tansport_data = const_cast<picoquic_tp_t *>(picoquic_get_default_tp(quic));
    inital_tansport_data->max_datagram_frame_size = 65535;
    picoquic_set_default_tp(quic, inital_tansport_data);

(I replaced that part with your suggestion, but nothing changed.)

With these three lines, datagrams can be sent from client to server. But for some reason it doesn't work in the other direction.

[a-andre]

How are you expecting to send datagrams from the server?

In this minimal example, I'm calling picoquic_mark_datagram_ready(cnx, 1); on server side, after the connection to the client has be established. This should trigger picoquic_callback_prepare_datagram, but it doesn't.

Are you using the queuing API or the just in time API?

I'm using the just in time API.

[huitema]

I would need to see the code to understand what is happening. That API is used in multiple applications, and it behaves as expected.

[a-andre]

The code is provided in #1827 (comment). You can click on the lines that say "Source code of .." to show it.

[huitema]

I am looking at your code, and I can see two issues:

    case picoquic_callback_prepare_datagram:
    {
        std::cout << "picoquic_callback_prepare_datagram" << std::endl;
        auto buffer = picoquic_provide_datagram_buffer_ex(bytes, length, picoquic_datagram_not_active);
        if (buffer == nullptr) {
            std::cerr << "error" << std::endl;
            return -1;
        }
        std::string msg = "Hello ";
        if (g_is_server)
            msg.append("from Server");
        else
            msg.append("from Client");
        memcpy(buffer, msg.c_str(), msg.length());
        break;
    }

1. In the call to picoquic_provide_datagram_buffer_ex, you set the continuation state to "not active". The next callback will only happen after you call picoquic_mark_datagram_ready again.
2. In the call to picoquic_provide_datagram_buffer_ex you allocate a buffer of size length, i.e., the number of bytes available for a datagram in the packet. But then, you write msg.length() bytes in that buffer. This will cause a buffer over flow if the number of allocated bytes is lower than the message length. If that number is slower, you will end up sending length bytes, with the bytes between msg.length() and length set to whatever is in memory. You should check "length >= msg.length()" before calling picoquic_provide_datagram_buffer_ex with the requested buffer length set to exactly the expected message size.

But this is not the direct cause of your issue. Instead, I looked at your main function, and see that you are calling:

    cnx = picoquic_create_cnx(quic, picoquic_null_connection_id, picoquic_null_connection_id,
            (struct sockaddr*)&server_address, current_time, 0, sni.c_str(), PICOQUIC_SAMPLE_ALPN, 1);

... on both the server and the client! This is wrong. If you call this on the server, you are actually creating a connection from the server to the specified address. You will end up with 4 connection context:

1. The connection context created in the client process for the client connection.
2. The connection context created in the server process for the explicitly created server connection.
3. The connection context automatically created by the server in response to the client connection request.
4. The connection context automatically created by the server in response to its own connection request.

You should only create the client connection if the main process is a client. The server side will create the connection context automatically when the request from the client arrives, and then issue a callback informing that application. You should call the picoquic_mark_datagram_ready in a server callback, according to the server logic.

[a-andre]

To keep this example simple I avoided the buffer length check.

Thanks for your help.