From d17442a3f86f4167c47b8c08cdd66ba1deaeff90 Mon Sep 17 00:00:00 2001
From: Arvin Schnell <aschnell@suse.de>
Date: Mon, 23 Oct 2023 12:29:52 +0200
Subject: [PATCH] - improved error handling in SystemCmd

---
 snapper/SystemCmd.cc | 61 ++++++++++++++++++++++----------------------
 snapper/SystemCmd.h  |  8 ++----
 2 files changed, 33 insertions(+), 36 deletions(-)

diff --git a/snapper/SystemCmd.cc b/snapper/SystemCmd.cc
index 68982c8c..d3bded76 100644
--- a/snapper/SystemCmd.cc
+++ b/snapper/SystemCmd.cc
@@ -78,17 +78,6 @@ SystemCmd::~SystemCmd()
     }
 
 
-void
-SystemCmd::closeOpenFds() const
-    {
-    int max_fd = getdtablesize();
-    for( int fd = 3; fd < max_fd; fd++ )
-	{
-	close(fd);
-	}
-    }
-
-
     void
     SystemCmd::execute()
     {
@@ -123,38 +112,50 @@ SystemCmd::closeOpenFds() const
 	    }
 	y2deb("sout:" << pfds[0].fd << " serr:" << pfds[1].fd);
 
+	const int max_fd = getdtablesize();
+
 	const TmpForExec args_p(make_args());
 	const TmpForExec env_p(make_env());
 
 	switch( (Pid_i=fork()) )
 	    {
 	    case 0:
+	    {
+		// Do not use exit() here. Use _exit() instead.
+
+		// Only use async‐signal‐safe functions here, see fork(2) and
+		// signal-safety(7).
+
 		if( dup2( sout[1], STDOUT_FILENO )<0 )
-		    {
-		    y2err("dup2 stdout child failed errno:" << errno << " (" << stringerror(errno) << ")");
-		    }
+		    _exit(125);
 		if( dup2( serr[1], STDERR_FILENO )<0 )
-		    {
-		    y2err("dup2 stderr child failed errno:" << errno << " (" << stringerror(errno) << ")");
-		    }
+		    _exit(125);
 		if( close( sout[0] )<0 )
-		    {
-		    y2err("close child failed errno:" << errno << " (" << stringerror(errno) << ")");
-		    }
+		    _exit(125);
 		if( close( serr[0] )<0 )
-		    {
-		    y2err("close child failed errno:" << errno << " (" << stringerror(errno) << ")");
-		    }
+		    _exit(125);
+
+		for (int fd = 3; fd < max_fd; ++fd)
+		    close(fd);
+
+		execvpe(args.get_values()[0].c_str(), args_p.get(), env_p.get());
 
-		closeOpenFds();
+		if (errno == ENOENT)
+		    _exit(127);
+		if (errno == ENOEXEC || errno == EACCES || errno == EISDIR)
+		    _exit(126);
+		_exit(125);
+	    }
+	    break;
 
-		Ret_i = execvpe(args.get_values()[0].c_str(), args_p.get(), env_p.get());
-		y2err("SHOULD NOT HAPPEN execvpe returned ret=" << Ret_i << " errno=" << errno);
-		break;
 	    case -1:
+	    {
 		Ret_i = -1;
-		break;
+	    }
+	    break;
+
 	    default:
+	    {
 		if( close( sout[1] )<0 )
 		    {
 		    y2err("close parent failed errno:" << errno << " (" << stringerror(errno) << ")");
@@ -177,8 +178,8 @@ SystemCmd::closeOpenFds() const
 
 		doWait( Ret_i );
 		y2mil("stopwatch " << stopwatch << " for \"" << cmd() << "\"");
-
-		break;
+	    }
+	    break;
 	    }
 	}
     else
diff --git a/snapper/SystemCmd.h b/snapper/SystemCmd.h
index 88172b73..f8bcede1 100644
--- a/snapper/SystemCmd.h
+++ b/snapper/SystemCmd.h
@@ -104,7 +104,6 @@ namespace snapper
     private:
 
 	void invalidate();
-	void closeOpenFds() const;
 	void execute();
 	bool doWait(int& Ret_ir);
 	void checkOutput();
@@ -140,17 +139,14 @@ namespace snapper
 	/**
 	 * Constructs the args for the child process.
 	 *
-	 * Must not be called after exec since allocating the memory
-	 * for the vector is not allowed then (in a multithreaded
-	 * program), see fork(2) and signal-safety(7). So simply call
-	 * it right before fork.
+	 * Not async‐signal‐safe, see fork(2) and signal-safety(7).
 	 */
 	TmpForExec make_args() const;
 
 	/**
 	 * Constructs the environment for the child process.
 	 *
-	 * Same not as for make_args().
+	 * Not async‐signal‐safe, see fork(2) and signal-safety(7).
 	 */
 	TmpForExec make_env() const;
 
-- 
2.47.3