Die in engine if prepare_body writing to psgi.input fails.

[billmoseley]

This is a partial fix for catching errors when writing to the pgsi.input file. Will die if there's an error on write. This is to address the potential DoS by filling up the temp partition blocking all other requests with a body.

I say "partial" because this also needs to be fixed in HTTP::Body, where it also fails to check for errors.

[billmoseley]

BTW -- here's the ticket (which include a patch) for the HTTP::Body code:
https://rt.cpan.org/Ticket/Display.html?id=105021

[jjn1056]

Hey so sorry it took time but I finally was able to take a look. I'm a bit worried about the ->print || ... bit. In looking at the source for Stream::Buffer (and http://cpansearch.perl.org/src/GBARR/IO-1.25/lib/IO/Handle.pm which defines print for file cases) its not clear to me that we can rely on the return value here as a way to determine if something is going wrong or not (print doesn't see to define a return value, its just implicit in the body of the subroutine but the docs don't define it and that could change in the future). I find it possible that this would die in cases when the file was fine.

Also, FWIW quite often by the time we get to Catalyst its too late to deal with this, since many Plack handlers (Starman, for example) pre buffer the input. That's actually on a task list for me but it requires changes to Plack and that is not easy to get agreement on. I'm going to leave this one on hold for me. Please let me know if you think my review is incorrect.

[billmoseley]

Hi @jjn1056, do you mean this code does not define a return value?

sub print {
    @_ or croak 'usage: $io->print(ARGS)';
    my $this = shift;
    print $this @_;
}

Well, it is returning what print returns, but I suppose it should say "return print $this @_" and document it. But, print() isn't even documented. printflush is documented to return the value of print, so I see the expectation is there. Seems very unlikely that the code would change to not return the value of print in the future since other methods that print are documented to do so.

Obviously, the code should be checking the return values of system calls (thank Perl Critic), and it would be best to catch the actual error (e.g. out of disk space) at the time of print than later as was happening in my case.

This code is making a copy of the input stream so the input stream still exists after HTTP::Body processing. So, not following your comment about Plack buffering.

BTW -- it would be nice to be able to disable this buffering if you don't really need the stream in the request. Why write it out to a temporary file (along with what HTTP::Body creates) if it's never going to be used? Does anything use that now?

[jjn1056]

I'm going to merge this, thanks!

regarding the 'disable buffering' that would be really great but I can't get traction in the plack world on it, at last for now. :(

[jjn1056]

Hey,

Catching up a bit. So yeah we'd like a version of Plack that made streaming buffering optional, sadly I'm having a hard time getting traction.