Jump to content
Search In
  • More options...
Find results that contain...
Find results in...
Maes

The shady segtextured overflow

Recommended Posts

I wonder if any other source port developer has ever stumbled upon this bug:

in the original r_segs.c there is this sneaky snippet of code:

	if (segtextured)
	{
	    // calculate texture offset
	    angle = (rw_centerangle + xtoviewangle[rw_x])>>ANGLETOFINESHIFT;
	    texturecolumn = rw_offset-FixedMul(finetangent[angle],rw_distance);
	    texturecolumn >>= FRACBITS;
which obviously computes the proper texture columns to display based on player's current position, the seg's angle vs the viewer and the normal of said angle, which then goes through Doom's funderful trigonometric LUT system (finetangent, in that case).

rw_centerangle is computed like this in some other place in r_segs.c:
rw_centerangle = ANG90 + viewangle - rw_normalangle;
Now, in Mocha Doom I occasionally got array index out of bounds exceptions at this point. They are hard to replicate and so as an initial hackish fix, I just caught and swallowed the exception.

Now, looking at the original code, the finetangent table contains only 4096 entries, which are supposed to represent only angles going from -90 to 90 degrees, and unlike the sine/cosine tables, there's no e.g. small overflow protection/overlap between finesine/finecosine, and valid indices are only 0..4095.

Looking at the first snippet of code again,
angle = (rw_centerangle + xtoviewangle[rw_x])>>ANGLETOFINESHIFT;
and the code before it, I realized that there's absolutely no guarantee that rw_centerangle + xtoviewangle[rw_x] will result in an angle such that shifting it will be a number contained between 0..4095 with no possibility of the slightest overflow. Sure, the way xtoviewangle is constructed should include only angles from -45 to 45 (I think), but the way rw_centralangle is constructed doesn't rule out that it will NOT result in a residual angle >180 degrees (in Doom's arithmetic terms, that means a hex value greater than 0x7FFFFFFF so that after shifting it won't exceed 0xFFF).
rw_centerangle = ANG90 + viewangle - rw_normalangle;
In fact, whenever I got overflows it was always for relatively small amounts, e.g. indices such as 4096, 4100 etc. which indicated that the above angle had been exceeded by a relatively small amount, plausible if you consider the whole fixed point arithmetic thing.

And even if I swallowed the exception, nothing weird appeared on screen. At some point I decided to "sanitize" the result by clipping it to its 31 LSB before applying the shift. No overflows, and no change in visuals either, so I accepted that as a definitive fix.

However, is this supposed to happen? In C/C++ you can happily go beyond the boundaries without even noticing it unless you are really searching for it, and in that particular occasion it would just result in a glitchy column at weird angles, or perhaps not even that.

I'd like to know if anyone else has noticed this, or it's something that only affects Doom implementations with strict bound checkings. Is it a known emulated overflow in choco/prboom etc.?

Share this post


Link to post
Maes said:

I wonder if any other source port developer has ever stumbled upon this bug:

I haven't personally heard of it, but it's not something that surprises me. The DOOM renderer has a lot of overflows and you will continue to find more without a doubt.

Maes said:

However, is this supposed to happen? In C/C++ you can happily go beyond the boundaries without even noticing it unless you are really searching for it, and in that particular occasion it would just result in a glitchy column at weird angles, or perhaps not even that.

I'd like to know if anyone else has noticed this, or it's something that only affects Doom implementations with strict bound checkings. Is it a known emulated overflow in choco/prboom etc.?

Supposed to? Almost certainly not. It's just a result of haphazard, hairbrained usage of fixed-size lookup tables as if they're infinite-domain functions. As you mentioned, it seems largely asymptomatic at runtime in an environment where overflow reads on arrays are tolerated. In the DOOM binary, finesine/finecosine immediately follows finetangent, so an overflow read by 10240 or fewer words will read some portion of that array. So I don't believe you'll find this is either widely known or currently accounted for.

Share this post


Link to post

Something I have noticed in the past but never bothered checking out is that in R_CheckPlane there is no check for max visplanes. There may be a possibility for overflow there as well.

Share this post


Link to post
4mer said:

Something I have noticed in the past but never bothered checking out is that in R_CheckPlane there is no check for max visplanes. There may be a possibility for overflow there as well.


Yeah I had caught that one during testing too. There were other "gems" such as the sprite sorting routine leaving dangling pointers (which didn't matter in

As for LUT usage...the fact that the "misses" always occur by a few elements seems to indicate that the original code had got the angle clipping formulas almost right, but not entirely bullet proof.

In fact, this bug is extremely hard to reproduce, as it may occur only at very particular view angles and with very particular player positions. Well, part of Mocha Doom's goal is to thorougly document what is going on in there....

Share this post


Link to post
4mer said:

Something I have noticed in the past but never bothered checking out is that in R_CheckPlane there is no check for max visplanes. There may be a possibility for overflow there as well.

This is actually the cause, I believe, of why some people reported crashing rather than a simple Visplane Overflow under extreme conditions. One of the checks (either VPO or No more Visplanes!) is checked after the fact, so before that time, you can get some bad memory access.

Share this post


Link to post
Wagi said:

This is actually the cause, I believe, of why some people reported crashing rather than a simple Visplane Overflow under extreme conditions. One of the checks (either VPO or No more Visplanes!) is checked after the fact, so before that time, you can get some bad memory access.

There are a lot of other overflows which can cause the crash before visplanes will, however. Such as openings and solidsegs - due to the areas of memory they overflow in, they tend to be instantly fatal, with a venetian blinds crash in vanilla.

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
×