(master) randr: free/terminate resource structs on AddResource() OOM failure

[metux]

Five constructors in Xext/randr/ leaked or permanently pinned their struct
when AddResource()'s internal calloc() failed (memory pressure):

• ProcRRCreateLease() (rrlease.c): links 'lease' into scr_priv->leases
  before AddResource(). RRLeaseDestroyResource() (the delete callback
  AddResource() invokes on a successfully-started-but-failed
  registration) only clears lease->id -- it doesn't unlink or free the
  lease. Without terminating it explicitly, the lease stays on
  scr_priv->leases forever, permanently marking its outputs/crtcs as leased
  (RROutputIsLeased()/RRCrtcIsLeased()) with no XID for the client to ever
  free it by. Fixed by calling RRTerminateLease(lease), mirroring the
  WriteFdToClient() failure branch a few lines below, which already does
  this correctly.

• RRModeGet() (rrmode.c), RRCrtcCreate() (rrcrtc.c), RROutputCreate()
  (rroutput.c), RRProviderCreate() (rrprovider.c): all four 'return NULL'
  directly on AddResource() failure without freeing the struct they just
  calloc'd. None of the four are linked into their owning screen-private
  array/field yet at that point (verified: each is only stored into
  pScrPriv->{modes,crtcs,outputs,provider} a few lines after the
  AddResource() check), so a plain free() is safe. rrmode.c already freed
  its secondary 'newModes' array allocation on this path but forgot 'mode'
  itself.

All five are narrow (OOM-gated) but real leaks/dangling-reference bugs, not
crashes -- bundled as one fix since they're the same missing-cleanup shape
in the same subsystem.

Found via a fleet-directed alloc-fail/UAF sweep of Xext/, not from a live
crash report.

Signed-off-by: Enrico Weigelt, metux IT consult info@metux.net

[metux]

🤖 Automated review — generated by Claude Code (ship: opus-review-3273) on behalf of @metux. Not a human review.

Independent review — verdict: changes requested (blocking)

Reviewed from scratch against master (dix/resource.c, Xext/randr/*), not
trusting the PR description. 1 of the 5 sub-fixes is correct; the other 4
introduce a double-free (heap corruption) on the exact OOM path they claim to
fix. The base code for those 4 was already correct.

Root cause: what AddResource() actually does on failure

The PR description states the delete callback is one "AddResource() would
invoke on a successfully-registered resource." That is the load-bearing
misconception. AddResource() has exactly one FALSE-return path
(dix/resource.c:833-837): its internal calloc(sizeof(ResourceRec)) fails,
and before returning FALSE it calls the resource type's deleteFunc on the
value:

ResourcePtr res = calloc(1, sizeof(ResourceRec));
if (!res) {
    (*resourceTypes[type & TypeMask].deleteFunc)(value, id);  // <-- runs the destroy cb
    return FALSE;
}

So whenever !AddResource(id, T, value) is true, type T's destroy callback
has already run on value. Whether extra cleanup is needed depends entirely
on what that callback does — and it differs per type. That's the whole review.

Per-file verdict

1. rrlease.c ProcRRCreateLease() — CORRECT ✓
RRLeaseDestroyResource() (rrlease.c:194) only does lease->id = None; it does
not unlink or free. So after AddResource() OOM, the lease is still on
scr_priv->leases and still allocated — permanently pinning its crtcs/outputs
as leased (RRCrtcIsLeased/RROutputIsLeased) with no live XID. Adding
RRTerminateLease(lease) is the right fix and correctly mirrors the
WriteFdToClient() failure branch immediately below (both hit the lease in
RRLeaseCreating state, on the list, lease->id == stuff->lid). Verified the
downstream chain: RRTerminateLease → driver hook → xf86CrtcLeaseTerminated
→ RRLeaseTerminated() (xorg_list_del, and FreeResource guarded by
lease->id != None — already cleared to None by the destroy cb, so it's
correctly skipped; FreeResource on an unregistered XID is a no-op anyway) →
RRLeaseFree() (frees). This one is a real, correctly-fixed bug.

2. rrmode.c RRModeCreate() — DOUBLE FREE ✗ (blocking)
RRModeDestroyResource (rrmode.c:257) → RRModeDestroy() → --mode->refcnt
(1→0, the ++ to 2 is on the success path after AddResource) → not >0 →
free(mode) (rrmode.c:253). So AddResource() OOM already frees mode.
The added free(mode) is a second free of the same pointer. The base
(free(newModes); return NULL;) was correct w.r.t. mode.

3. rrcrtc.c RRCrtcCreate() — DOUBLE FREE ✗ (blocking)
RRCrtcDestroyResource (rrcrtc.c:892) ends in free(crtc) (rrcrtc.c:930),
unconditionally (no refcnt). crtc->pScreen is set before AddResource, so the
callback's cleanup body runs and then frees crtc. The added free(crtc) is a
double free. Base (return NULL;) was correct.

4. rroutput.c RROutputCreate() — DOUBLE FREE ✗ (blocking)
RROutputDestroyResource ends in free(output), unconditionally. Added
free(output) is a double free. Base was correct.

5. rrprovider.c RRProviderCreate() — DOUBLE FREE ✗ (blocking)
RRProviderDestroyResource (rrprovider.c:423) sets pScrPriv->provider = NULL
and free(provider), unconditionally. Added free(provider) is a double free.
Base was correct.

For 2–5 the PR's "struct not stored in the pScrPriv array yet, so free() is
safe" reasoning is true but irrelevant: the leak it's guarding against doesn't
exist, because the destroy callback invoked inside AddResource already freed
the struct. Net effect of these four hunks is client-triggerable-under-memory-
pressure heap corruption — strictly worse than the (non-existent) leaks they
target.

Recommended change

• Keep only the rrlease.c hunk. Ideally split it into its own PR so the
  one good fix isn't blocked by the rest.
• Drop the free() additions in rrmode.c, rrcrtc.c, rroutput.c,
  rrprovider.c entirely — revert those four to their base form.

Secondary (pre-existing, not introduced by this PR — optional follow-up)

rrmode.c RRModeCreate() on the AddResource-fail branch does
free(newModes) while the file-static modes was not updated to
newModes. In the num_modes > 0 case newModes = reallocarray(modes, …) may
have moved/freed the old block, so after free(newModes) the static modes
dangles → next RRModeCreate reallocs a freed pointer. This exists on master
independent of this PR; flagging for a separate fix, not a condition on this one.

ABI impact

None. All changes are inside function bodies in .c files — no public
struct layout change, no _X_EXPORT'd signature/semantics change. The nvidia
blob and the in-tree xf86-* drivers are unaffected.

Backport-worthiness

The only correct piece (rrlease.c) is a narrow, OOM-gated
pinned-lease-state + leak — low severity (requires AddResource's internal
calloc to fail at exactly that point). Borderline; not compelling — fine
to backport for completeness once it lands on master, likely release/25.2
(then release/25.1 / release/25.0 if the code matches — confirm per branch),
but severity alone doesn't demand it. The other four hunks must not be
backported — they are bugs, not fixes.