linguini1 commented on code in PR #3644:
URL: https://github.com/apache/nuttx-apps/pull/3644#discussion_r3736738603
##########
games/NXDoom/src/doom/d_main.c:
##########
@@ -1294,6 +1294,12 @@ void d_doomloop(void)
while (1)
{
+ /* Safe point (outside any framebuffer/heap access) for nxstore's
Review Comment:
Remove mention of NXstore. That is an unrelated application.
In fact, comment at all isn't necessary here since `i_poll_quit_signal` is
pretty clear.
##########
games/NXDoom/src/d_iwad.c:
##########
@@ -271,6 +271,16 @@ static void buld_iwad_dir_list(void)
add_iwad_dir(m_dir_name(myargv[0]));
+ /* Add the board's configured DOOM data directory. Kconfig documents
+ * CONFIG_GAMES_NXDOOM_PREFDIR as "Directory where DOOM WAD files are
+ * stored", but until now it was only used for the config/save file
+ * location -- nothing actually searched it for IWADs, forcing every
+ * launch to rely on the current directory or DOOMWADDIR/DOOMWADPATH
+ * being set by hand first.
+ */
Review Comment:
Delete this comment. It is documenting old behaviour.
##########
games/NXDoom/src/i_system.c:
##########
@@ -320,6 +332,71 @@ void i_quit(void)
exit(0);
}
+/* i_quit_signal_handler
+ *
+ * A supervisor process (nxstore) has no reachable in-game quit path to
+ * drive (no keyboard/touch input is wired up here) - it can only ask
+ * from the outside, via SIGTERM. This handler does the one thing a
+ * signal handler is safe to do: set a flag. It must NOT call i_quit()
+ * (or anything it does - munmap, fclose, exit()'s atexit chain) directly,
+ * because a signal can land at literally any point in this process's own
+ * execution, including mid-malloc()/mid-blit - exactly the same "unsafe
+ * mid-operation teardown" risk as being force-killed from outside, just
+ * moved from another task's context into this one. i_poll_quit_signal()
+ * defers the real work to a known-safe boundary instead.
+ */
+
+static void i_quit_signal_handler(int signo)
+{
+ (void)signo;
+ quit_requested = 1;
+}
+
+void i_install_quit_signal(void)
+{
+ struct sigaction sa;
+
+ /* This board's flat, single address-space build can relaunch NXDoom
+ * (via nxpkg) as a fresh loadable ELF module - a proper posix_spawn of
+ * a new module load, which gets its own zeroed .bss/re-initialized
+ * .data - but GAMES_NXDOOM is a tristate Kconfig symbol and can also
+ * be built in as a true built-in (MODULE=n) sharing this process's
+ * address space across "launches" with no fresh .bss at all. Reset
+ * both pieces of state a stale second invocation could see: a leaked
+ * quit_requested flag would call i_quit() again before the game even
+ * starts, and a leaked exit_funcs chain would run every previous
+ * invocation's exit handlers a second time (double free()s, etc.) in
+ * addition to this invocation's own.
+ */
Review Comment:
Delete this.
##########
games/NXDoom/src/i_system.c:
##########
@@ -320,6 +332,71 @@ void i_quit(void)
exit(0);
}
+/* i_quit_signal_handler
+ *
+ * A supervisor process (nxstore) has no reachable in-game quit path to
+ * drive (no keyboard/touch input is wired up here) - it can only ask
+ * from the outside, via SIGTERM. This handler does the one thing a
+ * signal handler is safe to do: set a flag. It must NOT call i_quit()
+ * (or anything it does - munmap, fclose, exit()'s atexit chain) directly,
+ * because a signal can land at literally any point in this process's own
+ * execution, including mid-malloc()/mid-blit - exactly the same "unsafe
+ * mid-operation teardown" risk as being force-killed from outside, just
+ * moved from another task's context into this one. i_poll_quit_signal()
+ * defers the real work to a known-safe boundary instead.
+ */
+
+static void i_quit_signal_handler(int signo)
+{
+ (void)signo;
+ quit_requested = 1;
+}
+
+void i_install_quit_signal(void)
+{
+ struct sigaction sa;
+
+ /* This board's flat, single address-space build can relaunch NXDoom
+ * (via nxpkg) as a fresh loadable ELF module - a proper posix_spawn of
+ * a new module load, which gets its own zeroed .bss/re-initialized
+ * .data - but GAMES_NXDOOM is a tristate Kconfig symbol and can also
+ * be built in as a true built-in (MODULE=n) sharing this process's
+ * address space across "launches" with no fresh .bss at all. Reset
+ * both pieces of state a stale second invocation could see: a leaked
+ * quit_requested flag would call i_quit() again before the game even
+ * starts, and a leaked exit_funcs chain would run every previous
+ * invocation's exit handlers a second time (double free()s, etc.) in
+ * addition to this invocation's own.
+ */
+
+ quit_requested = 0;
+ exit_funcs = NULL;
+
+ memset(&sa, 0, sizeof(sa));
+ sa.sa_handler = i_quit_signal_handler;
+
+ if (sigaction(SIGTERM, &sa, NULL) < 0)
+ {
+ /* Not fatal - the game still runs, it just can't be asked to
+ * close cleanly from the outside (nxstore's close button will
+ * have nothing to signal into). Surface it rather than silently
+ * leaving close non-functional with no trace of why.
+ */
+
+ syslog(LOG_WARNING,
+ "nxdoom: failed to install SIGTERM handler: %d\n", errno);
+ }
+}
+
+void i_poll_quit_signal(void)
+{
+ if (quit_requested)
+ {
+ syslog(LOG_WARNING, "nxdoom: quit signal seen, calling i_quit\n");
Review Comment:
Why use syslog when the rest of the application prints to console?
##########
games/NXDoom/src/m_config.c:
##########
@@ -2017,7 +2019,11 @@ static void set_variable(default_t *def, const char
*value)
* file (save the old value in untranslated)
*/
- intparm = parse_int_parameter(value);
+ if (!parse_int_parameter(value, &intparm))
Review Comment:
ditto
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -195,7 +269,70 @@ static void r_make_spans(int x, int t1, int b1, int t2,
int b2)
void r_init_planes(void)
{
- /* Doh! */
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+ /* These renderer scratch buffers are sized for a comfortable margin
+ * above vanilla DOOM's original limits and, on a DRAM-constrained
+ * target, blow the internal DRAM budget as static arrays - opt-in
+ * heap allocation instead (comes out of the PSRAM-backed user heap
+ * on this target) via CONFIG_GAMES_NXDOOM_HEAP_BUFFERS.
+ */
+
+ 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() doesn't necessarily terminate the whole board on this
+ * flat, single address-space build (see the comment below on
+ * relaunch) - free whatever partially succeeded so a failed
+ * allocation attempt doesn't leak across a subsequent relaunch.
+ */
+
+ free(visplanes);
+ free(openings);
+ free(drawsegs);
+ free(vissprites);
+ visplanes = NULL;
+ openings = NULL;
+ drawsegs = NULL;
+ vissprites = NULL;
Review Comment:
You cannot call `free` on a NULL pointer. That is UB. So you will have to
only free non-null pointers here. I recommend performing that check in
`r_shutdown_planes` and just calling that function here.
##########
games/NXDoom/src/i_system.c:
##########
@@ -320,6 +332,71 @@ void i_quit(void)
exit(0);
}
+/* i_quit_signal_handler
+ *
+ * A supervisor process (nxstore) has no reachable in-game quit path to
+ * drive (no keyboard/touch input is wired up here) - it can only ask
+ * from the outside, via SIGTERM. This handler does the one thing a
+ * signal handler is safe to do: set a flag. It must NOT call i_quit()
+ * (or anything it does - munmap, fclose, exit()'s atexit chain) directly,
+ * because a signal can land at literally any point in this process's own
+ * execution, including mid-malloc()/mid-blit - exactly the same "unsafe
+ * mid-operation teardown" risk as being force-killed from outside, just
+ * moved from another task's context into this one. i_poll_quit_signal()
+ * defers the real work to a known-safe boundary instead.
Review Comment:
This comment does not need to explain how signal handlers work. I understand
you're using AI assistance for these PRs, but please review the comment output
to see if the information is actually relevant.
For these functions, it would be preferred to use the NuttX style function
comments.
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -195,7 +269,70 @@ static void r_make_spans(int x, int t1, int b1, int t2,
int b2)
void r_init_planes(void)
{
- /* Doh! */
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+ /* These renderer scratch buffers are sized for a comfortable margin
+ * above vanilla DOOM's original limits and, on a DRAM-constrained
+ * target, blow the internal DRAM budget as static arrays - opt-in
+ * heap allocation instead (comes out of the PSRAM-backed user heap
+ * on this target) via CONFIG_GAMES_NXDOOM_HEAP_BUFFERS.
+ */
+
+ 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() doesn't necessarily terminate the whole board on this
+ * flat, single address-space build (see the comment below on
+ * relaunch) - free whatever partially succeeded so a failed
+ * allocation attempt doesn't leak across a subsequent relaunch.
+ */
+
+ free(visplanes);
+ free(openings);
+ free(drawsegs);
+ free(vissprites);
+ visplanes = NULL;
+ openings = NULL;
+ drawsegs = NULL;
+ vissprites = NULL;
+
+ i_error("r_init_planes: failed to allocate renderer buffers");
+ }
+
+ /* i_quit() can be followed by another r_init_planes() call within the
+ * same boot (relaunching the game via nxpkg on this flat, single
+ * address-space build), so these heap buffers must be freed on exit
+ * or every relaunch leaks the previous allocation permanently.
+ */
Review Comment:
Remove mention of nxpkg.
##########
games/NXDoom/src/i_system.h:
##########
@@ -73,6 +73,23 @@ ticcmd_t *i_base_ticcmd(void);
void i_quit(void) NORETURN;
+/* Installs a SIGTERM handler that only sets a flag (async-signal-safe) -
+ * the actual i_quit() cleanup (unmapping the framebuffer, closing fds)
+ * runs later from i_poll_quit_signal(), called once per tic from a known
+ * safe point in the main loop rather than from the signal handler itself,
+ * so a supervisor process (nxstore) requesting an exit can never land in
+ * the middle of a frame's worth of direct framebuffer/heap access.
+ */
+
+void i_install_quit_signal(void);
+
+/* Checks the flag set by the SIGTERM handler and calls i_quit() if it's
+ * set. Must only be called from a safe point in the main loop - see
+ * i_install_quit_signal().
+ */
Review Comment:
ditto
##########
games/NXDoom/src/i_main.c:
##########
@@ -57,6 +57,15 @@ void d_doom_main(void);
int main(int argc, char **argv)
{
+ /* Lets nxstore (or any other supervisor) ask this process to exit
+ * cleanly via SIGTERM instead of the only other option being a forced
+ * task_delete() from outside - see i_system.h/i_system.c for why that
+ * matters on this board (a forced kill mid framebuffer/heap access was
+ * observed to hang the whole system, not just this task).
+ */
Review Comment:
Delete.
##########
games/NXDoom/src/i_system.h:
##########
@@ -73,6 +73,23 @@ ticcmd_t *i_base_ticcmd(void);
void i_quit(void) NORETURN;
+/* Installs a SIGTERM handler that only sets a flag (async-signal-safe) -
+ * the actual i_quit() cleanup (unmapping the framebuffer, closing fds)
+ * runs later from i_poll_quit_signal(), called once per tic from a known
+ * safe point in the main loop rather than from the signal handler itself,
+ * so a supervisor process (nxstore) requesting an exit can never land in
+ * the middle of a frame's worth of direct framebuffer/heap access.
+ */
Review Comment:
Use nxstyle function block. Don't mention nxstore.
##########
games/NXDoom/src/i_system.c:
##########
@@ -320,6 +332,71 @@ void i_quit(void)
exit(0);
}
+/* i_quit_signal_handler
+ *
+ * A supervisor process (nxstore) has no reachable in-game quit path to
+ * drive (no keyboard/touch input is wired up here) - it can only ask
+ * from the outside, via SIGTERM. This handler does the one thing a
+ * signal handler is safe to do: set a flag. It must NOT call i_quit()
+ * (or anything it does - munmap, fclose, exit()'s atexit chain) directly,
+ * because a signal can land at literally any point in this process's own
+ * execution, including mid-malloc()/mid-blit - exactly the same "unsafe
+ * mid-operation teardown" risk as being force-killed from outside, just
+ * moved from another task's context into this one. i_poll_quit_signal()
+ * defers the real work to a known-safe boundary instead.
+ */
+
+static void i_quit_signal_handler(int signo)
+{
+ (void)signo;
+ quit_requested = 1;
+}
+
+void i_install_quit_signal(void)
+{
+ struct sigaction sa;
+
+ /* This board's flat, single address-space build can relaunch NXDoom
+ * (via nxpkg) as a fresh loadable ELF module - a proper posix_spawn of
+ * a new module load, which gets its own zeroed .bss/re-initialized
+ * .data - but GAMES_NXDOOM is a tristate Kconfig symbol and can also
+ * be built in as a true built-in (MODULE=n) sharing this process's
+ * address space across "launches" with no fresh .bss at all. Reset
+ * both pieces of state a stale second invocation could see: a leaked
+ * quit_requested flag would call i_quit() again before the game even
+ * starts, and a leaked exit_funcs chain would run every previous
+ * invocation's exit handlers a second time (double free()s, etc.) in
+ * addition to this invocation's own.
+ */
+
+ quit_requested = 0;
+ exit_funcs = NULL;
+
+ memset(&sa, 0, sizeof(sa));
+ sa.sa_handler = i_quit_signal_handler;
+
+ if (sigaction(SIGTERM, &sa, NULL) < 0)
+ {
+ /* Not fatal - the game still runs, it just can't be asked to
+ * close cleanly from the outside (nxstore's close button will
+ * have nothing to signal into). Surface it rather than silently
Review Comment:
Do not mention nxstore. Remove last sentence.
##########
games/NXDoom/src/m_config.c:
##########
@@ -2136,6 +2159,11 @@ static void load_default_collection(default_collection_t
*collection)
memmove(strparm, strparm + 1, sizeof(strparm) - 1);
}
+ if (strparm[0] == '\0')
+ {
+ continue;
+ }
Review Comment:
Why do we check this? Should we get here if the first character is the null
terminator?
##########
games/NXDoom/src/m_config.c:
##########
@@ -2008,7 +2006,11 @@ static void set_variable(default_t *def, const char
*value)
case DEFAULT_INT:
case DEFAULT_INT_HEX:
- *def->location.i = parse_int_parameter(value);
+ if (parse_int_parameter(value, &intparm))
+ {
+ *def->location.i = intparm;
+ }
+
Review Comment:
Restore the original version.
##########
games/NXDoom/src/m_config.c:
##########
@@ -2096,12 +2103,28 @@ static void
load_default_collection(default_collection_t *collection)
return;
}
- while (!feof(f))
+ while (fgets(line, sizeof(line), f) != NULL)
{
- if (fscanf(f, "%79s %99[^\n]\n", defname, strparm) != 2)
+ strparm[0] = '\0';
+
+ /* Parse one physical line at a time. fscanf() with whitespace in
+ * its format can consume the next line as a missing value.
+ */
+
+ if (strchr(line, '\n') == NULL &&
+ strlen(line) == sizeof(line) - 1)
{
- /* This line doesn't match */
+ int ch;
+
+ while ((ch = fgetc(f)) != '\n' && ch != EOF)
+ {
+ }
Review Comment:
Why are we just discarding characters?
##########
games/NXDoom/src/m_config.c:
##########
@@ -1982,16 +1982,14 @@ static void
save_default_collection(default_collection_t *collection)
*
****************************************************************************/
-static int parse_int_parameter(const char *strparm)
+static int parse_int_parameter(const char *strparm, int *param)
{
- int param;
-
if (strparm[0] == '0' && strparm[1] == 'x')
- sscanf(strparm + 2, "%x", (unsigned int *)¶m);
- else
- sscanf(strparm, "%i", ¶m);
+ {
+ return sscanf(strparm + 2, "%x", (unsigned int *)param) == 1;
+ }
- return param;
+ return sscanf(strparm, "%i", param) == 1;
Review Comment:
Why are you changing the return value? This is wrong.
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -114,12 +119,46 @@ 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. Clamp y into range instead of touching memory outside the
+ * buffers' real bounds - this still renders the span (as one glitched
+ * row, the same "wrong but visible" failure mode vanilla DOOM has) so
+ * a bad plane doesn't leave a blank gap on screen either.
+ *
+ * The clamp bound must be viewheight, not SCREENHEIGHT: this y is
+ * stored into ds_y and later used by r_draw_span() to index
+ * ylookup[] (r_draw.c), which r_init_buffer() only populates for
+ * [0, viewheight) - viewheight can be smaller than SCREENHEIGHT (a
+ * sub-window within the physical screen), so entries from viewheight
+ * up to SCREENHEIGHT are zero-initialized (NULL) pointers. Clamping
+ * to SCREENHEIGHT - 1 instead of viewheight - 1 traded the original
+ * out-of-bounds write for a NULL-pointer-plus-offset framebuffer
+ * write - confirmed on real hardware as a load/store exception at a
+ * small virtual address. viewheight is always <= SCREENHEIGHT, so
+ * this bound is safe for cachedheight[]/etc. too.
Review Comment:
Delete and replace with
"Ensure array indices are in range before access."
This is a lot of unrelated information for someone reading through the code.
##########
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:
Maybe it's better here to `i_error` out? If the configuration is malformed
we shouldn't warn and continue.
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -160,27 +199,62 @@ static void r_map_plane(int y, int x1, int x2)
spanfunc();
}
+/* Row indices into spanstart[] (sized SCREENHEIGHT) that are only ever
+ * safe to use as an array index within that range - t1/b1/t2/b2 in
+ * r_make_spans() below are also compared directly against each other to
+ * drive the span-tracking state machine (including vanilla DOOM's 0xff
+ * sentinel for "no span"/edge-of-plane), and that comparison logic must
+ * see the real, un-clamped values or the sentinel handling breaks. Only
+ * the array touches themselves need guarding.
+ */
+
+static inline boolean r_row_in_range(int row)
+{
+ return row >= 0 && row < SCREENHEIGHT;
+}
+
static void r_make_spans(int x, int t1, int b1, int t2, int b2)
{
+ /* t1/b1/t2/b2 come from a visplane's top[]/bottom[] arrays. In valid
+ * play these are either a real screen row or vanilla DOOM's 0xff
+ * (255) "no span here" sentinel; the loop conditions normally keep
+ * that sentinel away from spanstart[]. A malformed renderer state
+ * can violate that invariant, however: row 255 was observed reaching
+ * r_map_plane() on real hardware, after spanstart[t1]/[b1] had already
+ * been evaluated as the call argument. Guard every spanstart[] touch
+ * directly instead of altering t1/b1/t2/b2, so the state-machine
+ * comparisons and normal sentinel handling remain unchanged. An
+ * invalid closing row uses column zero as its bounded fallback; an
+ * invalid opening row is ignored.
+ */
Review Comment:
Remove this. We can just say "checking that row is in range before indexing
arrays"
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -195,7 +269,70 @@ static void r_make_spans(int x, int t1, int b1, int t2,
int b2)
void r_init_planes(void)
{
- /* Doh! */
+#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS
+ /* These renderer scratch buffers are sized for a comfortable margin
+ * above vanilla DOOM's original limits and, on a DRAM-constrained
+ * target, blow the internal DRAM budget as static arrays - opt-in
+ * heap allocation instead (comes out of the PSRAM-backed user heap
+ * on this target) via CONFIG_GAMES_NXDOOM_HEAP_BUFFERS.
Review Comment:
Delete this. Why are they sized for a comfortable margin? They are user
configured. We don't need to describe the opt-in here.
##########
games/NXDoom/src/doom/r_plane.c:
##########
@@ -160,27 +199,62 @@ static void r_map_plane(int y, int x1, int x2)
spanfunc();
}
+/* Row indices into spanstart[] (sized SCREENHEIGHT) that are only ever
+ * safe to use as an array index within that range - t1/b1/t2/b2 in
+ * r_make_spans() below are also compared directly against each other to
+ * drive the span-tracking state machine (including vanilla DOOM's 0xff
+ * sentinel for "no span"/edge-of-plane), and that comparison logic must
+ * see the real, un-clamped values or the sentinel handling breaks. Only
+ * the array touches themselves need guarding.
+ */
Review Comment:
Remove.
##########
games/NXDoom/src/doom/r_draw.c:
##########
Review Comment:
Why are we using viewheight here? Does it get properly set?
--
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]
