PATCH: 'UID=42 whoami' proceeds despite error

PATCH: 'UID=42 whoami' proceeds despite error

[Daniel Shahaf]

With current master, `UID=42 foo` will run 'foo' as the current UID, not
as UID 42, if setuid() failed:

% UID=42 id -u; echo $?
zsh: failed to change user ID: operation not permitted
1000
0
% 

How about the following?

diff --git a/Src/params.c b/Src/params.c
index aa8b196..13fa36e 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -4060,8 +4060,10 @@ void
 uidsetfn(UNUSED(Param pm), zlong x)
 {
 #ifdef HAVE_SETUID
-    if (setuid((uid_t)x))
+    if (setuid((uid_t)x)) {
 	zwarn("failed to change user ID: %e", errno);
+	errflag |= ERRFLAG_ERROR;
+    }
 #endif
 }
 
@@ -4081,8 +4083,10 @@ void
 euidsetfn(UNUSED(Param pm), zlong x)
 {
 #ifdef HAVE_SETEUID
-    if (seteuid((uid_t)x))
+    if (seteuid((uid_t)x)) {
 	zwarn("failed to change effective user ID: %e", errno);
+	errflag |= ERRFLAG_ERROR;
+    }
 #endif
 }
 
@@ -4102,8 +4106,10 @@ void
 gidsetfn(UNUSED(Param pm), zlong x)
 {
 #ifdef HAVE_SETUID
-    if (setgid((gid_t)x))
+    if (setgid((gid_t)x)) {
 	zwarn("failed to change group ID: %e", errno);
+	errflag |= ERRFLAG_ERROR;
+    }
 #endif
 }
 
@@ -4123,8 +4129,10 @@ void
 egidsetfn(UNUSED(Param pm), zlong x)
 {
 #ifdef HAVE_SETEUID
-    if (setegid((gid_t)x))
+    if (setegid((gid_t)x)) {
 	zwarn("failed to change effective group ID: %e", errno);
+	errflag |= ERRFLAG_ERROR;
+    }
 #endif
 }
 
diff --git a/Test/B02typeset.ztst b/Test/B02typeset.ztst
index 6d85a63..9c56c7e 100644
--- a/Test/B02typeset.ztst
+++ b/Test/B02typeset.ztst
@@ -711,3 +711,13 @@
   typeset isreadonly=still
 1:typeset returns status 1 if setting readonly variable
 ?(eval):2: read-only variable: isreadonly
+
+  if (( UID )); then
+    UID=$((UID+1)) date; echo "Status is printed, $?"
+  else
+    ZTST_skip="cannot test setuid error when tests run as superuser"
+  fi
+0:when cannot change UID, the command isn't run
+# 'date' did not run.
+>Status is printed, 1
+?(eval):2: failed to change user ID: operation not permitted

Re: PATCH: 'UID=42 whoami' proceeds despite error

[Peter Stephenson]

On Fri, 2 Dec 2016 12:08:26 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> With current master, `UID=42 foo` will run 'foo' as the current UID, not
> as UID 42, if setuid() failed:
> 
> % UID=42 id -u; echo $?
> zsh: failed to change user ID: operation not permitted
> 1000
> 0
> % 
> 
> How about the following?

Yes, you'd certainly have thought doing it anyway with the wrong UID
wasn't what the user expected.

pws

Re: PATCH: 'UID=42 whoami' proceeds despite error

[Bart Schaefer]

On Dec 2, 12:08pm, Daniel Shahaf wrote:
}
} +    if (setuid((uid_t)x)) {
}  	zwarn("failed to change user ID: %e", errno);
} +	errflag |= ERRFLAG_ERROR;
} +    }

This is the wrong way to do this.  Just call zerr() instead of zwarn().