I don't think we should bring ! back.
I wanted to remove v and | (and some other stuff) shortly afterwards, but
several people objected.
I did suggest having a lightweight less in base for most people and adding
the full upstream less to ports for the stuff we don't want to maintain
(like we do for eg libevent) but other people didn't like that idea.
On 17 December 2017 at 15:48, kshe <ksh...@zoho.eu> wrote:
> On Sat, 16 Dec 2017 21:52:44 +0000, Theo de Raadt wrote:
> > > On Sat, 16 Dec 2017 19:39:27 +0000, Theo de Raadt wrote:
> > > > > On Sat, 16 Dec 2017 18:13:16 +0000, Jiri B wrote:
> > > > > > On Sat, Dec 16, 2017 at 04:55:44PM +0000, kshe wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > Would a patch to bring back the `!' command to less(1) be
> accepted? The
> > > > > > > commit message for its removal explains that ^Z should be used
> instead,
> > > > > > > but that obviously does not work if less(1) is run from
> something else
> > > > > > > than an interactive shell, for example when reading manual
> pages from a
> > > > > > > vi(1) instance spawned directly by `xterm -e vi' in a window
> manager or
> > > > > > > by `neww vi' in a tmux(1) session.
> > > > > >
> > > > > > Why should less be able to spawn another programs? This would
> undermine
> > > > > > all pledge work.
> > > > >
> > > > > Because of at least `v' and `|', less(1) already is able to invoke
> > > > > arbitrary programs, and accordingly needs the "proc exec" promise,
> so
> > > > > bringing `!' back would not change anything from a security
> perspective
> > > > > (otherwise, I would obviously not have made such a proposition).
> > > > >
> > > > > In fact, technically, what I want to do is still currently
> possible:
> > > > > from any less(1) instance, one may use `v' to invoke vi(1), and
> then use
> > > > > vi(1)'s own `!' command as desired. So the functionality of `!' is
> > > > > still there; it was only made more difficult to reach for no
> apparent
> > > > > reason.
> > > >
> > > > No apparent reason?
> > > >
> > > > Good you have an opinion. I have a different opinion: We should look
> > > > for rarely used functionality and gut it.
> > >
> > > I completely agree, and I also completely agree with the rest of what
> > > you said. However, in this particular case, the functionality of `!'
> is
> > > still fully (albeit indirectly) accessible, as shown above, and this is
> > > why its deletion, when not immediately followed by that of `|' and `v',
> > > made little sense for me.
> >
> > Oh, so you don't agree. Or do you. I can't tell. You haven't made up
> > your mind enough to have a final position?
>
> In the case of less(1), the underlying functionality of `!' (invoking
> arbitrary programs) has not been removed at all, as `!' itself was only
> one way amongst others of doing that. Therefore, I would have prefered
> that such an endeavour be conducted in steps at least as large as a
> pledge(2) category. You may say this is absolutist, but, in the end,
> users might actually be more inclined to accept such removals if they
> come with, and thus are justified by, a real and immediate security
> benefit, like stricter pledge(2) promises, rather than some vague
> theoretical explanation about the global state of their software
> environment.
>
> > [...]
> >
> > > May I go ahead and prepare a patch to remove "proc exec" entirely?
> >
> > Sure you could try, and see who freaks out. Exactly what the plan was
> > all along.
>
> The minimal diff below does that. If it is accepted, further cleanups
> would need to follow (in particular, removing a few unused variables and
> functions), and of course the manual would also need some adjustments.
>
> Index: cmd.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/less/cmd.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 cmd.h
> --- cmd.h 6 Nov 2015 15:58:01 -0000 1.10
> +++ cmd.h 17 Dec 2017 12:23:00 -0000
> @@ -42,12 +42,12 @@
> #define A_FF_LINE 29
> #define A_BF_LINE 30
> #define A_VERSION 31
> -#define A_VISUAL 32
> +/* 32 unused */
> #define A_F_WINDOW 33
> #define A_B_WINDOW 34
> #define A_F_BRACKET 35
> #define A_B_BRACKET 36
> -#define A_PIPE 37
> +/* 37 unused */
> #define A_INDEX_FILE 38
> #define A_UNDO_SEARCH 39
> #define A_FF_SCREEN 40
> Index: command.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/less/command.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 command.c
> --- command.c 12 Jan 2017 20:32:01 -0000 1.31
> +++ command.c 17 Dec 2017 12:23:00 -0000
> @@ -241,12 +241,6 @@ exec_mca(void)
> /* If tag structure is loaded then clean it up. */
> cleantags();
> break;
> - case A_PIPE:
> - if (secure)
> - break;
> - (void) pipe_mark(pipec, cbuf);
> - error("|done", NULL);
> - break;
> }
> }
>
> @@ -1396,35 +1390,6 @@ again:
> c = getcc();
> goto again;
>
> - case A_VISUAL:
> - /*
> - * Invoke an editor on the input file.
> - */
> - if (secure) {
> - error("Command not available", NULL);
> - break;
> - }
> - if (ch_getflags() & CH_HELPFILE)
> - break;
> - if (strcmp(get_filename(curr_ifile), "-") == 0) {
> - error("Cannot edit standard input", NULL);
> - break;
> - }
> - if (curr_altfilename != NULL) {
> - error("WARNING: This file was viewed via "
> - "LESSOPEN", NULL);
> - }
> - /*
> - * Expand the editor prototype string
> - * and pass it to the system to execute.
> - * (Make sure the screen is displayed so the
> - * expansion of "+%lm" works.)
> - */
> - make_display();
> - cmd_exec();
> - lsystem(pr_expand(editproto, 0), NULL);
> - break;
> -
> case A_NEXT_FILE:
> /*
> * Examine next file.
> @@ -1568,25 +1533,6 @@ again:
> cmd_exec();
> gomark(c);
> break;
> -
> - case A_PIPE:
> - if (secure) {
> - error("Command not available", NULL);
> - break;
> - }
> - start_mca(A_PIPE, "|mark: ", (void*)NULL, 0);
> - c = getcc();
> - if (c == erase_char || c == erase2_char ||
> - c == kill_char)
> - break;
> - if (c == '\n' || c == '\r')
> - c = '.';
> - if (badmark(c))
> - break;
> - pipec = c;
> - start_mca(A_PIPE, "!", ml_shell, 0);
> - c = getcc();
> - goto again;
>
> case A_B_BRACKET:
> case A_F_BRACKET:
> Index: decode.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/less/decode.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 decode.c
> --- decode.c 17 Sep 2016 15:06:41 -0000 1.18
> +++ decode.c 17 Dec 2017 12:23:00 -0000
> @@ -148,8 +148,6 @@ static unsigned char cmdtable[] =
> ':', 'x', 0, A_INDEX_FILE,
> ':', 'd', 0, A_REMOVE_FILE,
> ':', 't', 0, A_OPT_TOGGLE|A_EXTRA, 't', 0,
> - '|', 0, A_PIPE,
> - 'v', 0, A_VISUAL,
> '+', 0, A_FIRSTCMD,
>
> 'H', 0, A_HELP,
> Index: lesskey.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/less/lesskey.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 lesskey.c
> --- lesskey.c 17 Sep 2016 15:06:41 -0000 1.17
> +++ lesskey.c 17 Dec 2017 12:23:00 -0000
> @@ -134,7 +134,6 @@ struct cmdname cmdnames[] = {
> { "next-tag", A_NEXT_TAG },
> { "noaction", A_NOACTION },
> { "percent", A_PERCENT },
> - { "pipe", A_PIPE },
> { "prev-file", A_PREV_FILE },
> { "prev-tag", A_PREV_TAG },
> { "quit", A_QUIT },
> @@ -152,7 +151,6 @@ struct cmdname cmdnames[] = {
> { "toggle-option", A_OPT_TOGGLE },
> { "undo-hilite", A_UNDO_SEARCH },
> { "version", A_VERSION },
> - { "visual", A_VISUAL },
> { NULL, 0 }
> };
>
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/less/main.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 main.c
> --- main.c 17 Sep 2016 15:06:41 -0000 1.35
> +++ main.c 17 Dec 2017 12:23:00 -0000
> @@ -96,7 +96,7 @@ main(int argc, char *argv[])
> exit(1);
> }
> } else {
> - if (pledge("stdio rpath wpath cpath fattr proc exec tty",
> NULL) == -1) {
> + if (pledge("stdio rpath wpath cpath fattr tty", NULL) ==
> -1) {
> perror("pledge");
> exit(1);
> }
>
> Regards,
>
> kshe
>
>
