Re: ex/vi 100% CPU when STDIN_FILENO set to O_NONBLOCK

On Sun, Dec 11, 2022 at 04:10:41PM -0800, Jeremy Mates wrote:
> ...
> 42136 ex RET read -1 errno 35 Resource temporarily unavailable
> 42136 ex CALL read(0,0x3d94b585400,0xff)
> 42136 ex RET read -1 errno 35 Resource temporarily unavailable
> 42136 ex CALL read(0,0x3d94b585400,0xff)
> ...
>
> this condition can be reproduced with:
>
> #include <sys/wait.h>
> #include <err.h>
> #include <fcntl.h>
> #include <stdlib.h>
> #include <unistd.h>
> #define TARGET_FD STDIN_FILENO
> int main(int argc, char *argv[]) {
> int flags, status;
> pid_t pid;
> pid = fork();
> if (pid < 0) err(1, "fork failed");
> if (pid == 0) {
> flags = fcntl(TARGET_FD, F_GETFL, 0);
> if (flags == -1) err(1, "fcntl getfl failed");
> flags |= O_NONBLOCK;
> flags = fcntl(TARGET_FD, F_SETFL, flags);
> if (flags == -1) err(1, "fcntl setfl failed");
> argv++;
> execvp(*argv, argv);
> err(1, "execvp failed");
> }
> wait(&status);
> exit(0);
> }
>
> and then running whatever-the-above-was-compiled-to as
>
> ./whatever /usr/bin/ex
>
> or also under modern code that for some reason sets O_NONBLOCK and
> forgets to turn it off when calling out to an editor, hypothetically
>
> https://github.com/osa1/tiny
>
> and likely other, similar programs. Probably, O_NONBLOCK should be
> disabled on STDIN_FILENO before calling out to unknown programs.
> Probably, vi should be patched to not eat CPU when the previous case is
> not handled.
>
> Thoughts?

I think this is the wrong way around. The callers need to be fixed to pass
a blocking stdin to programs since that is what every unix utility
expects. What you propose it to fix every unix utility to have such a check
at the start of main. Sorry but no.

> diff --git usr.bin/vi/cl/cl_main.c usr.bin/vi/cl/cl_main.c
> index 33614c99594..f87a04cad8b 100644
> --- usr.bin/vi/cl/cl_main.c
> +++ usr.bin/vi/cl/cl_main.c
> @@ -54,7 +54,7 @@ main(int argc, char *argv[])
> CL_PRIVATE *clp;
> GS *gp;
> size_t rows, cols;
> - int rval;
> + int flags, oflags, rval;
> char *ttype;
>
> /* Create and initialize the global structure. */
> @@ -89,6 +89,14 @@ main(int argc, char *argv[])
> /* Ex wants stdout to be buffered. */
> (void)setvbuf(stdout, NULL, _IOFBF, 0);
>
> + /* Ensure blocking I/O to avoid 100% CPU on EAGAIN */
> + if ((flags = fcntl(STDIN_FILENO, F_GETFL, 0)) == -1)
> + exit (1);
> + oflags = flags;
> + flags &= ~O_NONBLOCK;
> + if (fcntl(STDIN_FILENO, F_SETFL, flags) == -1)
> + exit (1);
> +
> /* Start catching signals. */
> if (sig_init(gp, NULL))
> exit (1);
> @@ -102,6 +110,9 @@ main(int argc, char *argv[])
> /* Clean up the terminal. */
> (void)cl_quit(gp);
>
> + /* Restore flags */
> + fcntl(STDIN_FILENO, F_SETFL, oflags);
> +
> /*
> * XXX
> * Reset the O_MESG option.
>

--
:wq Claudio