Skip to content

mbus-protocol: Keep error string and decode buffers per thread - #245

Closed
moroznah wants to merge 1 commit into
rscada:masterfrom
moroznah:thread-local-statics
Closed

moroznah wants to merge 1 commit into
rscada:masterfrom
moroznah:thread-local-statics

Conversation

@moroznah

Copy link
Copy Markdown

libmbus records every failure's reason in one file-scope static char error_str[512] (returned by mbus_error_str()), and the normalized XML render returns its strings from function-static buffers. All threads in a process share both, so a program that drives M-Bus contexts from several threads:

  • reads another thread's failure reason. The TCP transport writes error_str on the receive timeout at the end of each exchange (mbus_tcp_recv_frame), so the reason a thread reads after its own failed exchange or decode can belong to another context's exchange, or be a mix of two.
  • gets torn renders. Two concurrent mbus_frame_data_xml_normalized() calls overwrite each other's header and record strings, so the XML carries another meter's fields or stops being well-formed.

A caller can serialize its own decodes, but no lock in the caller covers the transport's writes to error_str without serializing every meter exchange in the process.

Change

This declares __thread on error_str and on the eight function-static buffers that mbus_parse(), mbus_frame_data_parse() and mbus_frame_data_xml_normalized() reach:

storage function bytes
error_str file scope 512
buff mbus_data_variable_header_xml 4096
m_str mbus_decode_manufacturer 4
buff mbus_data_product_name 128
buff mbus_data_variable_medium_lookup 256
buff mbus_data_record_function 128
buff mbus_data_fixed_medium 256
buff mbus_data_fixed_unit 256
buff mbus_data_fixed_function 128

Nothing else changes: no signature, no allocation, no single-threaded behaviour. Each returned pointer is now the calling thread's array, valid for that thread's lifetime, and every caller inside the library copies it immediately (snprintf, strdup, mbus_str_xml_encode).

