EncCache invalid key ordering and buffer ownership cause leaks with multiple viewers #371

Describe the bug

EncCache has an invalid std::map comparator and loses ownership of encoded image buffers when an existing key is replaced. It also retains the old payload length on replacement and does not release remaining buffers at destruction.

These defects are present in both v1.5.0 and master at 98d25c6. The cache is enabled when more than one viewer is connected, making it relevant to memory growth while sharing an actively changing desktop.

Runtime observation (unmodified v1.5.0)

In an isolated Linux test environment with 4 vCPUs, 16 GiB RAM and a 1440×900 desktop, I displayed an automatically scrolling list in the desktop browser and connected browser viewers to the same session. I measured the Xkasmvnc process's PSS from /proc/<pid>/smaps_rollup:

Generic runtime reproduction: start v1.5.0, display a continuously scrolling list with varied content, connect one browser viewer and sample process memory, then add a second viewer without changing the workload and continue sampling.

This is a short runtime observation, not a long-duration benchmark. The original environment has been removed; I do not have a complete OS/kernel/browser version capture to attach. I have not built or run a patched full Xkasmvnc binary for a before/after runtime comparison. The isolated cache tests below establish the source defects, but do not prove that they account for all of the observed process memory growth.

Deterministic source-level reproduction

The comparator ORs the less-than comparisons of each field. For example, the following assertion fails with the upstream header:

#include <cassert>
#include <rfb/EncCache.h>

int main() {
    rfb::EncId a{}, b{};
    a.type = b.type = 7;
    a.w = b.w = a.h = b.h = 16;
    a.x = 0; a.y = 1;
    b.x = 1; b.y = 0;
    assert(!(a < b && b < a));
}

This violates strict weak ordering, so std::map insertion/lookup behavior cannot be relied on. Separately, cache[id] = data overwrites the old allocation without freeing it. Since len lives in the map key but is excluded from key identity, replacing an entry also leaves its previous length in place. The destructor is empty, despite clear() explicitly owning and freeing the buffers.

Expected behavior: each distinct encoding/rectangle key can be found reliably; replacement frees the old buffer and returns the new data and length; clear/destruction release all owned buffers.

Proposed fix and regression coverage

I have prepared a focused fix with lexicographic key ordering, payload length stored with the value, safe buffer replacement, and destructor cleanup. A standalone C++ test target compiles the actual cache implementation and covers ordering, five frames of 500 rectangles retrieved by a second viewer, replacement (including changed lengths and the same pointer), clear, and destruction. The fixed implementation passes all four tests in a Linux GCC 14 container with AddressSanitizer and LeakSanitizer enabled.

I checked the existing memory reports (#101, #257 and #204) and current open PRs; I did not find an existing report or patch identifying this EncCache defect. In particular, this report concerns Xkasmvnc's encoded-rectangle cache, not the kasmxproxy leak in #257.