From: Jonathan Gray <jsg@jsg.id.au>
Subject: Re: Mesa dlist_alloc apparent off-by-one
To: Thomas Frohwein <tfrohwein@fastmail.com>
Cc: tech@openbsd.org
Date: Fri, 9 Feb 2024 22:39:17 +1100

On Thu, Feb 08, 2024 at 09:13:24PM -0500, Thomas Frohwein wrote:
> 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.

looks right, ok jsg@

Opening a merge request upstream would be appreciated.

> 
> [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;
> 
>