Out of scope here:

  • the eleven function-static buffers on paths the normalized render does not reach (mbus_data_variable_xml / mbus_frame_xml record helpers and their VIF/unit lookups, mbus_data_error_lookup, mbus_frame_get_secondary_address, which already has _to_buffer since Update mbus_frame_get_secondary_address() API #227);
  • tcp_timeout_sec/_usec and the debug flags, which are process-wide configuration.

I can extend the change to the eleven buffers if you prefer them all handled the same way.

Cost

The patched libmbus.so gets a 5,764-byte TLS segment (x86-64, default flags; the stock library has none). glibc lays out the TLS of a library loaded at program start in every thread's static TLS area and zeroes it when the thread is created, so every thread of a program linking libmbus carries it, including threads that never call the library. Measured with 256 idle threads: 13.0 KiB RSS per thread against 9.1 KiB stock, i.e. one extra page.

Portability

__thread rather than C11 _Thread_local, because configure.ac sets no -std: gcc and clang accept __thread without a diagnostic in every C mode, while _Thread_local draws a pedantic warning below C11. Built with gcc (x86-64, riscv64, i686) and clang. MSVC spells it __declspec(thread); if a Windows build matters, I can add a small macro.

Testing

  • test/generate-xml.sh over test-frames, error-frames and unsupported-frames gives the same exit codes on stock and patched builds.
  • mbus_parse_hex and mbus_parse_hex -n output for all 78 test frames is byte-identical between the two builds.
  • Multi-threaded reproducer (below; 16 threads, each decoding all test frames and error frames 200 times from a different starting frame, compared with a single-threaded reference). Three runs each:
build normalized renders differing mbus_error_str() differing
master ac52f3f 225,185 – 226,442 of 249,600 19 – 30 of 32,000
this PR 0 of 249,600 0 of 32,000
Reproducer: mbus_thread_repro.c
/* usage: mbus_thread_repro <test/test-frames> <test/error-frames>
 * build: cc -O2 -pthread mbus_thread_repro.c -lmbus -lm */
#include <mbus/mbus.h>
#include <dirent.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define NTHREADS 16
#define ROUNDS 200
#define MAXFRAMES 128

struct frame {
    unsigned char bin[4096];
    size_t len;
    char *ref;
};

static struct frame good[MAXFRAMES], bad[MAXFRAMES];
static int ngood, nbad;
static long torn_renders, wrong_reasons;

/* normalized XML on success, mbus_error_str() on failure; both malloc'd */
static char *
decode(const struct frame *f)
{
    mbus_frame reply;
    mbus_frame_data data;
    char *out;

    memset(&reply, 0, sizeof(reply));
    memset(&data, 0, sizeof(data));
    if (mbus_parse(&reply, (unsigned char *) f->bin, f->len) != 0)
        return strdup(mbus_error_str());
    if (mbus_frame_data_parse(&reply, &data) != 0)
    {
        out = strdup(mbus_error_str());
        mbus_data_record_free(data.data_var.record);
        return out;
    }
    out = mbus_frame_data_xml_normalized(&data);
    mbus_data_record_free(data.data_var.record);
    return out ? out : strdup("(null)");
}

static int
load(const char *dir, struct frame *set, int want_ok)
{
    DIR *d = opendir(dir);
    struct dirent *e;
    int n = 0;

    if (!d)
    {
        perror(dir);
        exit(2);
    }
    while ((e = readdir(d)) && n < MAXFRAMES)
    {
        char path[4096], raw[4096] = {0};
        mbus_frame reply;
        mbus_frame_data data;
        FILE *fp;
        int ok;

        if (!strstr(e->d_name, ".hex"))
            continue;
        snprintf(path, sizeof(path), "%s/%s", dir, e->d_name);
        if (!(fp = fopen(path, "r")))
            continue;
        fread(raw, 1, sizeof(raw) - 1, fp);
        fclose(fp);
        set[n].len = mbus_hex2bin(set[n].bin, sizeof(set[n].bin),
                                  (unsigned char *) raw, sizeof(raw));
        memset(&reply, 0, sizeof(reply));
        memset(&data, 0, sizeof(data));
        if (mbus_parse(&reply, set[n].bin, set[n].len) != 0)
            continue;
        ok = mbus_frame_data_parse(&reply, &data) == 0;
        mbus_data_record_free(data.data_var.record);
        if (ok != want_ok)
            continue;
        set[n].ref = decode(&set[n]);   /* single-threaded reference */
        n++;
    }
    closedir(d);
    return n;
}

static void *
worker(void *arg)
{
    long id = (long) arg, torn = 0, wrong = 0;
    int r, i;

    for (r = 0; r < ROUNDS; r++)
    {
        for (i = 0; i < ngood; i++)
        {
            const struct frame *f = &good[(i + id) % ngood];
            char *out = decode(f);
            torn += strcmp(out, f->ref) != 0;
            free(out);
        }
        for (i = 0; i < nbad; i++)
        {
            const struct frame *f = &bad[(i + id) % nbad];
            char *out = decode(f);
            wrong += strcmp(out, f->ref) != 0;
            free(out);
        }
    }
    __atomic_add_fetch(&torn_renders, torn, __ATOMIC_RELAXED);
    __atomic_add_fetch(&wrong_reasons, wrong, __ATOMIC_RELAXED);
    return NULL;
}

int
main(int argc, char *argv[])
{
    pthread_t t[NTHREADS];
    long i;

    if (argc != 3)
    {
        fprintf(stderr, "usage: %s test-frames-dir error-frames-dir\n", argv[0]);
        return 2;
    }
    ngood = load(argv[1], good, 1);
    nbad = load(argv[2], bad, 0);

    for (i = 0; i < NTHREADS; i++)
        pthread_create(&t[i], NULL, worker, (void *) i);
    for (i = 0; i < NTHREADS; i++)
        pthread_join(t[i], NULL);

    printf("renders differing from reference: %ld of %ld (%d frames)\n",
           torn_renders, (long) NTHREADS * ROUNDS * ngood, ngood);
    printf("reasons differing from reference: %ld of %ld (%d frames)\n",
           wrong_reasons, (long) NTHREADS * ROUNDS * nbad, nbad);
    return torn_renders || wrong_reasons;
}

libmbus records every failure's reason in one file-scope error_str[512]
and returns the normalized XML render's strings from function-static
buffers, so all threads in a process share them. A program that drives
M-Bus contexts from several threads therefore reads another thread's
failure reason (the TCP transport writes error_str on the receive
timeout at the end of each exchange), and two concurrent
mbus_frame_data_xml_normalized() calls tear each other's output.

Declare error_str and the eight function-static buffers reachable from
mbus_parse(), mbus_frame_data_parse() and
mbus_frame_data_xml_normalized() __thread. No signature, allocation or
single-threaded behaviour changes; each returned pointer is the calling
thread's array, and every caller inside the library copies it
immediately.
@lategoodbye

Copy link
Copy Markdown
Collaborator

Thank you for your contribution. This library wasn't designed for a multi-thread environment. So in case this would be merged, users would expect the library to handle multi-thread in all situations.

I'm sorry, but I don't have the resources to maintain such a feature.

@moroznah moroznah closed this Nov 4, 2026
@moroznah
moroznah deleted the thread-local-statics branch November 4, 2026 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants