Heap accounting ($SYS/broker/heap/current) is incorrect

[rmk92]

Mosquitto version: 2.0.11, 2.0.20
Platform: 32-bit ARM running Debian Stable (Bookworm)

Bug: The value reported via $SYS/broker/heap/current gradually increases when the broker is receiving MQTT v5 publish with properties, despite the memory usage for mosquitto reported by ps not increasing.

Having tracked this down, this is due to the mosquitto_property_add_*() family of allocating the mosquitto_property struct using mosquitto__calloc() (which adds the usable size of the allocation to memcount) but property__free() (called by e.g. mosquitto_property_free_all() ) using a direct call to free(), and thus not removing the usable size from memcount.

Thus, if one makes use of the memory_limit configuration option, one eventually runs into the allocator functions incorrectly refusing to allocate memory.

Expected behaviour: $SYS/broker/heap/current should gradually increase over time when the actual memory usage of the mosquitto process is not increasing.

The incorrect call to free() was located using a custom-written preloaded shared object to intercept the C library allocation/free calls, track the memory allocations, verify that there are indeed no leaks, but also check how they are called (e.g. from where in the binary, or in the case of the tail-called free(), whether ARM register R2 contains the pointer to memcount) and print this information. This was then followed by analysis of the assembly and C code to validate the results.

[rmk92]

Note that the simple fix of replacing the free() in property__free() with mosquitto__free() is not possible because mosquitto_property_copy_all() is used to copy the properties, and the copied properties are allocated using the unwrapped allocators, but they are then freed using mosquitto_property_free_all() - which is also used to free the properties that were allocated using the wrapped allocators.

For example, lib/connect.c::mosquitto_connect_bind_v5() does:
mosquitto_property_free_all(&mosq->connect_properties);
and:
rc = mosquitto_property_copy_all(&mosq->connect_properties, properties);

src/send_connack.c::send__connack() also does:
rc = mosquitto_property_copy_all(&connack_props, properties);
...
mosquitto_property_free_all(&connack_props);

Both of these result in the mosquitto_properties structure being allocated with calloc() and freed with free(), but the property data itself are allocated with the unwrapped *alloc() and freed with mosquitto__free(). This leads to memcount being reduced more than it should.

Whereas src/handle_publish.c::handle__publish() does:

rc = property__read_all(CMD_PUBLISH, &context->in_packet, &properties);

which allocates the mosquitto_properties structure using mosquitto_calloc(), and the property data itself is allocated with mosquitto_*alloc(), and then it calls:

mosquitto_property_free_all(&properties);

This leads to memcount increasing despite all the allocated memory being freed.

[ralight]

Thanks very much for the analysis. We've got functionality in the develop branch to help catch these sorts of mismatches but have obviously missed porting back some of the fixes.