aviralgarg05 commented on code in PR #3644:
URL: https://github.com/apache/nuttx-apps/pull/3644#discussion_r3644882455
##########
games/NXDoom/src/doom/r_bsp.c:
##########
@@ -78,7 +78,7 @@ line_t *linedef;
sector_t *frontsector;
sector_t *backsector;
-drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS];
+drawseg_t *drawsegs;
Review Comment:
You're right to question this. I needed an opt-in way to move the large
renderer scratch buffers to the PSRAM-backed heap on this target, but making
that the default was too broad. The final version keeps static storage as the
default and enables heap allocation only with
`CONFIG_GAMES_NXDOOM_HEAP_BUFFERS=y`.
##########
games/NXDoom/src/doom/r_bsp.h:
##########
@@ -52,7 +52,7 @@ extern boolean markceiling;
extern boolean skymap;
-extern drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS];
+extern drawseg_t *drawsegs;
Review Comment:
Same correction here: this remains a static declaration by default. It
changes to heap-backed storage only when
`CONFIG_GAMES_NXDOOM_HEAP_BUFFERS=y` is selected.
##########
games/NXDoom/src/doom/r_main.c:
##########
@@ -685,6 +685,24 @@ fixed_t r_scale_from_global_angle(angle_t visangle)
void r_set_view_size(int blocks, int detail)
{
+ /* screenblocks is only ever meant to hold 3..11 (set that way by the
+ * options menu and by the config default of 9). The renderer's view
+ * geometry math divides by values derived from it - notably
+ * pspriteiscale = FRACUNIT * SCREENWIDTH / viewwidth in
+ * r_execute_set_view_size() - so a 0 or otherwise out-of-range value
+ * turns into a divide-by-zero hardware exception (EXCCAUSE=6), which on
+ * this flat-memory build takes the whole board down rather than just
+ * this task. Clamp defensively so a bad/missing config value degrades
+ * to the default screen size instead of a system crash.
+ */
+
+ if (blocks < 3 || blocks > 11)
+ {
+ printf("r_set_view_size: screenblocks=%d out of range, using 10\n",
+ blocks);
+ blocks = 10;
+ }
Review Comment:
Yes. A configuration file interrupted during writing left `screenblocks`
without a value, and the old path allowed an invalid value into the view-size
calculation. I no longer have the original exception log, so I have said that
plainly in the PR. I reran the malformed-config case on the signed tree,
recorded the fixture hash and outcome, and the game retained the safe
default, reached gameplay, and closed cleanly. `screenblocks` is now
independently bounded to `[3, 11]`.
##########
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:
Same issue as the BSP buffers. Static storage is restored as the default
here; heap placement is available only through the explicit
`CONFIG_GAMES_NXDOOM_HEAP_BUFFERS` option.
##########
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:
Agreed with the follow-up: the final code does not silently skip the span or
terminate the process. It bounds the row to `viewheight - 1`, guards each
`spanstart[]` access, checks visplane capacity before writing, and then
renders the bounded span. That keeps the visible glitch closer to vanilla
DOOM while preventing the out-of-bounds access.
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -195,7 +206,48 @@ static void r_make_spans(int x, int t1, int b1, int t2,
int b2)
void r_init_planes(void)
{
- /* Doh! */
+ /* These renderer scratch buffers are sized for a comfortable margin
+ * above vanilla DOOM's original limits and would blow the platform's
+ * internal DRAM budget as static arrays, so they're heap-allocated
+ * instead (comes out of the PSRAM-backed user heap on this target).
+ */
+
+ visplanes = malloc(sizeof(visplane_t) * CONFIG_GAMES_NXDOOM_MAXVISPLANES);
+ openings = malloc(sizeof(short) * MAXOPENINGS);
+ drawsegs = malloc(sizeof(drawseg_t) * CONFIG_GAMES_NXDOOM_MAXDRAWSEGS);
+ vissprites = malloc(sizeof(vissprite_t) *
+ CONFIG_GAMES_NXDOOM_MAXVISSPRITES);
+
+ if (visplanes == NULL || openings == NULL || drawsegs == NULL ||
+ vissprites == NULL)
+ {
+ i_error("r_init_planes: failed to allocate renderer buffers");
+ }
Review Comment:
Agreed. These buffers are static again by default. Heap allocation is
retained only as an explicit `CONFIG_GAMES_NXDOOM_HEAP_BUFFERS=y` option for
targets where the large static footprint is a problem.
##########
games/NXDoom/src/i_main.c:
##########
@@ -57,17 +57,28 @@ void d_doom_main(void);
int main(int argc, char **argv)
{
- /* save arguments */
+ /* save arguments
+ *
+ * +1 and an explicit NULL terminator: argv is conventionally
+ * NULL-terminated at argv[argc] (this is what the OS/exec path
+ * guarantees for the `argv` parameter above), and some of this
+ * codebase's own argument handling was written assuming that holds
+ * for myargv too - an under-sized allocation here leaves myargv[argc]
+ * pointing at whatever the allocator happens to return next, which
+ * only reads as "probably zero" by chance depending on heap layout.
+ */
myargc = argc;
- myargv = malloc(argc * sizeof(char *));
+ myargv = malloc((argc + 1) * sizeof(char *));
assert(myargv != NULL);
for (int i = 0; i < argc; i++)
Review Comment:
I re-audited the complete source and could not find any consumer that reads
`myargv[myargc]`, and I could not substantiate my earlier claim that the
observed crash came from a missing sentinel. I removed this change. Sorry for
presenting that root cause as established earlier; it was not supported by
the code or retained evidence.
##########
games/NXDoom/src/m_config.c:
##########
@@ -2098,7 +2098,9 @@ static void load_default_collection(default_collection_t
*collection)
while (!feof(f))
{
- if (fscanf(f, "%79s %99[^\n]\n", defname, strparm) != 2)
+ strparm[0] = '\0';
Review Comment:
This came from the board failure caused by a config file left incomplete
during writing: `screenblocks` had no value. The parser now reads one
physical
line with `fgets()`, rejects incomplete or overlong lines, checks integer
conversion, and retains the compiled default when conversion fails. I added
the malformed fixture hash and the successful gameplay/close result to the
Testing section; the original exception log was not retained.
--
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]
