xcb_get_image_reply() NULL Deref on malloc() Failure

A crash path hiding inside a routine image-copy routine

Screen-streaming code spends most of its time doing the same unglamorous thing: pull a frame from the X server, copy it into a buffer, hand it to the next stage of the pipeline. That repetition is exactly where defensive checks get skipped — the code "always works," until the one time memory is tight. That's what happened in xcb_get_image_reply(), the function responsible for turning a raw XCB protocol reply into a usable xcb_get_image_reply_t structure for the streaming bridge.

The function computes a buffer size from the reply length (len = (size_t)r2->length * 4), calls malloc() to allocate room for both the struct header and the pixel payload, and then immediately writes into that memory with memset() and memcpy(). There was no check between the allocation and the writes. If malloc() returned NULL — which it will do under memory exhaustion, a plausible condition for a long-running screen-capture process pushing frame after frame — the very next instruction dereferences a null pointer.

Affected Versions

The Vulnerability Explained

Here's the code path before the fix:

size_t len = (size_t)r2->length * 4;
xcb_get_image_reply_t *out =
    malloc(sizeof(xcb_get_image_reply_t) + len);
memset(out, 0, sizeof(xcb_get_image_reply_t));
out->response_type = 1;
out->depth = r2->depth;

The problem is simple to state and easy to miss in review: out is used by memset() on the very next line with no check that malloc() actually succeeded. malloc() returns NULL when the allocation can't be satisfied — a routine, expected outcome under memory pressure, not an exotic edge case. memset(out, 0, sizeof(xcb_get_image_reply_t)) then writes zero bytes starting at address 0, and the kernel delivers SIGSEGV. The subsequent memcpy() at line 307, which would have copied the actual pixel data into out, never even executes because the process is already dead.

Because len is derived from r2->length, the size of the allocation request tracks the size of the frame being captured. A streaming session handling larger frames, or a host already under memory pressure from other processes, is exactly the scenario where malloc() is most likely to fail — and that's precisely when this code had no safety net. The result for a service built on this bridge isn't information disclosure or privilege escalation; it's a hard crash of the entire streaming process at the worst possible moment: when the system is already resource-constrained. For a long-lived relay process, that converts transient memory pressure into a full denial of service that may need an external supervisor to restart.

The Fix

The fix inserts a single guard clause between the allocation and the first write, exactly where the missing check belonged:

xcb_get_image_reply_t *out =
    malloc(sizeof(xcb_get_image_reply_t) + len);
if (!out) {
    free(r2);
    plog("streamproxy: out-of-memory allocating xcb reply\n");
    return NULL;
}
memset(out, 0, sizeof(xcb_get_image_reply_t));

Three things happen in the new branch, each necessary:

1. free(r2) — r2 is the intermediate XCB reply object that was already allocated earlier in the function. Without freeing it on the failure path, the function would leak that memory on every out-of-memory event, compounding the original problem.
2. plog(...) — the failure is logged so operators can see that the process is under memory pressure instead of silently losing frames or, previously, crashing with no diagnostic trail.
3. return NULL — callers of xcb_get_image_reply() are expected to handle a NULL reply (it's the function's documented failure signal), so propagating NULL here lets the existing caller-side error handling do its job instead of letting the process die inside memset().

The fix doesn't change the allocation strategy, the sizing logic, or the function's signature — it closes the one gap between "allocation requested" and "allocation assumed to have succeeded."

Key Takeaways

• malloc(sizeof(xcb_get_image_reply_t) + len) where len scales with frame size is more likely to fail under load than a fixed small allocation — size-dependent allocations deserve a NULL check even if "it never fails in testing."
• A memset() or memcpy() immediately following malloc() is a strong signal to check for this exact bug pattern during review: the allocator's return value needs to be validated before it's dereferenced, not after.
• When a failure path appears late in a function, check what else was already allocated — here, r2 had to be explicitly freed in the new error branch to avoid trading a crash for a leak.
• Returning NULL on failure only helps if callers already check for it; confirm the calling convention before relying on an early return as the fix.

How Orbis AppSec Detected This

• Source: the XCB protocol reply (r2, from xcb_get_image_reply's own intermediate call) whose length field drives the size of a subsequent heap allocation.
• Sink: memset(out, 0, sizeof(xcb_get_image_reply_t)) and memcpy() writing through the pointer returned by malloc(sizeof(xcb_get_image_reply_t) + len).
• Missing control: no NULL check on the malloc() return value before it was dereferenced.
• CWE: CWE-476 (NULL Pointer Dereference).
• Fix: added an if (!out) guard that frees the intermediate reply, logs the out-of-memory condition, and returns NULL instead of writing through an invalid pointer.

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

This is a textbook CWE-476 finding made concrete: a size-dependent malloc() in xcb_get_image_reply() feeding straight into memset() and memcpy() with no check in between. The fix is minimal — four lines — but it changes the failure mode from "process segfaults mid-frame under memory pressure" to "function returns NULL and logs why." For code sitting in a streaming hot path, that difference is the gap between a logged, recoverable error and an unexplained crash in production.