Skip to content

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

Merged
metux merged 1 commit into
release/25.1from
rfc/backport-25.1-region-double-free
Oct 2, 2026
Merged

metux merged 1 commit into
release/25.1from
rfc/backport-25.1-region-double-free

Conversation

@metux

@metux metux commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Backport-Ursprung

Dieser PR ist ein Backport. Quelle auf master:
GH-3779

Der Merge-Status des Originals steht in der Referenz oben.

Backport-Übersicht

Master-PR: #3779

Backport von #3779 auf release/25.1.

Was

RegionValidate() kopiert *badreg nach ri[0].reg, wodurch ri[0].reg.data und
badreg->data auf denselben Block zeigen. Ein spaeteres RECTALLOC_BAIL() kann
diesen Block per realloc() verschieben — ri[0].reg.data wird dann aktualisiert,
badreg->data nicht. Auf dem Bail-Pfad gibt xfreeData(&ri[i].reg) den Speicher
frei, und RegionBreak(badreg) gibt badreg->data ein zweites Mal frei: ein stale
Pointer auf bereits freigegebenen oder umallozierten Speicher. Doppel-Frei.

Fix: badreg->data = NULL; direkt vor return RegionBreak(badreg).

  • Datei: dix/region.c (eine Datei, +5 Zeilen)
  • Source-Commit auf master: e84a8065c4f33178bbc687fa0a67511fb9eb4165
  • Client-erreichbar: RegionValidate() laeuft auf Client-Regions (XFixes/XComposite)

Source-SHA — bitte beachten

Der in der Auftrags-Direktive genannte Wert e02f90995bfc ist nicht auf
origin/master; er liegt nur auf dem make-PR-Branch-Tip von #3779
(origin/pr/master-regionvalidate-...-2026-10-01_15-16-20). Bei rebase-Merge sind das
zwei verschiedene SHAs. Korrekt und auf master ist e84a8065c4. Die Patches sind
byteweise identisch (git diff e84a8065c4^ e84a8065c4 -- dix/region.c gegen den
Branch-Tip: leerer Diff), es ist also die Beschriftung, nicht der Inhalt.

Backport-Commits

Genau ein Commit, cherry-pickt mit -x. Kein Sammel-Commit, damit der Fix
einzeln entfernbar bleibt, falls er zurueckgenommen werden muss.

