aviralgarg05 commented on code in PR #3644:
URL: https://github.com/apache/nuttx-apps/pull/3644#discussion_r3597248175
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -114,12 +114,23 @@ static void r_map_plane(int y, int x1, int x2)
fixed_t length;
unsigned index;
-#ifdef CONFIG_GAMES_NXDOOM_RANGECHECK
- if (x2 < x1 || x1 < 0 || x2 >= viewwidth || y > viewheight)
+ /* y indexes cachedheight[]/cacheddistance[]/cachedxstep[]/cachedystep[]
+ * below, all sized SCREENHEIGHT - a y outside that range (observed on
+ * this port: y=255 against a 200-entry array, well past even
+ * viewheight) is an out-of-bounds array write, not just a "debug
+ * assertion". This used to be gated behind CONFIG_GAMES_NXDOOM_
+ * RANGECHECK and fatal (i_error(), which tears down the whole process
+ * on what vanilla Doom would just render as one glitched span) - both
+ * wrong: the memory-safety check must not be optional, and killing the
+ * entire game over one bad plane span is worse than just not drawing
+ * it. Skip the draw instead of touching memory or the process outside
+ * the buffers' real bounds.
+ */
+
+ if (x2 < x1 || x1 < 0 || x2 >= viewwidth || y < 0 || y >= SCREENHEIGHT)
{
- i_error("R_MapPlane: %i, %i at %i", x1, x2, y);
+ return;
Review Comment:
The bounds check itself was already there, just gated behind a debug-only
config and fatal when tripped,
so in a normal build it wasn't even compiled in, and the array write
happened uncontrolled. I made the check unconditional, but killing the whole
process over one glitched plane span felt worse than what vanilla DOOM does
(renders the glitch and moves on), so I made it skip the draw instead of
calling i_error(). Open to putting the i_error() back if you'd rather fail loud
than render a glitch, happy to hear which you'd prefer
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -57,12 +57,12 @@ planefunction_t ceilingfunc;
/* Here comes the obnoxious "visplane". */
-visplane_t visplanes[CONFIG_GAMES_NXDOOM_MAXVISPLANES];
+visplane_t *visplanes;
Review Comment:
will put it behind the same Kconfig switch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
