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.
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
