[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dash
Subject:    Re: [BUG] 'fc -s' infinite loop
From:       Harald van Dijk <harald () gigawatt ! nl>
Date:       2019-01-09 23:41:06
Message-ID: b88cbf62-3e32-d980-3709-36442d4c0822 () gigawatt ! nl
[Download RAW message or body]

On 01/01/2019 16:24, Harald van Dijk wrote:
> On 01/01/2019 14:27, Martijn Dekker wrote:
>> On dash and on gwsh, the 'fc -s' command, that re-executes a command 
>> from the history, executes the command repeatedly in an infinite loop. 
>> It should execute it just once.
> 
> This is caused by the block with the XXX comment in src/histedit.c:
> 
>>                                 if (displayhist && hist) {
>>                                         /*
>>                                          *  XXX what about recursive and
>>                                          *  relative histnums.
>>                                          */
>>                                         history(hist, &he, H_ENTER, s);
>>                                 }
> 
> This changes the position in the history list and also clobbers he, so 
> the if (he.num == last) check afterwards always returns false, and if 
> the check was supposed to return false, the next history(hist, &he, 
> direction) would not do what was intended.
> 
> The immediate problem can be fixed either by dropping support for the 
> non-standard and undocumented fc -s first last and immediately breaking 
> out of the loop, or by restoring the position (restoring he at the same 
> time).

Keeping fc -s first last working is pretty much not an option without 
modifying it to generate a full list of commands to execute first. 
Otherwise, the comment

>                 /*
>                  * At end?  (if we were to lose last, we'd sure be
>                  * messed up).
>                  */

is sure to hit: executing the new commands may clear out previous 
history entries, including last. fc -s first does not have that problem 
since it only takes a single command -- except that it should enter the 
command into the history before executing it, not after it, at which 
point you can get that problem after all. However...

> But there are more problems: H_ENTER adds a new command to the history. 
> bash and ksh do not do this:
> 
>    echo hello
>    fc -s 1
>    fc -l
> 
> will show (bash, but ksh is similar)
> 
>    hello
>    1        echo hello
>    2        echo hello
> 
> That is, the echo hello command appears twice in the history, but the fc 
> -s 1 command does not appear at all. This is what POSIX probably[*] 
> requires as well.

...modifying fc to use H_REPLACE nicely avoids that problem. Since no 
new history entry is created, no old entry can be cleared out.

> By the time fc is called, the fc line is already in the history, so to 
> get the bash/ksh behaviour, the current command would need to be 
> overwritten. The documentation of libedit does not show any option which 
> can overwrite entries already created. It documents H_ADD and H_APPEND, 
> which can append, and has an undocumented H_REPLACE used by the equally 
> undocumented replace_history_entry() function in <editline/readline.h>. 
> I do not know yet if it makes sense to start using this.

Having thought about it, I think it does make sense, and have looked at 
how to use H_REPLACE. It takes a char * and a void *, where the char * 
specifies the new string for the currently selected history entry, and 
the void * is application-specific data. Since no application-specific 
data is ever used, it can simply be hardcoded to (void *) NULL, making 
sure to use a cast because history() is a variadic function.

Since H_REPLACE operates on the currently selected history entry, it 
requires H_FIRST to re-select the most-recent history entry.

A corner case is when one command line consists of multiple fc commands:

   echo abc
   echo def
   fc -s 1; fc -s 2

Here, bash and ksh let the last fc -s command "win": after this, the 
history is

   echo abc
   echo def
   echo def

but the output is

   abc
   def
   abc
   def

Using H_REPLACE replicates that.

> Also, when neither -s nor -l is specified and an editor is invoked, the 
> new command is supposed to be entered into the history as well. That too 
> is not implemented.

This can be implemented by modifying the parser to allow creating 
history entries for files pushed back later. Currently, the check is 
limited to stdin:

> if (parsefile->fd == 0 && hist && something)

but keeping track of whether the history should be saved for each file, 
and then turning it on when fc without -s/-l is used, lets this work.

Other bugs:

- fc without arguments is perfectly valid and should not raise an error.

- The extension to allow negative numeric arguments without -- should 
work on BSD but does not work on glibc, because optind is set to 0 
rather than 1.

- Commands containing blank lines are entered into the history without 
the blank lines. Entirely blank commands should not be entered in the 
history, but

   echo "hello

   world"

should not enter the history as

   echo "hello
   world"

since that has a different effect.

- fc -s's old=new option should not be accepted and ignored when -s is 
not specified. old=new is supposed to be interpreted as a search string 
in that case.

- When an out-of-range number is specified for last, it should not be 
resolved using simply H_FIRST, as that is the command containing the 
current fc invocation.[*]

What I have implemented does not apply to dash cleanly due to other 
changes I have made, but could be easily adapted if desired. The fc 
command is only useful in interactive shells, and dash is not used much 
as an interactive shell, so I do not know how much interest there is in 
letting this work correctly.

Cheers,
Harald van Dijk

[*] At least, unless it is desired to make that invocation accessible, 
but in that case it should also be printed by default with fc -l. Some 
other shells do that, but bash doesn't, the way it's implemented in ksh 
appears to be broken, and I don't personally think it makes sense.
[prev in list] [next in list] [prev in thread] [next in thread]