* [PATCH] http: do not allow bad getline+close responses to kill us
@ 2016-08-05 2:31 Eric Wong
0 siblings, 0 replies; only message in thread
From: Eric Wong @ 2016-08-05 2:31 UTC (permalink / raw)
To: meta
PSGI applications (like our WWW :P) can fail unpredictability,
but lets try to avoid bringing the entire process down when this
happens.
---
lib/PublicInbox/HTTP.pm | 73 +++++++++++++++++++++++++++++++++----------------
t/httpd-corner.psgi | 12 ++++++++
t/httpd-corner.t | 33 ++++++++++++++++++++++
3 files changed, 94 insertions(+), 24 deletions(-)
diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index fa34b44..729d46f 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -245,29 +245,49 @@ sub response_done ($$) {
$self->write(sub { $alive ? next_request($self) : $self->close });
}
+sub getline_cb ($$$) {
+ my ($self, $write, $close) = @_;
+ local $/ = \8192;
+ my $forward = $self->{forward};
+ # limit our own running time for fairness with other
+ # clients and to avoid buffering too much:
+ if ($forward) {
+ my $buf = eval { $forward->getline };
+ if (defined $buf) {
+ $write->($buf); # may close in Danga::Socket::write
+ unless ($self->{closed}) {
+ my $next = $self->{pull};
+ if ($self->{write_buf_size}) {
+ $self->write($next);
+ } else {
+ PublicInbox::EvCleanup::asap($next);
+ }
+ return;
+ }
+ } elsif ($@) {
+ err($self, "response ->getline error: $@");
+ $forward = undef;
+ $self->close;
+ }
+ }
+
+ $self->{forward} = $self->{pull} = undef;
+ # avoid recursion
+ if ($forward) {
+ eval { $forward->close };
+ if ($@) {
+ err($self, "response ->close error: $@");
+ $self->close; # idempotent
+ }
+ }
+ $close->();
+}
+
sub getline_response {
my ($self, $body, $write, $close) = @_;
$self->{forward} = $body;
weaken($self);
- my $pull = $self->{pull} = sub {
- local $/ = \8192;
- my $forward = $self->{forward};
- # limit our own running time for fairness with other
- # clients and to avoid buffering too much:
- while ($forward && defined(my $buf = $forward->getline)) {
- $write->($buf);
- last if $self->{closed};
- if ($self->{write_buf_size}) {
- $self->write($self->{pull});
- } else {
- PublicInbox::EvCleanup::asap($self->{pull});
- }
- return;
- }
- $self->{forward} = $self->{pull} = undef;
- $forward->close if $forward; # avoid recursion
- $close->();
- };
+ my $pull = $self->{pull} = sub { getline_cb($self, $write, $close) };
$pull->();
}
@@ -331,12 +351,15 @@ sub input_prepare {
sub env_chunked { ($_[0]->{HTTP_TRANSFER_ENCODING} || '') =~ /\bchunked\b/i }
+sub err ($$) {
+ eval { $_[0]->{httpd}->{env}->{'psgi.errors'}->print($_[1]."\n") };
+}
+
sub write_err {
my ($self, $len) = @_;
- my $err = $self->{httpd}->{env}->{'psgi.errors'};
my $msg = $! || '(zero write)';
$msg .= " ($len bytes remaining)" if defined $len;
- $err->print("error buffering to input: $msg\n");
+ err($self, "error buffering to input: $msg");
quit($self, 500);
}
@@ -347,8 +370,7 @@ sub recv_err {
$self->{input_left} = $len;
return;
}
- my $err = $self->{httpd}->{env}->{'psgi.errors'};
- $err->print("error reading for input: $! ($len bytes remaining)\n");
+ err($self, "error reading for input: $! ($len bytes remaining)");
quit($self, 500);
}
@@ -451,7 +473,10 @@ sub close {
my $env = $self->{env};
delete $env->{'psgix.io'} if $env; # prevent circular referernces
$self->{pull} = $self->{forward} = $self->{env} = undef;
- $forward->close if $forward;
+ if ($forward) {
+ eval { $forward->close };
+ err($self, "forward ->close error: $@") if $@;
+ }
$self->SUPER::close(@_);
}
diff --git a/t/httpd-corner.psgi b/t/httpd-corner.psgi
index 222b9e0..ed1f92c 100644
--- a/t/httpd-corner.psgi
+++ b/t/httpd-corner.psgi
@@ -60,6 +60,18 @@ my $app = sub {
}
} elsif ($path eq '/empty') {
$code = 200;
+ } elsif ($path eq '/getline-die') {
+ $code = 200;
+ $body = Plack::Util::inline_object(
+ getline => sub { die 'GETLINE FAIL' },
+ close => sub { die 'CLOSE FAIL' },
+ );
+ } elsif ($path eq '/close-die') {
+ $code = 200;
+ $body = Plack::Util::inline_object(
+ getline => sub { undef },
+ close => sub { die 'CLOSE FAIL' },
+ );
}
[ $code, $h, $body ]
diff --git a/t/httpd-corner.t b/t/httpd-corner.t
index 5ecc69b..1e8465c 100644
--- a/t/httpd-corner.t
+++ b/t/httpd-corner.t
@@ -86,6 +86,30 @@ my $spawn_httpd = sub {
}
{
+ my $conn = conn_for($sock, 'getline-die');
+ $conn->write("GET /getline-die HTTP/1.1\r\nHost: example.com\r\n\r\n");
+ ok($conn->read(my $buf, 8192), 'read some response');
+ like($buf, qr!HTTP/1\.1 200\b[^\r]*\r\n!, 'got some sort of header');
+ is($conn->read(my $nil, 8192), 0, 'read EOF');
+ $conn = undef;
+ my $after = capture($err);
+ is(scalar(grep(/GETLINE FAIL/, @$after)), 1, 'failure logged');
+ is(scalar(grep(/CLOSE FAIL/, @$after)), 1, 'body->close not called');
+}
+
+{
+ my $conn = conn_for($sock, 'close-die');
+ $conn->write("GET /close-die HTTP/1.1\r\nHost: example.com\r\n\r\n");
+ ok($conn->read(my $buf, 8192), 'read some response');
+ like($buf, qr!HTTP/1\.1 200\b[^\r]*\r\n!, 'got some sort of header');
+ is($conn->read(my $nil, 8192), 0, 'read EOF');
+ $conn = undef;
+ my $after = capture($err);
+ is(scalar(grep(/GETLINE FAIL/, @$after)), 0, 'getline not failed');
+ is(scalar(grep(/CLOSE FAIL/, @$after)), 1, 'body->close not called');
+}
+
+{
my $conn = conn_for($sock, 'excessive header');
$SIG{PIPE} = 'IGNORE';
$conn->write("GET /callback HTTP/1.0\r\n");
@@ -489,4 +513,13 @@ SKIP: {
done_testing();
+sub capture {
+ my ($f) = @_;
+ open my $fh, '+<', $f or die "failed to open $f: $!\n";
+ local $/ = "\n";
+ my @r = <$fh>;
+ truncate($fh, 0) or die "truncate failed on $f: $!\n";
+ \@r
+}
+
1;
--
EW
^ permalink raw reply related [flat|nested] only message in thread
only message in thread, other threads:[~2016-08-05 2:31 UTC | newest]
Thread overview: (only message) (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-05 2:31 [PATCH] http: do not allow bad getline+close responses to kill us Eric Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).