Posssible OOB read in R_DrawColumn()?

[imustafin]

While working on my Brie Doom I think I found a bug in the original DOOM 

 

We have R_DrawMaskedColumn:

void R_DrawMaskedColumn (column_t* column)
{
    int		topscreen;
    int 	bottomscreen;
    fixed_t	basetexturemid;
	
    basetexturemid = dc_texturemid;
	
    for ( ; column->topdelta != 0xff ; ) 
    {
	// calculate unclipped screen coordinates
	//  for post
	topscreen = sprtopscreen + spryscale*column->topdelta;
	bottomscreen = topscreen + spryscale*column->length;

	dc_yl = (topscreen+FRACUNIT-1)>>FRACBITS;
	dc_yh = (bottomscreen-1)>>FRACBITS;
		
	if (dc_yh >= mfloorclip[dc_x])
	    dc_yh = mfloorclip[dc_x]-1;
	if (dc_yl <= mceilingclip[dc_x])
	    dc_yl = mceilingclip[dc_x]+1;

	if (dc_yl <= dc_yh)
	{
	    dc_source = (byte *)column + 3;
	    dc_texturemid = basetexturemid - (column->topdelta<<FRACBITS);
	    // dc_source = (byte *)column + 3 - column->topdelta;

	    // Drawn by either R_DrawColumn
	    //  or (SHADOW) R_DrawFuzzColumn.
	    colfunc ();	
	}
	column = (column_t *)(  (byte *)column + column->length + 4);
    }
	
    dc_texturemid = basetexturemid;
}

At one point in the loop we have these values:

column bytes = {0x1b, 0x23, 0x45, 0x45, 0x45, 0x49, ...}
// so 
column->topdelta = 0x1b
column->length = 0x23 = decimal 35
  
// and other values
dc_texturemid = 0xffbe71af
dc_yl = 0x83
dc_yh = 0x8f
centery = 0x48

 

We get into this function from R_DrawVisSprite(vissprite_t* vis={patch=8, ...}, int x1=104, int x2=154). Thus the patch is from the lump number 561. The WAD I'm using is attached just in case (this is the Doom Shareware v1.9, right?)

 

So, the problem is that when we call colfunc() which points to R_DrawColumn():

void R_DrawColumn (void) 
{ 
    // ...

    // Determine scaling,
    //  which is the only mapping to be done.
    fracstep = dc_iscale; 
    frac = dc_texturemid + (dc_yl-centery)*fracstep; 

    // Inner loop that does the actual texture mapping,
    //  e.g. a DDA-lile scaling.
    // This is as fast as it gets.
    do 
    {
      // Re-map color indices from wall texture column
      //  using a lighting/special effects LUT.
      
      // The variable i is by me
      int i = (frac>>FRACBITS)&127;
      if (i == 127 && frac == -70) {
          printf("  TAKE 127 frac %d\n", frac);
      }
      *dest = dc_colormap[dc_source[i]]; // ORIGINAL: *dest = dc_colormap[dc_source[(frac>>FRACBITS)&127]];

      dest += SCREENWIDTH; 
      frac += fracstep;
	
    } while (count--); 
} 

We read from dc_source[127] which as I understand is not correct because dc_source is only 35 bytes long.

 

Am I right that this is a bug?

 

It seems that nothing crashes because of such reads but still...

 

How to reproduce:

Spoiler

I can reproduce this almost consistently with this code after starting a game on E1M1:

void DebugB() {
  mobj_t* mo = players[consoleplayer].mo;
  player_t* p = &players[consoleplayer];
  if (!mo) {
    printf("DebugB: player.mo is NULL\n");
  } else {
    mo->x = 79471682;
    mo->y = -231677363;
    mo->angle = 1098907648;
    mo->momx = 116642;
    mo->momy = 414988;
    mo->momz = 0;
    p->bob = 863093;
    p->deltaviewheight = 0;
    p->viewheight = 2686976;
    p->viewz = 2891133;
  }
}

