[Catalyst] Re: Not cleaning up temporary files / HTTP::Body

[Bill Moseley]

Just to be clear, this opens up all Catalyst apps to a pretty easy DoS --
or in our case denial of sleep as people were woken up in the middle of the
night as servers ran low on disk space.   Good thing we have monitoring set
up.

The risk to any individual Catalyst user is obviously limited, but
something to be aware of.

The design flaw here means Catalyst apps, by default, give write acces to
/tmp (which often isn't that big) to any user of your app and doesn't clean
up after itself.   Running out of /tmp space is never fun.

HTTP::Body should be fixed because of this issue, and I've created a ticket
against it.  A fix there could be done w/o any changes needed for Catalyst.

But, there's other things to consider.   For example, there's nothing to
limit the size of a single request.  Some of those tests should be higher
up in the stack.

Apache has a setting "LimitRequestBody" but IIRC that only looks at the
Content-Length header.  But, HTTP::Body does limit the body to what is
reported in the Content-Length header.

Starman buffers requests to /tmp (in an unlinked file) w/o any limit that I
could see in my limited tests.




On Thu, Mar 14, 2013 at 9:53 PM, Bill Moseley <moseley at hank.org> wrote:

>
>
> On Wed, Jan 13, 2010 at 6:53 AM, Bill Moseley <moseley at hank.org> wrote:
>
>> HTTP::Body::Multipart creates temporary files for uploads.  The temp
>> files are created with File::Temp( UNLINK =3D> 0 ).
>>
>
> Well, this is still broken.
>
> Yes, since 2010 HTTP::Body has been updated to have a DESTROY where it
> removes files, and Catalyst::Request calls the $body->cleanup(1) attribute
> to enable this feature.
>
> But, the problem is DESTROY (see below) looks for files to remove in
> $self->{upload} hash.  But, that is an empty hash until the upload is
> completed.  So, if the upload is aborted (which is when you need the clean
> up to work) there's nothing in the upload hash -- so nothing to unlink.
>
> Three years ago when I first posted about this I copied
> HTTP::Body::MultiPart and set UNLINK =3D> 1 on  File::Temp->new and creat=
ed
> my own version that overrides.    That still works.
>
> But, if I simply modify the current H::B::MultiPart and add UNLINK=3D>1 t=
hen
> the file gets deleted and I get an error. So, the fix isn't that simple a=
ny
> more.  That needs some looking at.
>
>
> Clearly, what we are after is deleting the temp files.  So, might as well
> let File::Temp do its job.  Just have to make sure the handle doesn't go
> out of scope too early.
>
>
> Oh, BTW, you can't test this with the dev server.  I had to use Apache.
>  With Apache chunks are sent to Catalyst.  But, when running on the dev
> server it seems like the entire upload was buffered before prepare_body
> gets called -- so even with a 16MB upload I could not hit "esc" to abort
> the upload while HTTP::Body was processing.
>
> sub DESTROY {
>     my $self =3D shift;
>
>     if ( $self->{cleanup} ) {
>         my @temps =3D ();
>         *for my $upload ( values %{ $self->{upload} } ) {*
>             push @temps, map { $_->{tempname} || () }
>                 ( ref $upload eq 'ARRAY' ? @{$upload} : $upload );
>         }
>         unlink map { $_ } grep { -e $_ } @temps;  ## this is empty when
> aborted
>     }
>
> }
>
>
>
>
>
>>
>> Catalyst then deletes these temp files in $c->finalize.  The problem is
>> that an exception can happen and then the temp files are not deleted.
>> Happens quite often, it turns out.  I seem to always see this in the logs
>> at the time of the orphaned temp file:
>>
>> Caught exception in engine "Apache2::RequestIO::read: (104) Connection
>> reset by peer
>>
>> Aborting an upload isn't that unusual.
>>
>> I can set temp directory for these uploads and use cron to clean them
>> out, but I wonder if they could not be cleaned up better automatically.
>>
>> For example, unlink in DESTROY when the request object goes out of
>> scope.  Or perhaps better, don't have HTTP::Body set UNLINK =3D> 1 by de=
fault
>> and when the upload object finally goes out of scope File::Temp will rem=
ove
>> the temp file. HTTP::Body explicitly removes the File::Temp object from =
the
>> upload part, but I'm not sure why it needs to do that.   Why not leave t=
he
>> File::Temp object in the upload part object then let it go out of scope =
at
>> the end of the request.
>>
>> Where should this be addressed?  In Catalyst or in HTTP::Body?
>>
>> --
>> Bill Moseley
>> moseley at hank.org
>>
>
>
>
> --
> Bill Moseley
> moseley at hank.org




-- =

Bill Moseley
moseley at hank.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.scsys.co.uk/pipermail/catalyst/attachments/20130317/01ad6=
dd1/attachment.htm