tcsh-org / tcsh

This is a read-only mirror of the tcsh code repository.
https://www.tcsh.org/
Other
232 stars 42 forks source link

tcsh is unusable when `$status` is read-only #40

Open GabrielRavier opened 3 years ago

GabrielRavier commented 3 years ago

I don't know whether to put this in the previous issue, but this seems different enough to put it into a new one, even if it is directly related.

Anyway, doing set -r status makes tcsh completely unusable afterwards, i.e. a session looks like this:

> set -r status
> echo a
set: $status is read-only.
> /bin/echo a
set: $status is read-only.
> non-existing-command
set: $status is read-only.
> exit
set: $status is read-only.
> 
suominen commented 3 years ago

What is the behaviour you are hoping to get from set -r status?

GabrielRavier commented 3 years ago

Well, probably something else than the current behavior. There are a few things that would be possible, in my opinion:

  1. Give an error when trying to make $status read-only, as that breaks everything
  2. Make it just freeze $status as a whole, and avoid giving random errors relating to automatic updates of it (especially in the sense that it makes the whole shell completely and utterly unusable as commands don't run at all)
  3. Have it prevent user changes to $status but not built-in changes like the one that occurs after every command

I personally don't like 3 much but 1 and 2 seem like they could both be practical in some way, at least certainly better than the current behavior (I personally have a slight preference for 2, but it's not for me to decide).

suominen commented 3 years ago

Without a solid use-case for set -r status it doesn't seem reasonable to special case it in the code.

To me set -r status is not a sensible thing to do. I was trying to understand what you are trying to achieve. If a non-sensible action results in an unusable shell, my inclination would just be to recommend against taking the non-sensible action. :)

GabrielRavier commented 3 years ago

Well, even if the other possible use-cases I presented here don't seem very useful (though they'd all be made possible by one of the simple possible fixes to avoid breaking the shell), there's most certainly a solid use-case for having it at least not break the shell, in the same way there's a solid use-case for fixing other bugs that break or crash the shell (at least, I certainly hope you consider that worth doing). So the least that should be done, in my opinion, is making it not break the shell by either having the shell ignore errors from read-only $status or by preventing the user from making it read-only in the first place.

suominen commented 3 years ago

I don't think the user shoud set -r status to begin with. From the manual page:

Read-only variables may not be modified or unset; attempting to do so will cause an error. Once made read-only, a variable cannot be made writable, so `set -r' should be used with caution.

If you accidentally set -r status, you can exit the shell and start a new one to recover.

In my mind this falls into the sudo rm -rf / category of things: the user needs to consider if their actions are reasonable or desirable for the state of the system.

It would seem to me that tcsh is now behaving as documented: it is displaying the error that results from making $status read-only (as opposed to becoming catatonic and/or crashing).

GabrielRavier commented 3 years ago

How is being unable to execute any commands at all (even exit) not "becoming catatonic" ? I've tried to test a few things, and apart from triggering syntax errors, putting tcsh in this state seems to just makes it a "set: $status is read-only" dispenser

GabrielRavier commented 3 years ago

It would seem to me that tcsh is now behaving as documented: it is displaying the error that results from making $status read-only

This doesn't actually seem true: It isn't mentioned anywhere that the shell will execute the equivalent of set status=0 before every command and will thus fail to execute anything at all if status is read-only, at best you could potentially infer it as a potential implementation strategy as it is said that "The shell updates [...] status when necessary" but not much else

zoulasc commented 3 years ago

I guess that a possible fix is to special-case $status and only warn if it is read-only.

Jamie-Landeg-Jones commented 2 years ago

I just tried testing this. I guess it's because I have precmd set, but I just got the error that status is read-only repeating in an endless loop until tcsh coredumped with SIGSEGV after about a minute!