about summary refs log tree commit homepage
path: root/lib/PublicInbox
diff options
context:
space:
mode:
authorEric Wong <e@80x24.org>2023-11-25 20:54:33 +0000
committerEric Wong <e@80x24.org>2023-11-26 01:05:24 +0000
commitdb7e689902f7ea0b0f9188d14a28ce57fb9f221c (patch)
tree9316e8525adfa7e632f3f67fad46f38068f90075 /lib/PublicInbox
parent5073e8eceaf0ad20ddf9a5515e1d0aa7acbad5b3 (diff)
downloadpublic-inbox-db7e689902f7ea0b0f9188d14a28ce57fb9f221c.tar.gz
We must not attempt to read request bodies from the HTTP client
while processing a long request since that drains pipelined
requests.  The NNTP/IMAP/POP3 event_step callbacks follow the
same behavior when {long_cb} is present from ->long_response.

This bug has little real-world consequence since HTTP/1.1
pipelining is not widely-used, especially when behind varnish
or other reverse proxies.

I found this bug while randomly strace-ing an active -netd
process to see the kind of traffic it was seeing.
Diffstat (limited to 'lib/PublicInbox')
-rw-r--r--lib/PublicInbox/HTTP.pm17
1 files changed, 8 insertions, 9 deletions
diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index 85991ae7..7162732e 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -76,7 +76,7 @@ sub new ($$$) {
 sub event_step { # called by PublicInbox::DS
         my ($self) = @_;
         local $SIG{__WARN__} = $self->{srv_env}->{'pi-httpd.warn_cb'};
-        return unless $self->flush_write && $self->{sock};
+        return unless $self->flush_write && $self->{sock} && !$self->{forward};
 
         # only read more requests if we've drained the write buffer,
         # otherwise we can be buffering infinitely w/o backpressure
@@ -230,6 +230,13 @@ sub identity_write ($$) {
 
 sub response_done {
         my ($self, $alive) = @_;
+        if (my $forward = delete $self->{forward}) { # avoid recursion
+                eval { $forward->close };
+                if ($@) {
+                        warn "response forward->close error: $@";
+                        return $self->close; # idempotent
+                }
+        }
         delete $self->{env}; # we're no longer busy
         # HEAD requests set $alive = 3 so we don't send "0\r\n\r\n";
         $self->write(\"0\r\n\r\n") if $alive == 2;
@@ -268,14 +275,6 @@ sub getline_pull {
                 warn "response ->getline error: $@";
                 $self->close;
         }
-        # avoid recursion
-        if (delete $self->{forward}) {
-                eval { $forward->close };
-                if ($@) {
-                        warn "response ->close error: $@";
-                        $self->close; # idempotent
-                }
-        }
         response_done($self, delete $self->{alive});
 }