From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Mesa dlist_alloc apparent off-by-one
To: Thomas Frohwein <tfrohwein@fastmail.com>
Cc: tech@openbsd.org, jsg@jsg.id.au
Date: Fri, 9 Feb 2024 11:08:49 +0100

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

Diff makes sense to me, ok stsp@

This is an easy mistake to make. Linux won't flag small buffer
overwrites as agressively as OpenBSD does it. They might be
seeing other side effects of this or nothing at all.
 
> 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;
> 
>