(master) RegionValidate: Fix double free of badreg->data on the error path

[metux]

RegionValidate() copies *badreg into ri[0].reg, so ri[0].reg.data aliases badreg->data. A later
RECTALLOC_BAIL() can realloc() that block and move it, updating ri[0].reg.data but not badreg->data. On
the bail path, freeing ri[0].reg.data and then calling RegionBreak(badreg) frees badreg->data a second
time, which by then is a stale pointer into memory already freed or reallocated to something else.

Signed-off-by: Jeremy Huddleston Sequoia jeremyhu@apple.com
Part-of: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2281
(cherry picked from commit 4ef1b4c)
(cherry picked from commit bb02769445b1a516f33b4855977cc227f0542cb7)
(cherry picked from commit bc0760d)

---

Release merges are manual, by the maintainer.

[metux]

🤖 Automated review — generated by Starfleet ship Voyager (model: heavy-model) on behalf of @metux. Not a human review.

Review: passed — backport candidate (memory safety)

Genuine use-after-free / double-free on the error path. I verified the mechanism against
the tree rather than taking the commit message at face value, because the obvious
objection to this patch is that the added line looks redundant.

Why it looks redundant, and why it is not

The patch adds badreg->data = NULL; immediately before return RegionBreak(badreg);.
RegionBreak() ends with pReg->data = &RegionBrokenData;, so the assignment appears to
be overwritten one line later and therefore pointless. It is not.

The reason is xfreeData() at the top of the same file:

static inline void xfreeData(RegionPtr reg) {
    if (reg && reg->data && reg->data->size &&          /* <-- dereferences data */
        reg->data != &RegionBrokenData &&
        reg->data != &RegionEmptyData)
            free(reg->data);
}

RegionBreak() calls xfreeData(pReg) first. Without the new line,
xfreeData() reaches a badreg->data that is already dangling and evaluates
reg->data->size on freed memory — the use-after-free — and can then free() it a second
time. With the new line, xfreeData() short-circuits on !reg->data and never touches
the pointer. So the assignment is load-bearing: it protects the dereference inside the
callee, it is not dead code.

It matches the function's own established idiom

The same function already does exactly this a few lines earlier, on the numRects == 1
path:

xfreeData(badreg);
badreg->data = (RegDataPtr) NULL;

So the patch is consistent with how this function already handles "data freed, pointer
no longer valid". That is the strongest available corroboration short of reproducing the
crash.

Aliasing premise

The commit message states that ri[0].reg.data aliased badreg->data before
RegionRectAlloc() could reallocate it. The bail path frees every ri[i].reg before
calling RegionBreak(), and badreg->data is not yet overwritten with ri[0].reg on
that path (the *badreg = ri[0].reg happens only on the success path), so badreg->data
at the point of RegionBreak() is the pre-existing pointer that the ri[] frees and the
in-place reallocations may already have invalidated. The premise holds.

Backport: yes, all release lines

Backport-worthy — use-after-free and double-free, reachable through region validation.
Likely applies to release/25.2, release/25.1, release/25.0; applicability to be
confirmed per branch by the maintainer.

[metux]

Backport-Übersicht — Merge-Status live

Die Referenzen stehen als Task-Liste, damit GitHub jede beim Rendern zu einem
Eintrag mit Titel und aktuellem Status aufklappt. Es ist ausdrücklich keine
eigene Status-Spalte gepflegt: die altert per Definition.

• (release/25.2) RegionValidate: Fix double free of badreg->data on the error path #3794
• (release/25.1) RegionValidate: Fix double free of badreg->data on the error path #3799
• (release/25.0) RegionValidate: Fix double free of badreg->data on the error path #3805

Auflösung: GH- allein rendert nur als Kurzlink. Erst in einer Liste klappt
GitHub Titel und State aus (GitHub-Doku, "Autolinked references and URLs").