From: Thomas Frohwein <tfrohwein@fastmail.com>
Subject: Mesa dlist_alloc apparent off-by-one
To: tech@openbsd.org
Cc: jsg@jsg.id.au
Date: Thu, 8 Feb 2024 21:13:24 -0500

Hi,

This is a follow-up on [1] previously brought up on ports@, now raising
on tech@ as I can narrow down on the source of the error. From my
reading of dlist.c the apparent cause of the problems is an
off-by-one when deciding whether to allocate a new block for the
display lists, as addressed in the diff below. 
ctx->ListState.CurrentPos is zero-based (see line 13200 in dlist.c).

With this diff applied, Mesa doesn't segfault anymore on multiple test
runs of Recoil (compare [2]) that would previously crash most of the
time. I can't speak to why the issue may not have been noticed on Linux.
On OpenBSD, I did some runs with a printf triggered when CurrentPost +
numNodes + contNodes exceeds BLOCK_SIZE, and this isn't reached in any
other graphically and shader intensive games in our ports tree like 0ad,
xonotic, or Northgard (the latter via hashlink). On the other hand,
Beyond All Reason running on Recoil reaches this all the time, likely
due to the heavy use of display lists/vertex arrays.

I would appreciate some eyes on this to verify if my analysis of the
logic seems correct. I'm happy to file this with upstream if this looks
like the right way to check the condition.

[1] https://marc.info/?l=openbsd-ports&m=170709003327657&q=mbox
[2] https://github.com/jasperla/openbsd-wip/tree/master/games/recoil-rts

Index: dlist.c
===================================================================
RCS file: /cvs/xenocara/lib/mesa/src/mesa/main/dlist.c,v
retrieving revision 1.15
diff -u -p -r1.15 dlist.c
--- dlist.c	2 Nov 2023 04:53:44 -0000	1.15
+++ dlist.c	9 Feb 2024 01:53:06 -0000
@@ -1220,7 +1220,7 @@ dlist_alloc(struct gl_context *ctx, OpCo
       ctx->ListState.CurrentPos++;
    }
 
-   if (ctx->ListState.CurrentPos + numNodes + contNodes > BLOCK_SIZE) {
+   if (ctx->ListState.CurrentPos + numNodes + contNodes >= BLOCK_SIZE) {
       /* This block is full.  Allocate a new block and chain to it */
       Node *newblock;
       Node *n = ctx->ListState.CurrentBlock + ctx->ListState.CurrentPos;