openvpn Destructor for generic/top-level client in plugin not called on conditional restart

Describe the bug

When performing a conditional restart (SIGUSR1) of an OpenVPN server with a plugin, openvpn_plugin_client_constructor_v1 is called to create a new generic/top-level client instance without deconstructing the previous one with openvpn_plugin_client_destructor_v1. When the server is stopped after one or more conditional restarts, only the last instance is destructed.

This not only causes a memory leak, it also creates a logical issue when trying to distinguish the allocation of a generic/top-level client and a real client early on by counting allocations/deallocations.

To reproduce

Write a simple OpenVPN plugin (see below for a minimal example).
- Make an allocation in openvpn_plugin_client_constructor_v1 and return a pointer to it.
- Free the allocation in openvpn_plugin_client_destructor_v1.
Start an OpenVPN server with the plugin loaded.
Trigger a conditional restart of the OpenVPN server via SIGUSR1 to observe the issue.

Expected behavior

On a conditional restart, either openvpn_plugin_client_constructor_v1 should not be called, or openvpn_plugin_client_destructor_v1 should be called beforehand, so that there is no memory leak.

Version information

OS: Debian Bookworm
OpenVPN version: 2.6.13 (also 2.5.9)

Additional context

Minimal example:

#include <openvpn/openvpn-plugin.h>

#define PLUGIN_NAME         "Issue Demo"

OPENVPN_EXPORT int openvpn_plugin_open_v3(
    int version,
    const struct openvpn_plugin_args_open_in *args,
    struct openvpn_plugin_args_open_return *ret)
{
    ret->handle = args->callbacks;
    return OPENVPN_PLUGIN_FUNC_SUCCESS;
}

OPENVPN_EXPORT int openvpn_plugin_func_v3(
    int version,
    const struct openvpn_plugin_args_func_in *args,
    struct openvpn_plugin_args_func_return *ret)
{
    return OPENVPN_PLUGIN_FUNC_ERROR;
}

OPENVPN_EXPORT void openvpn_plugin_close_v1(openvpn_plugin_handle_t handle)
{
}

OPENVPN_EXPORT void *openvpn_plugin_client_constructor_v1(openvpn_plugin_handle_t handle)
{
    struct openvpn_plugin_callbacks *callbacks = handle;

    void *per_client_context = calloc(1, sizeof(int));
    callbacks->plugin_log(PLOG_ERR, PLUGIN_NAME,
        "### openvpn_plugin_client_constructor_v1: %p ###", per_client_context);

    return per_client_context;
}

OPENVPN_EXPORT void openvpn_plugin_client_destructor_v1(
    openvpn_plugin_handle_t handle,
    void *per_client_context)
{
    struct openvpn_plugin_callbacks *callbacks = handle;

    callbacks->plugin_log(PLOG_ERR, PLUGIN_NAME,
        "### openvpn_plugin_client_destructor_v1: %p ###", per_client_context);
    free(per_client_context);
}

OPENVPN_EXPORT int openvpn_plugin_min_version_required_v1()
{
    return 3;
}

Log:

2025-01-23 16:47:15 us=692534 OpenVPN 2.6.13 x86_64-pc-linux-gnu [SSL (OpenSSL)] [LZO] [LZ4] [EPOLL] [PKCS11] [MH/PKTINFO] [AEAD] [DCO]
...
2025-01-23 16:47:15 us=692829 PLUGIN Issue Demo: ### openvpn_plugin_client_constructor_v1: 0x5776fb579170 ###
...
2025-01-23 16:47:15 us=696927 Initialization Sequence Completed
...
2025-01-23 16:47:27 us=366162 SIGUSR1[hard,] received, process restarting
2025-01-23 16:47:27 us=366207 Restart pause, 1 second(s)
...
2025-01-23 16:47:28 us=367471 PLUGIN Issue Demo: ### openvpn_plugin_client_constructor_v1: 0x5776fb596700 ###
...
2025-01-23 16:47:36 us=391133 SIGUSR1[hard,] received, process restarting
2025-01-23 16:47:36 us=391172 Restart pause, 1 second(s)
...
2025-01-23 16:47:37 us=392536 PLUGIN Issue Demo: ### openvpn_plugin_client_constructor_v1: 0x5776fb585640 ###
...
2025-01-23 16:47:37 us=399453 Initialization Sequence Completed
...
^C
...
2025-01-23 16:47:41 us=730149 PLUGIN Issue Demo: ### openvpn_plugin_client_destructor_v1: 0x5776fb585640 ###
...
2025-01-23 16:47:41 us=730235 SIGINT[hard,] received, process exiting

Jan 23 '25 17:01 LennartF22

I have never used these constructor/destructor functions myself, but from your description this definitely sounds like something we need to look into. Pinging @dsommers as well.

Jan 23 '25 17:01 cron2

#include <stdlib.h>

#define PLUGIN_NAME "Issue Demo"

typedef struct {
    struct openvpn_plugin_callbacks *callbacks;
    void *client_instance;
} plugin_context_t;

OPENVPN_EXPORT int openvpn_plugin_open_v3(
    int version,
    const struct openvpn_plugin_args_open_in *args,
    struct openvpn_plugin_args_open_return *ret)
{
    plugin_context_t *context = calloc(1, sizeof(plugin_context_t));
    if (!context) {
        return OPENVPN_PLUGIN_FUNC_ERROR;
    }

    context->callbacks = args->callbacks;
    ret->handle = context;
    return OPENVPN_PLUGIN_FUNC_SUCCESS;
}

OPENVPN_EXPORT int openvpn_plugin_func_v3(
    int version,
    const struct openvpn_plugin_args_func_in *args,
    struct openvpn_plugin_args_func_return *ret)
{
    return OPENVPN_PLUGIN_FUNC_ERROR;
}

OPENVPN_EXPORT void openvpn_plugin_close_v1(openvpn_plugin_handle_t handle)
{
    plugin_context_t *context = handle;
    if (context) {
        free(context);
    }
}

OPENVPN_EXPORT void *openvpn_plugin_client_constructor_v1(openvpn_plugin_handle_t handle)
{
    plugin_context_t *context = handle;

    if (context->client_instance) {
        free(context->client_instance);
        context->client_instance = NULL;
    }

    void *per_client_context = calloc(1, sizeof(int));
    context->client_instance = per_client_context;
    return per_client_context;
}

OPENVPN_EXPORT void openvpn_plugin_client_destructor_v1(
    openvpn_plugin_handle_t handle,
    void *per_client_context)
{
    plugin_context_t *context = handle;

    free(per_client_context);
    context->client_instance = NULL;
}

OPENVPN_EXPORT int openvpn_plugin_min_version_required_v1()
{
    return 3;
}

/*Plugin Context: We create a small structure (plugin_context_t) to keep track of the plugin's callbacks and the current client instance. This helps us manage things better.

Plugin Open: When the plugin starts, we allocate memory for the context and store the callbacks. This is like setting up the basics for the plugin to work.

Plugin Func: This function doesn’t do much in this example. It just returns an error. You can add your logic here if needed.

Plugin Close: When the plugin is closed, we free the memory we allocated for the context. This is like cleaning up after ourselves.

Client Constructor: When a new client is created, we first check if there’s an old client instance. If yes, we clean it up. Then we allocate memory for the new client and return it. This ensures no memory is wasted.

Client Destructor: When a client is no longer needed, we free its memory and clear the pointer. This is like saying goodbye and cleaning up.

Min Version: We tell OpenVPN that this plugin needs at least version 3 to work.*/

Feb 10 '25 11:02 coderknk