I just call DebugB in G_BuildTiccmd when my special button is pressed.

 

P.S. Can this be related to https://github.com/AXDOOMER/mochadoom/blob/a1c6b24747ed21d3912608c9f6dc712dc57ce9c9/src/rr/AbstractThings.java#L736?

doom1.wad.zip

Edited June 23, 2021 by imustafin

[wesleyjohnson]

The original draw routines are known to have limitations.  Using them to draw things that do not meet the original assumptions leads to buggy behavior.

However, the routine that calls the column_draw could be blamed, or not having a column_draw specific for the usage could be blamed, or the wad could be blamed

for trying to draw with a small texture.

 

The  "&127" in the column draw supports tiling the texture over a large wall.  It assumes that the texture is 128 high.  The tiling does not work on textures that are not 128 high.

Trying to use the column draw on a smaller texture, and allowing the draw to wrap, will result in drawing garbage.  This is the result of not restricting the texture draw to an appropriately  small area.

 

I have rewritten those routines several times in DoomLegacy.

The whole issue is complicated and highly dependent on exactly how your port is going to deal with the odd sized draws.

 

I must refer you to the draw code in DoomLegacy and similar advanced ports.

In DoomLegacy, within the last year, I reworked the column draws to support DeepSea patches.  It also fixed some outstanding issues with the column draws being misused.

This required explicit support for the non-tiling draws in the texture draw function structure.

I cannot remember enough details to even describe it well.

 

[Sinshu]

When I was working on a C# port, I encountered a lot of random crashes due to OOB.
I remember that most of the crashes were in the software renderer, so some of them might be the thing that you pointed out.
To avoid crashes like this, I have added padding to patches to ensure that the 127th element in a column is always accessible.

[imustafin]

5 hours ago, wesleyjohnson said:

The original draw routines are known to have limitations.  Using them to draw things that do not meet the original assumptions leads to buggy behavior.

However, the routine that calls the column_draw could be blamed, or not having a column_draw specific for the usage could be blamed, or the wad could be blamed

for trying to draw with a small texture.

Right, but I believe I used the original code and the original wad here.

 

5 hours ago, wesleyjohnson said:

I must refer you to the draw code in DoomLegacy and similar advanced ports.

In DoomLegacy, within the last year, I reworked the column draws to support DeepSea patches.  It also fixed some outstanding issues with the column draws being misused.

This required explicit support for the non-tiling draws in the texture draw function structure.

I cannot remember enough details to even describe it well.

 

I'll take a look, thanks.

 

3 hours ago, Sinshu said:

When I was working on a C# port, I encountered a lot of random crashes due to OOB.
I remember that most of the crashes were in the software renderer, so some of them might be the thing that you pointed out.
To avoid crashes like this, I have added padding to patches to ensure that the 127th element in a column is always accessible.

Thanks. For now I've also decided just to skip reading if it is out of bounds https://github.com/imustafin/brie_doom/commit/888861bc76a03e5ad238eb0933a265c7b5c7a7ae#diff-f6ca101332ed4a11d3c6f249557b5b7f832f3c4fc86b77d69279bec5fd05eb9eL211-R219.

 

So, for now I will assume that this is a bug indeed. I will try to investigate this more

[SaladBadger]

Potential segment violations in cases like these are usually disguised by the zone allocator, which is why it generally doesn't crash. The patches are always in the zone heap, and unless in the exceptional circumstance that particular patch and that particular column is right at the end of the zone heap, it won't crash because in the eyes of the operating system, all the memory in the zone heap is fair game for your program.

 

As for the actual bug here, I'm going to assume it's a rounding error, since frac is coming up negative, and I don't really think there's any reason for it to start negative when drawing columns for sprites and masked walls. Doom has some problems with them, where you'll see things like the lefts and rights of textures peek in on the opposite side even on perfectly sized and aligned walls.