improve internal error checking of dup2, pipe

sekiguchi-nagisa · Apr 19, 2024 · c93522a · c93522a
1 parent b5868af
commit c93522a
Show file tree

Hide file tree

Showing 8 changed files with 137 additions and 47 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@
 - **Breaking Change**: when ``err-raise`` option is enabled, check pipeline exit status even if last-pipe
 - improve assertion error message of binary ``==`` and ``=~`` expression
     - now show left hand-side, right hand-side expression value
+- improve internal error checking of ``dup2``, ``pipe`` system call, now propagate these errors as ``SystemError``
 
 #### Builtin
 

diff --git a/src/constant.h b/src/constant.h
@@ -191,14 +191,16 @@ constexpr const char *ERROR_EXEC = "execution error: ";
 constexpr const char *ERROR_REDIR = "io redirection failed";
 constexpr const char *ERROR_UNDEF_ENV = "undefined environmental variable: ";
 constexpr const char *ERROR_SET_ENV = "not set environmental variable: ";
-constexpr const char *ERROR_READLINE = "readLine failed";
 
 // =====  error message  =====
 
 constexpr const char *ERROR_CMD_SUB = "command substitution failed";
 constexpr const char *ERROR_STRING_LIMIT = "reach String size limit";
 constexpr const char *ERROR_MAP_LIMIT = "reach Map size limit";
 constexpr const char *ERROR_NULL_CHAR_PATH = "file path contains null characters";
+constexpr const char *ERROR_READLINE = "readLine failed";
+constexpr const char *ERROR_PIPE = "pipe opend failed";
+constexpr const char *ERROR_FD_SETUP = "file descriptor setup failed";
 
 // =====  generic type name  =====
 

