Re: [PATCH] Silence compilation warnings about setuid, setgid

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Wed, 13 Jun 2018 04:49:39 -0700
Eitan Adler <lists@xxxxxxxxxxxxxx> wrote:
> On 7 May 2018 at 04:18, Sebastian Gniazdowski
> <sgniazdowski@xxxxxxxxx> wrote:
> > Hello,
> > on a Linux box I see:
> > Looking at the source, the reported calls are "extra" ones, they are
> > followed by proper setuid, setgid calls. I've found some way out
> > from this situation, of using the report value and reporting it
> > (gmail paste, proper patch is attached):
> >  
> 
> >  #ifdef HAVE_SETUID
> > -       setuid(getuid());
> > -       setgid(getgid());  
> 
> While we're touching this code can we please correct the order of
> setuid and setgid?
> 
> setgid must be called before setuid. If setuid is called first, on
> some platforms, it no longer has privs to call setgid aymore.

Presumably that's a trivial swap?  I don't know if we need both
setgid()s before both setuid()s, because I don't know why they're
repeated --- but if the second case is simply to test for an error that's
not a big deal since if it worked properly there won't be one.

I didn't look at the original patch before now --- the obvious way to
fix it would simply be a cast to void.  There's no comment about why the
code is like that, so perhaps retaining the error number is safer.
However, I think it's just confusing except in the (few?)  cases where
the error number is different the first time.  I ended up with this...

diff --git a/Src/options.c b/Src/options.c
index 590652e..14d9c3c 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -769,15 +769,24 @@ dosetopt(int optno, int value, int force, char *new_opts)
     } else if(optno == PRIVILEGED && !value) {
 	/* unsetting PRIVILEGED causes the shell to make itself unprivileged */
 #ifdef HAVE_SETUID
-	setuid(getuid());
-	setgid(getgid());
-        if (setuid(getuid())) {
-            zwarn("failed to change user ID: %e", errno);
-            return -1;
-	} else if (setgid(getgid())) {
+	int uerr = 0, gerr = 0;
+
+	errno = 0;
+	if (setgid(getgid()))
+	    gerr = errno;
+	if (setuid(getuid()))
+	    uerr = errno;
+	if (setgid(getgid())) {
             zwarn("failed to change group ID: %e", errno);
+            if (gerr && gerr != errno)
+                zwarn("(error of additional preceding setgid() call: %e)", gerr);
             return -1;
-        }
+        } else if (setuid(getuid())) {
+            zwarn("failed to change user ID: %e", errno);
+            if (uerr && uerr != errno)
+                zwarn("(error of additional preceding setuid() call: %e)", uerr);
+            return -1;
+	}
 #else
         zwarn("setuid not available");
         return -1;