Skip to content

Loading…

Wrong "sysread" return code processing in Starman::Server::_read_headers #56

Closed
fsitedev opened this Issue · 5 comments

3 participants

@fsitedev

If we send empty request to starman, we'v got an error like this "Read error: ".
It's because of "|| $read == 0" section after syscall.
Empty request is not an error, it doesn't set $! variable.
Sysread manual:
..Returns the number of bytes actually read, 0 at end of file, or undef if there was an error (in the latter case $! is also set)...

eval {
    local $SIG{ALRM} = sub { die "Timed out\n"; };

    alarm( READ_TIMEOUT );

    while (1) {
        # Do we have a full header in the buffer?
        # This is before sysread so we don't read if we have a pipelined request
        # waiting in the buffer
        last if defined $self->{client}->{inputbuf} && $self->{client}->{inputbuf} =~ /$CRLF$CRLF/s;

        # If not, read some data
        my $read = sysread $self->{server}->{client}, my $buf, CHUNKSIZE;

        if ( !defined $read || $read == 0 ) {
            die "Read error: $!\n";
        }

        if ( DEBUG ) {
            warn "[$$] Read $read bytes: " . dump($buf) . "\n";
        }

        $self->{client}->{inputbuf} .= $buf;
    }
};
@miyagawa
Owner
@fsitedev

While writing a test case i have realiazed that problem is not only in sysread, but also in strict CRLF checking.

Starting starman in debug mode:
export STARMAN_DEBUG=1; starman -e 'sub { my $env = shift; return [200 => ["Content-Type: text/html\n"], ["body"]] }'

2012/12/12-11:34:24 Starman::Server (type Net::Server::PreFork) starting! pid(3093)
Resolved []:5000 to [0.0.0.0]:5000, IPv4
Host [
] resolved to IPv6 address [::] but IO::Socket::INET6->new fails: Address family not supported by protocol at /usr/local/perl-5.14.2/lib/site_perl/5.14.2/Net/Server/Proto.pm line 133.
Binding to TCP port 5000 on host 0.0.0.0 with IPv4
Setting gid to "30328 30328 496 501 30328 30489 30490"
Setting up serialization via flock
Beginning prefork (5 processes)
Starting "5" children
Child Preforked (3103)
[3103] Initializing the PSGI app
Parent ready for children.
Child Preforked (3104)
[3104] Initializing the PSGI app
Child Preforked (3107)
[3107] Initializing the PSGI app
Child Preforked (3106)
[3106] Initializing the PSGI app
Child Preforked (3105)
[3105] Initializing the PSGI app

Sending empty request:
perl -e 'use Socket; my $p = sockaddr_in(5000, inet_aton("127.0.0.1")); socket(SOCK, PF_INET, SOCK_STREAM, getprotobyname("tcp")); connect(SOCK, $p) or die $!; printf "Bytes sent: %d\n", send(SOCK, "", 0, $p);'

We got error:

2012/12/12-11:35:00 CONNECT TCP Peer: "[127.0.0.1]:45097" Local: "[127.0.0.1]:5000"
[3104] Read error:
[3104] Closing connection

I have expected "Bad request" in debug, but got only "Read error" with empty $!.
Moreover i see "Read error" in requests like this:
send(SOCK, "GET / HTTP/1.0\n\n", 0, $p)

Probably the true problem is not only in sysread's return value checking, but also in strict CRLF checking while parsing headers.
Here is a quote from HTTP 1.1 spec:

The line terminator for message-header fields is the sequence CRLF. However, we recommend that applications, when parsing such headers, recognize a single LF as a line terminator and ignore the leading CR.

In my case starman works as a backend-server, processing requests from nginx (working in reverse proxy mode).
Nginx (and other web servers, ex. apache), follows above recommendation and doesn't drop requests without leading CR.
In fact there is a huge number of different crawlers, robots, bots etc, that do not follow http spec and send only "\n". It would be great if starman could process both CRLF and LF line terminators.

Thanks.

@miyagawa
Owner

Starman is intended to be used in the backend behind frontends like nginx (like your case), so it's arguable whether it should support a broken client like such bots. It makes sense to not at least crash in such situations though.

@fsitedev

In perfect case it would be great to have starman supporting such recommendations, just because it's a HTTP server in the first place. This will make starman's behaviour more like an apache and nginx. Migration to starman will become more transparent particularly for complex and highly loaded projects where even LF-terminator does matter.

However $read checking after sysread is still incorrect and likely can be fixed in rather simple manner:

if ( !defined $read ) {
    die "Read error: $!\n";
}
elsif ( $read == 0 ) {
    last;
}

Thanks.

@autarch

The line ending issue has become a problem for us at MaxMind too. We are moving our web services our to Web::Machine on top of Starman. We have thousands of clients, and some of them are apparently sending requests with just NL as the line ending. We do have Starman behind haproxy, but that doesn't help. Apparently haproxy doesn't rewrite the entire request. I can't imagine we're the only ones wanting to use Starman behind haproxy.

I suspect this could be fixed fairly simply by looking for /\r?\n/ instead of /\r\n/. Would you take a patch for this?

@miyagawa miyagawa closed this in 24f7160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.