#132 Shared library: memory leak in ngGet_Evt_NodeInfo result cleanup

[George]

While adding OP/DC digital-node support to NgspiceTclBridge, I found a memory leak in the shared-library event query implementation.

The attached patch is against commit a80f6e3e95d51534905b1f23410a951802666656 (master, ngspice-47) .

In src/xspice/evt/evtshared.c, delete_ret() frees the individual records in return_all->evt_dect and their values, followed by return_all, but does not free the evt_dect pointer array itself.

Repeated calls to ngGet_Evt_NodeInfo() therefore leak one pointer array per replaced query result. This becomes relevant when querying digital-node states repeatedly during DC sweeps or repeated simulations.

There is also a lifecycle issue: the final cached query result remains allocated when its circuit is destroyed.

The proposed patch:

• Frees return_all->evt_dect in delete_ret().
• Clears the cached pointers after cleanup.
• Releases the shared query cache during EVTdest() in shared-library builds.
• Adds null checks for missing event structures and node data in EVTshareddata().

To exercise the affected path, run a mixed-signal circuit containing an ADC bridge, then repeatedly call ngGet_Evt_NodeInfo() for one of its digital nodes. Each query replaces the previous cached result; the pointer array from that previous result is currently left allocated.

The patch was built and tested on Linux with the shared library. Integration tests covering OP/DC digital-node queries, repeated runs, and circuit replacement passed.

No public API or event-value changes are intended. Could you please review the cleanup and confirm whether EVTdest() is the appropriate place to release the final cached result?

[Giles Atkinson]

Needs reproducible test case.

[George]

Test case:

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define XSPICE 1
#include <ngspice/sharedspice.h>

static int output(char *text, int id, void *user)
{
    (void)id;
    (void)user;
    fprintf(stderr, "%s\n", text);
    return 0;
}

int main(int argc, char **argv)
{
    if (argc < 2) {
        fprintf(stderr, "Usage: %s /path/to/digital.cm [queries]\n", argv[0]);
        return 1;
    }

    int queries = argc > 2 ? atoi(argv[2]) : 1000000;

    ngSpice_Init(output, NULL, NULL, NULL, NULL, NULL, NULL);

    char load[strlen(argv[1]) + sizeof("codemodel ")];
    snprintf(load, sizeof load, "codemodel %s", argv[1]);
    ngSpice_Command(load);

    char *deck[] = {
        "Event query memory leak",
        "vin in 0 0",
        "rload in 0 1k",
        "adc [in] [d] adcmod",
        ".model adcmod adc_bridge(in_low=0.5 in_high=1.5)",
        ".end",
        NULL
    };
    ngSpice_Circ(deck);

    char op[] = "op";
    ngSpice_Command(op);

    pevt_shared_data data = ngGet_Evt_NodeInfo("d");
    if (!data || !data->num_steps)
        return 1;

    printf("Initial state: %s\n", data->evt_dect[0]->node_value);

    for (int i = 0; i < queries; ++i)
        ngGet_Evt_NodeInfo("d");

    printf("Completed %d queries of the same stored result.\n", queries);
    return 0;
}

./event_query_leak /path/to/digital.cm 1000
./event_query_leak /path/to/digital.cm 1000000

Results (measured peak RSS on Linux):

Queries	Without cleanup patch	With cleanup patch
1,000	7,432 KiB	7,600 KiB
1,000,000	38,832 KiB	7,496 KiB

[Giles Atkinson]

Now it begins to look like a decent bug report. The leak is clear, but the proposed fix contains unnecessary stuff: ckt->evt is unconditionally valid and the returned memory should not be freed in evtdest.c, as the code implicitly (doc and code) transfers ownership to the caller.

Missing is any mention of a serious documentation gap: the function is a trap for anyone who does not read the source, as each call invalidates previous results. Fix in progress.