Failure of "typeset" and exit status

Failure of "typeset" and exit status

[Bart Schaefer]

torch% ( () { typeset +g -m \* && echo No error } )
(anon): failed to change user ID: operation not permitted
(anon): failed to change group ID: operation not permitted
(anon): failed to change effective user ID: operation not permitted
(anon): failed to change effective group ID: operation not permitted
No error
torch% 

OK, that's not SO bad, except that "typeset +g -m \*" is intended to
have made all the variables local ... which it has NOT, as you can see
(be sure to do this in a subshell if you try it yourself):

torch% ( () { typeset +g -m \* && unset -m \* } && typeset -p )
(anon): failed to change user ID: operation not permitted
(anon): failed to change group ID: operation not permitted
(anon): failed to change effective user ID: operation not permitted
(anon): failed to change effective group ID: operation not permitted
(anon): read-only variable: HISTCMD
torch% 

If I add the -h flag to mask specials, it works as expected:

torch% ( () { typeset +g -h -m \* && unset -m \* } && typeset -p )
typeset 0=Src/zsh
...
typeset zsh_scheduled_events
torch% 

Why did failure on those five variables result in ignoring +g for all of
the other variables?  And if failure is going to be treated as idempotent,
shouldn't it exit nonzero?

-- 
Barton E. Schaefer

Re: Failure of "typeset" and exit status

[Peter Stephenson]

On Mon, 11 May 2015 19:43:20 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> torch% ( () { typeset +g -m \* && echo No error } )
> (anon): failed to change user ID: operation not permitted
> (anon): failed to change group ID: operation not permitted
> (anon): failed to change effective user ID: operation not permitted
> (anon): failed to change effective group ID: operation not permitted
> No error
> torch% 
> 
> OK, that's not SO bad, except that "typeset +g -m \*" is intended to
> have made all the variables local

No, it isn't.  The "g" is a bit of a snare and a delusion.  "-g" means
"use the global scope, never create a new variable even if you're a
local scope".  "+g" means "it's OK to create a new variable if you
need", it doesn't mean "never use the existing variable".  Remember that
option letters with no argument have a binary effect in typeset and +g
is the default (exactly equivalent to saying -g is not the default).
You're using the -m option to refer to existing variables, so the +g has
no effect.

pws

Re: Failure of "typeset" and exit status

[Peter Stephenson]

On Tue, 12 May 2015 09:42:25 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> You're using the -m option to refer to existing variables, so the +g has
> no effect.

Oh, wait, there's an explicit exception for the case of -m and +g.

Ick.

Yes, that's a *very* good way of ending up with a broken mess...

pws

Re: Failure of "typeset" and exit status

[Bart Schaefer]

On May 11,  7:43pm, Bart Schaefer wrote:
} Subject: Failure of "typeset" and exit status
}
} torch% ( () { typeset +g -m \* && echo No error } )
} (anon): failed to change user ID: operation not permitted
} (anon): failed to change group ID: operation not permitted
} (anon): failed to change effective user ID: operation not permitted
} (anon): failed to change effective group ID: operation not permitted
} No error
} torch% 
} 
} OK, that's not SO bad, except that "typeset +g -m \*" is intended to
} have made all the variables local ... which it has NOT, as you can see

OK, this is both more confusing and less borked than I thought.

The variables HAVE all been made local.  A subtlety that escaped me is
that in the example above, the read-only error on HISTCMD did not
happen, as it did below:

} (be sure to do this in a subshell if you try it yourself):
} 
} torch% ( () { typeset +g -m \* && unset -m \* } && typeset -p )
} (anon): failed to change user ID: operation not permitted
} (anon): failed to change group ID: operation not permitted
} (anon): failed to change effective user ID: operation not permitted
} (anon): failed to change effective group ID: operation not permitted
} (anon): read-only variable: HISTCMD
} torch% 

