On Wed, Aug 10, 2022 at 09:45:44PM +0200, Omar Polo wrote:
> On 2022/08/10 15:07:15 +0200, Claudio Jeker wrote:
> > On Sun, Aug 07, 2022 at 11:10:22AM +0200, Omar Polo wrote:
> > > blob - ddf83f965d0e6a99ada695694bea77b775bae2aa
> > > blob + 1d577ba63efca388ca3644d1a52d9b3d9f246014
> > > --- usr.sbin/slowcgi/slowcgi.c
> > > +++ usr.sbin/slowcgi/slowcgi.c
> > > @@ -515,7 +515,34 @@ slowcgi_accept(int fd, short events, void *arg)
> > > void
> > > slowcgi_timeout(int fd, short events, void *arg)
> > > {
> > > - cleanup_request((struct request*) arg);
> > > + struct request *c;
> > > +
> > > + c = arg;
> > > +
> > > + if (c->script_flags & (STDOUT_DONE | STDERR_DONE | SCRIPT_DONE))
> >
> > Is this the correct check? If we hit the timeout we want to close all
> > pipes to the CGI process and hope it dies because of that. Now one of
> > STDOUT_DONE or STDERR_DONE may have happened but the script is still
> > running.
> >
> > I think it would be better to use:
> > if (c->script_flags == (STDOUT_DONE | STDERR_DONE | SCRIPT_DONE))
> > return;
> > here.
>
> Woops! Yes, I meant to use == and not & here.
>
> > Should slowcgi kill the command if SCRIPT_DONE is not set?
>
> RFC 3875 says that a script should be prepared to handle abnormal
> termination and tha the server acn interrupt or terminate it at any
> time and without warning, so killing the script is a possibility.
>
> However, I'm a bit worried. We could hit the timer not because of a
> faulty script but because an HTTP client is purposefully reading data
> slowly. This could even allow to kill the scripts at specific points.
> Maybe I'm overthinking and it's not an issue.
The question I have is if scripts properly notice the IO error and
terminate. If a script got stuck somehow and just sits there it will not
realize it got killed. If that process exits at a later point slowcgi will
be confused about a spurious SIGCHLD.
What would happen if this instead sends a SIGTERM and after 5sec timeout a
SIGKILL to the process? It should result in a proper close of the session
via the usual channels (signal handler, io handler). Isn't that a better
way of handling a timeout?
> Anyway, here's an updated version of the diff for slowcgi with the
> fixed logic.
>
> diff refs/remotes/origin/master refs/heads/wip
> commit - 3f5af05ce0a79edb8ab7606a968e2ff2617f1972
> commit + acc148801b858e3177b9b43a93d20fd53f97d40d
> blob - 9a0f8b5656d10fd09e62691a5ca827d357f1ff7a
> blob + 77e370c99d0d26646e699a285665f90323411444
> --- usr.sbin/slowcgi/slowcgi.c
> +++ usr.sbin/slowcgi/slowcgi.c
> @@ -515,7 +515,34 @@ slowcgi_accept(int fd, short events, void *arg)
> void
> slowcgi_timeout(int fd, short events, void *arg)
> {
> - cleanup_request((struct request*) arg);
> + struct request *c;
> +
> + c = arg;
> +
> + if (c->script_flags == (STDOUT_DONE | STDERR_DONE | SCRIPT_DONE))
> + return;
> +
> + if (!c->stdout_fd_closed) {
> + c->stdout_fd_closed = 1;
> + close(EVENT_FD(&c->script_ev));
> + event_del(&c->script_ev);
> + }
> +
> + if (!c->stderr_fd_closed) {
> + c->stderr_fd_closed = 1;
> + close(EVENT_FD(&c->script_err_ev));
> + event_del(&c->script_err_ev);
> + }
> +
> + if (!c->stdin_fd_closed) {
> + c->stdin_fd_closed = 1;
> + close(EVENT_FD(&c->script_stdin_ev));
> + event_del(&c->script_stdin_ev);
> + }
> +
> + c->script_status = SIGALRM;
> + c->script_flags = STDOUT_DONE | STDERR_DONE | SCRIPT_DONE;
> + create_end_record(c);
> }
>
> void
>
--
:wq Claudio
