From af65e06b3ba65952f1223e09b9df0736965bdaeb Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 4 Aug 2016 23:36:34 +0000 Subject: http: do not allow bad getline+close responses to kill us 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 fa34b443..729d46fb 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 222b9e01..ed1f92c0 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 5ecc69b5..1e8465c2 100644 --- a/t/httpd-corner.t +++ b/t/httpd-corner.t @@ -85,6 +85,30 @@ my $spawn_httpd = sub { is($body, "hello world\n", 'callback body matches expected'); } +{ + 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'; @@ -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; -- cgit v1.2.3-24-ge0c7