Return error code -1 when closing file descriptors < 3 (stdin, stdout, stderr)

[banglday]

closes Issue #435

Conflict in This Issue

1. Some user programs may need to close stdout or stderr.
2. rv32emu itself relies on stdout and stderr for its own output.

Because of these two needs, there is a conflict about how close() should behave when fd < 3.

Solution : Do not actually close, but return an error code (-1)

Advantages:

• stdout and stderr remain open, so rv32emu can continue printing messages or logs.
• User programs get an error code, so they know the close operation was not successful. This avoids them thinking they successfully closed the output channels.
• After seeing the error, user programs can redirect output or do other error handling. This prevents confusion like “I thought it was closed, but it’s still printing.”

Disadvantages:

• This does not match real system behavior exactly. In a typical Unix-like system, calling close(1) or close(2) is allowed. Here, the emulator denies it to keep its own output working.

---

Conclusion

To prevent a user program from thinking it closed stdout or stderr when it really did not, the simplest approach is **Solution **: do nothing but return an error code (-1).

• This tells the user program that closing fd < 3 failed, so it will not assume the channels are closed.
• For most needs, where preserving emulator output is important (especially for debugging or analysis), this is a good compromise.

Since rv32emu is often used in teaching and research, being able to see the emulator’s own outputs is very helpful. Thus, returning -1 is recommended by default. If needed, you can provide parameters or redirection mechanisms to allow truly closing the file descriptors in special cases. This maintains a good user experience while still letting people enable the more realistic behavior if they want.

PS: I save all details of implementation in this document.

Summary by Bito

This PR updates the syscall_close function to prevent closing standard file descriptors (fd < 3) by returning an error code instead of success. The change improves system stability by protecting stdout and stderr while properly signaling failures to user programs. The implementation consolidates error handling and adds clearer error signaling.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

[bito-code-review]

Code Review Agent Run #339909

Actionable Suggestions - 1

• src/syscall.c - 1

  • Consider consolidating duplicate error handling code · Line 247-249

Review Details

• Files reviewed - 1 · Commit Range: 97328ae..97328ae

  • src/syscall.c
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Fb Infer (Static Code Analysis) - ✖︎ Failed

---

AI Code Review powered by

[bito-code-review]

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Bug Fix - File Descriptor Protection Enhancement	- syscall.c - Modified syscall_close to return -1 error code instead of 0 when attempting to close standard file descriptors

[bito-code-review]

Consider consolidating duplicate error handling code

Consider consolidating error handling by moving the error case to the if block. The current structure has duplicate error handling code in both branches.

Code suggestion

Check the AI-generated fix before applying

 -    uint32_t fd = rv_get_reg(rv, rv_reg_a0);
 +    uint32_t fd = rv_get_reg(rv, rv_reg_a0);
 +    rv_set_reg(rv, rv_reg_a0, -1);  /* Set error by default */
 
      if (fd >= 3) { /* lookup the file descriptor */
          map_iter_t it;
          map_find(attr->fd_map, &it, &fd);
          if (!map_at_end(attr->fd_map, &it)) {
              if (fclose(map_iter_value(&it, FILE *))) {
 -                /* error */
 -                rv_set_reg(rv, rv_reg_a0, -1);
                  return;
              }
              map_erase(attr->fd_map, &it);
              /* success */
              rv_set_reg(rv, rv_reg_a0, 0);
          }
 -    } else {
 -        /* error */
 -        rv_set_reg(rv, rv_reg_a0, -1);
      }

Code Review Run #339909

---

Is this a valid issue, or was it incorrectly flagged by the Agent?

• it was incorrectly flagged

[visitorckw]

I agree with robot's point. While it's a matter of taste, CONTRIBUTING.md explicitly advises avoiding nested if-else statements when possible.

[bito-code-review]

Code Review Agent Run #ae23a1

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 50eb2b2..50eb2b2

  • src/syscall.c
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Fb Infer (Static Code Analysis) - ✖︎ Failed

---

AI Code Review powered by

[banglday]

1. Read https://cbea.ms/git-commit/ carefully and refine the git commit messages.
2. Append Close #435 at the end of git commit messages.

Finished refining the commit message.

[bito-code-review]

Code Review Agent Run #73e0ad

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 843e61a..843e61a

  • src/syscall.c
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Fb Infer (Static Code Analysis) - ✖︎ Failed

---

AI Code Review powered by

[ChinYikMing]

Add some rv_log_error() here for better clarity?

[banglday]

When the emulator attempts to close a file descriptor less than 3, error logs are generated, resulting in the following output.

ubuntu@riscv-playground:~/projects/rv32emu$ build/rv32emu ~/projects/my-riscv-tests/hello.elf 
12:47:54 INFO  src/riscv.c:418: Log level: TRACE
12:47:54 INFO  src/riscv.c:431: /home/ubuntu/projects/my-riscv-tests/hello.elf ELF loaded
12:47:54 INFO  src/main.c:286: RISC-V emulator is created and ready to run
hello riscv
12:47:54 ERROR src/syscall.c:249: Attempted to close a file descriptor < 3 (fd=0). Operation not supported.
12:47:54 ERROR src/syscall.c:249: Attempted to close a file descriptor < 3 (fd=1). Operation not supported.
12:47:54 ERROR src/syscall.c:249: Attempted to close a file descriptor < 3 (fd=2). Operation not supported.
12:47:54 INFO  src/main.c:300: RISC-V emulator is destroyed

Source code of hello.elf doesn't close any file descriptor. This is likely because the operating system automatically closes all file descriptors belonging to a program when it terminates. As a result, close(0), close(1), and close(2) are executed, causing the error message to be printed multiple times.

I believe that outputting an error message in the syscall_close implementation may mislead users. I am currently considering a better way to display the error message.

[ChinYikMing]

We can utilize the built-in ELF loader to obtain the program counter (PC) of the exit entry in crt0. This allows us to determine whether the standard streams are closed by the process itself or by crt0, as crt0 follows the sequence: exit() → close(fd) when closing standard streams.

Please apply the patch if it skips the logging of closing standard streams by the crt0:

From 0fe2ac16114701fc53b0b2f90d49d0d77848df6e Mon Sep 17 00:00:00 2001
From: ChinYikMing <[email protected]>
Date: Fri, 14 Feb 2025 01:21:27 +0800
Subject: [PATCH] Deny closing standard stream by the process

---
 src/emulate.c | 12 +++++++++---
 src/riscv.c   |  9 +++++++++
 src/riscv.h   |  8 ++++++++
 src/syscall.c | 15 +++++++++++++++
 4 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/src/emulate.c b/src/emulate.c
index 70ec291..ec6292c 100644
--- a/src/emulate.c
+++ b/src/emulate.c
@@ -1053,13 +1053,19 @@ void rv_step(void *arg)
         /* by now, a block should be available */
         assert(block);
 
-        /* After emulating the previous block, it is determined whether the
+#if !RV32_HAS(SYSTEM)
+        /* on exit */
+        if (unlikely(block->ir_head->pc == PRIV(rv)->exit_addr))
+            PRIV(rv)->on_exit = true;
+#endif
+
+#if RV32_HAS(BLOCK_CHAINING)
+        /*
+         * After emulating the previous block, it is determined whether the
          * branch is taken or not. The IR array of the current block is then
          * assigned to either the branch_taken or branch_untaken pointer of
          * the previous block.
          */
-
-#if RV32_HAS(BLOCK_CHAINING)
         if (prev) {
             rv_insn_t *last_ir = prev->ir_tail;
             /* chain block */
diff --git a/src/riscv.c b/src/riscv.c
index d277618..61d95d2 100644
--- a/src/riscv.c
+++ b/src/riscv.c
@@ -434,6 +434,15 @@ riscv_t *rv_create(riscv_user_t rv_attr)
     if ((end = elf_get_symbol(elf, "_end")))
         attr->break_addr = end->st_value;
 
+#if !RV32_HAS(SYSTEM)
+    /* set not exiting */
+    attr->on_exit = false;
+
+    const struct Elf32_Sym *exit;
+    if ((exit = elf_get_symbol(elf, "exit")))
+        attr->exit_addr = exit->st_value;
+#endif
+
     assert(elf_load(elf, attr->mem));
 
     /* set the entry pc */
diff --git a/src/riscv.h b/src/riscv.h
index bc343e8..88eceef 100644
--- a/src/riscv.h
+++ b/src/riscv.h
@@ -548,6 +548,14 @@ typedef struct {
     /* the data segment break address */
     riscv_word_t break_addr;
 
+#if !RV32_HAS(SYSTEM)
+    /* the exit entry address */
+    riscv_word_t exit_addr;
+
+    /* flag to determine if the emulator exits the target program */
+    bool on_exit;
+#endif
+
     /* SBI timer */
     uint64_t timer;
 } vm_attr_t;
diff --git a/src/syscall.c b/src/syscall.c
index 09eab56..403fabe 100644
--- a/src/syscall.c
+++ b/src/syscall.c
@@ -230,6 +230,21 @@ static void syscall_close(riscv_t *rv)
     /* _close(fd); */
     uint32_t fd = rv_get_reg(rv, rv_reg_a0);
 
+#if !RV32_HAS(SYSTEM)
+    /*
+     * The crt0 closes standard file descriptor(0, 1, 2) when
+     * the process exits. Thus, the operations by the crt0
+     * should not considered as error.
+     */
+    if (fd < 3 && !PRIV(rv)->on_exit) {
+        rv_log_error(
+            "Attempted to close a file descriptor < 3 (fd=%u). Operation "
+            "not supported.",
+            fd);
+        return;
+    }
+#endif
+
     if (fd >= 3) { /* lookup the file descriptor */
         map_iter_t it;
         map_find(attr->fd_map, &it, &fd);
-- 
2.34.1

[ChinYikMing]

I forgot this: rv_set_reg(rv, rv_reg_a0, -1); :)

[bito-code-review]

Code Review Agent Run #e31fb8

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 6e775d3..6e775d3

  • src/syscall.c
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Fb Infer (Static Code Analysis) - ✖︎ Failed

---

AI Code Review powered by

[jserv]

Rebase the latest master branch, which resolves all known CI regressions.