I forgot that "unset" on a read-only variable is a fatal error; it has
killed the entire subshell, so "typeset -p" never runs.  I incorrectly
interpreted the lack of output as meaning that all parameters were
unset, and thus were not local.

The *reason* that I believed that is because, when I ran that command
without the subshell parens, the prompt disappeared.  Turns out that
what's really happening there is that when PROMPT is made local, the
value of PS1 is erased because of the way the two are linked.  PS1 is
not yet a local at that point, so even when it is made local later the
damage has been done, localizing PROMPT has clobbered the global PS1
and thereby also clobbered the global PROMPT.

I'm entirely without ideas as to what we should do about that.  Likely
nothing except perhaps documenting it somewhere.

Now, the other question is, why is it a fatal error to attempt to change
a variable that is explicitly read-only, but only a warning to attempt
to change UID, GID, etc. when you do not have permission to do so?

Re: Failure of "typeset" and exit status

[Peter Stephenson]

On Tue, 12 May 2015 21:59:19 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Now, the other question is, why is it a fatal error to attempt to change
> a variable that is explicitly read-only, but only a warning to attempt
> to change UID, GID, etc. when you do not have permission to do so?

So you're worried about this

  % (){ local UID && print Still going; }
  (anon): failed to change user ID: operation not permitted
  Still going

and the other typeset stuff isn't relevant here.

As far as the *shell* is concerned, you *do* have permission to change
it.  It just happens to be attached to a system call that failed.  The
UID was succesfully made local, and printing it successfully reflects
your current UID.

I don't really have any feel for how to treat failures of this kind.

Here's one possibility: in that case, there's no explicit set to UID so
maybe we should make it local and leave it alone --- I'm not sure how to
detect a case like this, though.  Then if you explicitly assign to it
(in our out of typeset) and *that* fails, return status 1.  Whether this
is a hard error is really up to convenience, since there's no obvious
prior art:  is it more useful forcing people to use a subshell to test
for the assignment failure to ensure attempts to set UID are picked up
safely, or to require them to test the command status?

pws

Re: Failure of "typeset" and exit status

[Bart Schaefer]

On May 13,  9:39am, Peter Stephenson wrote:
}
} So you're worried about this
} 
}   % (){ local UID && print Still going; }
}   (anon): failed to change user ID: operation not permitted
}   Still going
} 
} Here's one possibility: in that case, there's no explicit set to UID so
} maybe we should make it local and leave it alone --- I'm not sure how to
} detect a case like this, though.

It's weird that unset produces no error, but local does so even if UID was
previously unset:

torch% unset UID
torch% (){ local UID } 
(anon): failed to change user ID: operation not permitted
torch% (){ unset UID }
torch% 

} Then if you explicitly assign to it
} (in our out of typeset) and *that* fails, return status 1.

I looked at that first, but there is a whole chain of void-returning
functions down from bin_typeset() to the setuid() call that triggers
the warning.  Propagating a non-fatal error would require a lot of
rejiggering.

It appears that the real problem is that UID is typed as an integer,
so "local UID" implicitly assigns zero.  That means that for a process
that IS allowed to change UID, merely declaring it local causes that
process to assume root privilege.  That's clearly both wrong and a
potential security issue.

-- 
Barton E. Schaefer

Re: Failure of "typeset" and exit status

[Peter Stephenson]

On Wed, 13 May 2015 08:48:27 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> It appears that the real problem is that UID is typed as an integer,
> so "local UID" implicitly assigns zero.  That means that for a process
> that IS allowed to change UID, merely declaring it local causes that
> process to assume root privilege.  That's clearly both wrong and a
> potential security issue.

How about this?  It's not so clear it's needed for HISTSIZE and
SAVEHIST, but it's not obviously stupid, and having a set of
paranoia-inducing variables separate from the restricted set looks like
overkill.

What about non-integer restricted variables?  They seem to be less
problematic.

pws

diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index 44df07c..eb3eb36 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -642,6 +642,9 @@ privileges, you may change the effective group ID of the shell
 process by assigning to this parameter.  Also (assuming sufficient
 privileges), you may start a single command with a different
 effective group ID by `tt(LPAR()EGID=)var(gid)tt(; command+RPAR())'
+
+If this is made local, it is not implicitly set to 0, but may be
+explicitly set locally.
 )
 vindex(EUID)
 item(tt(EUID) <S>)(
@@ -650,6 +653,9 @@ privileges, you may change the effective user ID of the shell process
 by assigning to this parameter.  Also (assuming sufficient privileges),
 you may start a single command with a different
 effective user ID by `tt(LPAR()EUID=)var(uid)tt(; command+RPAR())'
+
+If this is made local, it is not implicitly set to 0, but may be
+explicitly set locally.
 )
 vindex(ERRNO)
 item(tt(ERRNO) <S>)(
@@ -666,6 +672,9 @@ you may change the group ID of the shell process by assigning to this
 parameter.  Also (assuming sufficient privileges), you may start a single
 command under a different
 group ID by `tt(LPAR()GID=)var(gid)tt(; command+RPAR())'
+
+If this is made local, it is not implicitly set to 0, but may be
+explicitly set locally.
 )
 vindex(HISTCMD)
 item(tt(HISTCMD))(
@@ -801,6 +810,9 @@ you may change the user ID of the shell by assigning to this parameter.
 Also (assuming sufficient privileges), you may start a single command
 under a different
 user ID by `tt(LPAR()UID=)var(uid)tt(; command+RPAR())'
+
+If this is made local, it is not implicitly set to 0, but may be
+explicitly set locally.
 )
 vindex(USERNAME)
 item(tt(USERNAME) <S>)(
@@ -1098,6 +1110,9 @@ The maximum number of events stored in the internal history list.
 If you use the tt(HIST_EXPIRE_DUPS_FIRST) option, setting this value
 larger than the tt(SAVEHIST) size will give you the difference as a
 cushion for saving duplicated history events.
+
+If this is made local, it is not implicitly set to 0, but may be
+explicitly set locally.
 )
 vindex(HOME)
 item(tt(HOME) <S>)(
@@ -1392,6 +1407,9 @@ It is expanded in the same way as tt(PS2).
 vindex(SAVEHIST)
 item(tt(SAVEHIST))(
 The maximum number of history events to save in the history file.
+
+If this is made local, it is not implicitly set to 0, but may be
+explicitly set locally.
 )
 vindex(SPROMPT)
 item(tt(SPROMPT) <S>)(
diff --git a/Src/builtin.c b/Src/builtin.c
index 70e75ff..95537a9 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2344,7 +2344,12 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	    pm->gsu.s->setfn(pm, ztrdup(""));
 	    break;
 	case PM_INTEGER:
-	    pm->gsu.i->setfn(pm, 0);
+	    /*
+	     * Restricted integers are dangerous to inialize to 0,
+	     * so don't do that.
+	     */
+	    if (!(pm->old->node.flags & PM_RESTRICTED))
+		pm->gsu.i->setfn(pm, 0);
 	    break;
 	case PM_EFLOAT:
 	case PM_FFLOAT:

Re: Failure of "typeset" and exit status

[Bart Schaefer]

On May 13,  5:38pm, Peter Stephenson wrote:
} Subject: Re: Failure of "typeset" and exit status
}
} On Wed, 13 May 2015 08:48:27 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > It appears that the real problem is that UID is typed as an integer,
} > so "local UID" implicitly assigns zero.
} 
} How about this?

Except that you misspelt "initialise" in the comment, I think this is
just right.

} What about non-integer restricted variables?  They seem to be less
} problematic.

The only one that comes to mind is USERNAME, which also attempts to change
UID, but it's already sanitized because it checks for a valid password
file entry before doing anything.

The problems with localizing PATH-flavored variables are already known.