diff --git a/src/job.cpp b/src/job.cpp
@@ -235,7 +235,7 @@ JobObject::JobObject(unsigned int size, const Proc *procs, bool saveStdin,
     this->procs[i] = procs[i];
   }
   if (saveStdin) {
-    this->oldStdin = dupFDCloseOnExec(STDIN_FILENO);
+    this->oldStdin = dupFDCloseOnExec(STDIN_FILENO); // FIXME: report error
     setFlag(this->meta, ATTR_LAST_PIPE);
   }
   if (pid_t pid = this->getValidPid(0); pid > -1 && pid == getpgid(pid)) {
@@ -355,7 +355,7 @@ std::string JobObject::formatInfo(JobInfoFormat fmt) const {
 
 bool JobObject::restoreStdin() {
   if (this->oldStdin > -1 && this->isControlled()) {
-    dup2(this->oldStdin, STDIN_FILENO);
+    dup2(this->oldStdin, STDIN_FILENO); // FIXME: report error
     close(this->oldStdin);
     this->oldStdin = -1;
     return true;

diff --git a/src/redir.cpp b/src/redir.cpp
@@ -93,10 +93,12 @@ static int doIOHere(const StringRef &value, int newFd, bool insertNewline) {
         pipe.close(READ_PIPE);
         if (dup2(pipe[WRITE_PIPE], STDOUT_FILENO) < 0 ||
             write(STDOUT_FILENO, value.data(), value.size()) < 0) {
+          perror("IO here process failed");
           exit(1);
         }
         if (insertNewline) { // for here str (insert newline)
           if (write(STDOUT_FILENO, "\n", 1) < 0) {
+            perror("IO here process failed");
             exit(1);
           }
         }
@@ -249,11 +251,18 @@ static int redirectImpl(const RedirObject::Entry &entry, const bool overwrite) {
 }
 
 bool RedirObject::redirect(ARState &state) {
-  this->saveFDs();
+  std::string msg;
+  int errNum = 0;
+  if (!this->saveFDs()) {
+    errNum = errno;
+    msg = ERROR_REDIR;
+    msg += ": cannot save file descriptors";
+    goto END;
+  }
   for (auto &entry : this->entries) {
-    int r = redirectImpl(entry, state.has(RuntimeOption::CLOBBER));
-    if (!this->backupFDSet.empty() && r != 0) {
-      std::string msg = ERROR_REDIR;
+    errNum = redirectImpl(entry, state.has(RuntimeOption::CLOBBER));
+    if (!this->backupFDSet.empty() && errNum != 0) {
+      msg = ERROR_REDIR;
       if (entry.value) {
         if (entry.value.hasType(TYPE::String)) {
           auto ref = entry.value.asStrRef();
@@ -268,10 +277,14 @@ bool RedirObject::redirect(ARState &state) {
           msg += std::to_string(rawFd);
         }
       }
-      raiseSystemError(state, r, std::move(msg));
-      return false;
+      goto END;
     }
   }
+END:
+  if (errNum) {
+    raiseSystemError(state, errNum, std::move(msg));
+    return false;
+  }
   return true;
 }
 

diff --git a/src/redir.h b/src/redir.h
@@ -30,10 +30,11 @@ enum class PipeAccessor : unsigned char {};
 constexpr auto READ_PIPE = PipeAccessor{0};
 constexpr auto WRITE_PIPE = PipeAccessor{1};
 
-inline void tryToDup(int srcFd, int targetFd) {
+[[nodiscard]] inline bool tryToDup(int srcFd, int targetFd) {
   if (srcFd > -1) {
-    dup2(srcFd, targetFd);
+    return dup2(srcFd, targetFd) > -1;
   }
+  return true;
 }
 
 class Pipe {
@@ -46,9 +47,9 @@ class Pipe {
    * @return
    * if has error, return false
    */
-  bool open();
+  [[nodiscard]] bool open();
 
-  bool tryToOpen(const bool shouldOpen) {
+  [[nodiscard]] bool tryToOpen(const bool shouldOpen) {
     if (shouldOpen) {
       return this->open();
     }
@@ -77,9 +78,12 @@ class PipeList : public InlinedArray<Pipe, 6> {
 public:
   explicit PipeList(size_t size) : InlinedArray(size) {}
 
-  bool openAll() {
+  [[nodiscard]] bool openAll() {
     for (size_t i = 0; i < this->size(); i++) {
       if (!(*this)[i].open()) {
+        const int old = errno;
+        this->closeAll();
+        errno = old;
         return false;
       }
     }
@@ -93,17 +97,11 @@ class PipeList : public InlinedArray<Pipe, 6> {
   }
 };
 
-inline void redirInToNull() {
-  int fd = open("/dev/null", O_RDONLY);
-  dup2(fd, STDIN_FILENO);
-  close(fd);
-}
-
 struct PipeSet {
   Pipe in;
   Pipe out;
 
-  bool openAll(ForkKind kind) {
+  [[nodiscard]] bool openAll(ForkKind kind) {
     bool useInPipe = false;
     bool useOutPipe = false;
 
@@ -128,23 +126,38 @@ struct PipeSet {
     case ForkKind::PIPE_FAIL:
       break;
     }
-    return this->in.tryToOpen(useInPipe) && this->out.tryToOpen(useOutPipe);
+    if (!this->in.tryToOpen(useInPipe) || !this->out.tryToOpen(useOutPipe)) {
+      const int old = errno;
+      this->closeAll();
+      errno = old;
+      return false;
+    }
+    return true;
   }
 
   /**
    * only call once in child
    */
-  void setupChildStdin(ForkKind forkKind, bool jobctl) {
-    tryToDup(this->in[READ_PIPE], STDIN_FILENO);
+  [[nodiscard]] bool setupChildStdin(ForkKind forkKind, bool jobctl) {
+    if (!tryToDup(this->in[READ_PIPE], STDIN_FILENO)) {
+      return false;
+    }
     if ((forkKind == ForkKind::DISOWN || forkKind == ForkKind::JOB) && !jobctl) {
-      redirInToNull();
+      // redirect stdin to null
+      int fd = open("/dev/null", O_RDONLY);
+      const bool s = fd > -1 && dup2(fd, STDIN_FILENO) > -1;
+      const int old = errno;
+      close(fd);
+      errno = old;
+      return s;
     }
+    return true;
   }
 
   /**
    * only call once in child
    */
-  void setupChildStdout() { tryToDup(this->out[WRITE_PIPE], STDOUT_FILENO); }
+  [[nodiscard]] bool setupChildStdout() { return tryToDup(this->out[WRITE_PIPE], STDOUT_FILENO); }
 
   /**
    * call in parent and child
@@ -222,13 +235,17 @@ class RedirObject : public ObjectWithRtti<ObjectKind::Redir> {
   bool redirect(ARState &state);
 
 private:
-  void saveFDs() {
+  [[nodiscard]] bool saveFDs() {
     for (unsigned int i = 0; i < std::size(this->oldFds); i++) {
       if (this->backupFDSet.has(i)) {
         this->oldFds[i] = dupFDCloseOnExec(static_cast<int>(i));
+        if (this->oldFds[i] < 0 && errno != EBADF) {
+          return false;
+        }
       }
     }
     this->saved = true;
+    return true;
   }
 
   void restoreFDs() {
@@ -242,7 +259,7 @@ class RedirObject : public ObjectWithRtti<ObjectKind::Redir> {
         if (oldFd < 0) {
           close(fd);
         } else {
-          dup2(oldFd, fd);
+          dup2(oldFd, fd); // FIXME: report error
         }
       }
     }

diff --git a/src/vm.cpp b/src/vm.cpp
@@ -527,7 +527,10 @@ bool VM::forkAndEval(ARState &state, Value &&desc) {
 
   // set in/out pipe
   PipeSet pipeSet;
-  pipeSet.openAll(forkKind); // FIXME: check errno
+  if (!pipeSet.openAll(forkKind)) {
+    raiseSystemError(state, errno, ERROR_PIPE);
+    return false;
+  }
   const pid_t pgid = resolvePGID(state.isRootShell(), forkKind);
   const auto procOp = resolveProcOp(state, forkKind);
   const bool jobCtrl = state.isJobControl();
@@ -597,10 +600,17 @@ bool VM::forkAndEval(ARState &state, Value &&desc) {
 
     state.stack.ip() += offset - 1;
   } else if (proc.pid() == 0) { // child process
-    pipeSet.setupChildStdin(forkKind, jobCtrl);
-    pipeSet.setupChildStdout();
+    int errNum = 0;
+    const bool s = pipeSet.setupChildStdin(forkKind, jobCtrl) && pipeSet.setupChildStdout();
+    if (!s) {
+      errNum = errno;
+    }
     pipeSet.closeAll();
 
+    if (errNum != 0) {
+      raiseSystemError(state, errNum, ERROR_FD_SETUP);
+      return false;
+    }
     state.stack.ip() += 3;
   } else {
     raiseSystemError(state, EAGAIN, "fork failed");
@@ -777,7 +787,8 @@ bool VM::forkAndExec(ARState &state, const char *filePath, const ArrayObject &ar
   // setup self pipe
   Pipe selfPipe;
   if (!selfPipe.open()) {
-    fatal_perror("pipe creation error");
+    raiseSystemError(state, errno, ERROR_PIPE);
+    return false;
   }
 
   const pid_t pgid = resolvePGID(state.isRootShell(), ForkKind::NONE);
@@ -1223,9 +1234,11 @@ bool VM::callPipeline(ARState &state, Value &&desc, bool lastPipe, ForkKind fork
   assert(pipeSize > 0);
 
   PipeSet pipeSet;
-  pipeSet.openAll(forkKind); // FIXME: check errno
   PipeList pipes(pipeSize);
-  pipes.openAll(); // FIXME: checl errno
+  if (!pipeSet.openAll(forkKind) || !pipes.openAll()) {
+    raiseSystemError(state, errno, ERROR_PIPE);
+    return false;
+  }
 
   // fork
   InlinedArray<Proc, 6> children(procSize);
@@ -1254,24 +1267,34 @@ bool VM::callPipeline(ARState &state, Value &&desc, bool lastPipe, ForkKind fork
    * | PIPELINE | size: 1byte | br1(child): 2yte | ~  | brN(parent): 2byte |
    * +----------+-------------+------------------+    +--------------------+
    */
-  if (proc.pid() == 0) {  // child
+  if (proc.pid() == 0) { // child
+    int errNum = 0;
     if (procIndex == 0) { // first process
-      dup2(pipes[procIndex][WRITE_PIPE], STDOUT_FILENO);
-      pipeSet.setupChildStdin(forkKind, jobCtrl);
-    }
-    if (procIndex > 0 && procIndex < pipeSize) { // other process.
-      dup2(pipes[procIndex - 1][READ_PIPE], STDIN_FILENO);
-      dup2(pipes[procIndex][WRITE_PIPE], STDOUT_FILENO);
-    }
-    if (procIndex == pipeSize && !lastPipe) { // last process
-      dup2(pipes[procIndex - 1][READ_PIPE], STDIN_FILENO);
-      pipeSet.setupChildStdout();
+      if (dup2(pipes[procIndex][WRITE_PIPE], STDOUT_FILENO) < 0 ||
+          !pipeSet.setupChildStdin(forkKind, jobCtrl)) {
+        errNum = errno;
+      }
+    } else if (procIndex > 0 && procIndex < pipeSize) { // other process.
+      if (dup2(pipes[procIndex - 1][READ_PIPE], STDIN_FILENO) < 0 ||
+          dup2(pipes[procIndex][WRITE_PIPE], STDOUT_FILENO) < 0) {
+        errNum = errno;
+      }
+    } else if (procIndex == pipeSize && !lastPipe) { // last process
+      if (dup2(pipes[procIndex - 1][READ_PIPE], STDIN_FILENO) < 0 || !pipeSet.setupChildStdout()) {
+        errNum = errno;
+      }
     }
-    pipeSet.closeAll(); // FIXME: check error and force exit (not propagate error due to uncaught)
+    pipeSet.closeAll();
     pipes.closeAll();
 
+    if (errNum) {
+      raiseSystemError(state, errNum, ERROR_FD_SETUP);
+      return false;
+    }
+
     // set pc to next instruction
     state.stack.ip() += read16(state.stack.ip() + 1 + procIndex * 2) - 1;
+    return true;
   } else if (procIndex == procSize) { // parent (last pipeline)
     if (lastPipe) {
       /**
@@ -1280,8 +1303,15 @@ bool VM::callPipeline(ARState &state, Value &&desc, bool lastPipe, ForkKind fork
       auto jobEntry = JobObject::create(procSize, children.ptr(), true, state.emptyFDObj,
                                         state.emptyFDObj, std::move(desc));
       state.jobTable.attach(jobEntry);
-      dup2(pipes[procIndex - 1][READ_PIPE], STDIN_FILENO);
+      int errNum = 0;
+      if (dup2(pipes[procIndex - 1][READ_PIPE], STDIN_FILENO) < 0) {
+        errNum = errno;
+      }
       pipes.closeAll();
+      if (errNum) {
+        raiseSystemError(state, errNum, ERROR_FD_SETUP);
+        return false;
+      }
       state.stack.push(Value::create<PipelineObject>(state, std::move(jobEntry)));
     } else {
       pipeSet.in.close(READ_PIPE);

diff --git a/test/exec/cases/base/builtin_ulimit.ds → test/exec/cases/base/builtin_ulimit1.ds b/test/exec/cases/base/builtin_ulimit.ds → test/exec/cases/base/builtin_ulimit1.ds
diff --git a/test/exec/cases/base/builtin_ulimit2.ds b/test/exec/cases/base/builtin_ulimit2.ds
@@ -0,0 +1,27 @@
+#$test($ignored = 'cygwin|msys')
+
+# set file descriptor limit
+var ex = 435 as Any
+
+ulimit -S -n 6
+try {
+    ls | grep '*' | grep '*' | grep '*' | grep '*' | grep '*'
+} catch e { $ex = $e; }  # too many open pipe
+assert ($ex as SystemError).message() =~ $/pipe opend failed, caused by.*many.*file.*/
+
+{
+    var fds : [FD]
+    for(var i = 0; $i < 10; $i++) {
+        $fds.add(try{ $STDIN.dup(); } catch _ { break; })
+    }  # open fd up to limit
+    if !$fds.empty() {
+        $fds.pop() # close one fd
+    }
+    $ex = 34
+    try {
+        echo &> /dev/null
+    } catch e { $ex = $e; }
+    assert ($ex as SystemError).message() =~ $/.*cannot save file descriptors, caused by.*argument.*/
+}
+
+true