Trailer: Part-of: zeigt auf den xorg-Ursprung (MR 2281), nicht auf unseren PR.
Kein [PR #NNNN]-Marker — der gehoert in unser Incubation-Ledger, nicht in einen
Port eines xorg-Commits.

Branch

rfc/backport-25.1-region-double-free, direkt von origin/release/25.1
(c961c43) abgezweigt. Kontrolle: git rev-list --count origin/release/25.1..HEAD
= 1 (nur der Backport).

Bewusst nicht auf rfc/backport-25.1 — das ist der geteilte xorg/main-Inkubator.
Auf 25.1 liegt er 6 Commits vor release/25.1
(git rev-list --left-right --count origin/release/25.1...origin/rfc/backport-25.1
-> 2 6). Die Groesse unterscheidet sich pro Release, die Falle nicht: auf 25.2
sind es 32.

Build

meson setup -Dwerror=true -Dxephyr=true -Dxnest=true -Dxvfb=true -Dxorg=true,
dann ninja -k 0 ueber alle 710 Targets.

Genau ein Fehler, und er ist die eine dokumentierte vorbestehende Ausnahme:

os/Xtranssock.c:631 -Werror=format-truncation

Gegenprobe am unpatchten Tip 84636fd095 mit demselben Build-Dir: identische
Fehlermenge, identisches FAILED-Target (os/libxserver_os.a.p/transport.c.o).
Der Backport fuegt also nichts hinzu. Alles andere waere aus dem Backport
gekommen — es ist nichts.

Dashboard

Topic: task-backport-3779-regionvalidate-double-free-to-release-25-1
Batch-Topic: task-backport-batch-2026-10-01-3776-3777-3779-auf-release-25-2-25-1-25-0

Kein Merge durch die Flotte — release/* wird vom Maintainer von Hand gemergt,
Merge-Mode rebase.


Verknüpfung

Release merges are manual, by the maintainer.

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)
(cherry picked from commit e84a806)
@metux

metux commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

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

Review: pass

Content verified against the master commit rather than taken on faith.

Patch fidelity

The added lines are byte-identical to master #3779 (md5 of + lines: 8451cc23b416
on all three backports and on master). Only blob hash and hunk line numbers differ,
which is what a cherry-pick across a release branch looks like when it goes right.

Provenance

Exemplary, and worth calling out because it is easy to get wrong:

Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2281>
(cherry picked from commit 4ef1b4cd2690c6d8e4b029b02bf707cb04d0da24)
(cherry picked from commit bb02769445b1a516f33b4855977cc227f0542cb7)
(cherry picked from commit bc0760d9514fe5f2d9e4f5287ab0318d45c32736)
(cherry picked from commit e84a8065c4f33178bbc687fa0a67511fb9eb4165)

Part-of: points at the xorg merge request — the external form, which is the one
that lets a reviewer on the release branch find the original discussion. Not a link
back into our own PR. The chain terminates in e84a8065c4, the master commit, so the
backport is traceable all the way to upstream.

The fix, restated for the release branch

RegionValidate()'s error path frees the buffer that badreg->data still points at
and then passes badreg to RegionBreak(), which reads that pointer — a
double-free, client-reachable through RegionValidate on client-supplied regions
(XFixes/XComposite). Setting badreg->data = NULL before the return closes it.

Rule 3 — driver ABI

No impact. One file, dix/region.c; no struct layout change and no _X_EXPORTed
symbol touched (0 occurrences in the diff). RegionValidate() operates on
RegionPtr, allocated by the server, and is not embedded in any driver-visible
structure.

Rule 2 — backport assessment

Already the backport, and it is the right one: memory safety, client-reachable. No
further propagation needed beyond the three release lines already covered.

CI for this PR

17 success, 2 skipped, 0 failures — green. Note the small
matrix: release/25.1 has fewer applicable lanes than master.

@metux metux added the bot-review-passed Automated bot review found no blocking issues label Oct 2, 2026
@metux

metux commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Verification basis

Nachprüfbar statt behauptet. Alle Werte gemessen, nicht aus dem PR-Body abgelesen.

master source commit : e84a8065c4f33178bbc687fa0a67511fb9eb4165
local origin/master  : d8cee7e08b
PR base (GitHub)     : c961c43cece8f171541848a7ff6be850363195c1
local origin/release/25.1 : c961c43cece8f171541848a7ff6be850363195c1
base comparison      : IDENTISCH  (lokal == GitHub)
source on master     : JA   (git merge-base --is-ancestor e84a8065c4 origin/master)
backport commit      : 9dfb86c2ea8e46c3c5839ac4008763a03f897044
commits over base    : 1  → 9dfb86c2ea,  Datei dix/region.c, +5
sign-offs            : 1  → Jeremy Huddleston Sequoia <jeremyhu@apple.com>

Warum local origin/master hier nicht der heutige Wert ist

Das war der Stand im Arbeits-Clone beim Cherry-Pick. Inzwischen ist er
weitergezogen — in den xserver-Clones des Workspace stehen zwölf verschiedene
Stände von origin/master, weil jedes Ship zu einem anderen Zeitpunkt gefetcht
hat. Wer das nachprüft, holt den Ref neu und bekommt einen anderen SHA. Das ist
erwartet und kein Widerspruch: die Prüfung, die zählt, ist
git merge-base --is-ancestor, und die ist ref-unabhängig, sobald der Ref frisch ist.

Ein stale Ref liefert hier übrigens kein „vielleicht", sondern ein falsches
Nein
: in drei von vier gemessenen Clones wurde derselbe Quell-SHA korrekt
verworfen, weil deren origin/master zu alt war.

Warum commits over base = 1 und nicht 0

Die Prüfung git rev-list --count origin/release/<ziel>..HEAD muss 0 sein —
und zwar vor dem Cherry-Pick. Nach dem Push steht dort der eine
Backport-Commit, und genau der ist oben benannt. Ein Wert über 1 hieße Fremdinhalt.

Der Punkt, der eine Wiederholung unwahrscheinlich macht

Der Quell-Commit stammt aus gh pr view 3779 --json mergeCommit, nicht aus dem
PR-Branch-Tip. Der ursprünglich notierte Wert e02f90995bfc liegt nicht auf
origin/master, sondern nur auf
origin/pr/master-regionvalidate-…-2026-10-01_15-16-20 — bei rebase-Merge hat
der make-PR-Branch-Tip eine eigene SHA. Ein Cherry-Pick darauf liefert
bad object.

Gegenprobe, die „falsche Beschriftung" von „falscher Inhalt" trennt: die
hinzugefügten Zeilen dieses Backports sind byteweise identisch mit denen des
Quell-Commits.

git diff e84a8065c4^ e84a8065c4 -- dix/region.c   vs.   Diff dieses Backports

Merge-Mode wäre rebase. Kein bot-review-passed angefordert — release/* wird
vom Maintainer von Hand gemergt.

Topic: task-backport-3779-regionvalidate-double-free-to-release-25-1
Batch: task-backport-batch-2026-10-01-3776-3777-3779-auf-release-25-2-25-1-25-0

@metux

metux commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Korrektur zur Beschriftung im Verifikationsblock: die Zeile PR base (GitHub) ist zu GitHub branch tip umzubenennen.

Gemessen wurde gh api repos/X11Libre/xserver/git/ref/heads/<branch> -q '.object.sha' — der Branch-Tip auf GitHub, nicht der PR-seitige Base-OID (baseRefOid liefert im gh pr view --json leer). Das ist auch die richtigere Groesse: sie ist das, wogegen GitHub real merged, und sie aendert sich nicht, wenn jemand die PR-Basis verschiebt.

Bei diesem PR fallen beide zusammen — die Basis ist unveraendert c961c43cece8 —, also kein inhaltlicher Fehler. Die Beschriftung said aber etwas anderes als der Wert, und genau das will dieser Block vermeiden: nachrechnen, nicht vertrauen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-review-passed Automated bot review found no blocking issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants