Re: Question about cgi.rb's read_multipart method and possible fix

Zev Blut wrote:

> Hello,
>
> My colleagues and I have discovered a potential problem with the
> implementation of the CGI library's read_multipart method.  The
> current CGI implementation will create a temporary file for many of
> the multipart data entries if the posted content length is greater
> than 10 kilobytes.
>
> I would like to ask what is the intended goal of the implementation?
> Is the goal to make temporary files for each data entry that is more
> than 10 kilobytes and keep entries that are less in StringIO objects?
>
> The reason I ask is that the current implementation works in a
> different way.  It will create temporary files for each data entry
> until the remaining content data stream is less than 10 kilobytes.
> Then it will use StringIO for the remaining entries.
>
> This implementation turns out to have disastrous consequences for some
> of my applications.  We have some situations where we send images and
> many select options in a post.  This ends up creating hundreds and
> sometimes thousands of Tempfiles, which causes our servers to become
> unresponsive.
>
> I have modified read_multipart to only create TempFiles for each data
> entry greater than 10 kilobytes.  With this modification our servers
> are responsive and show no other problems with my change.
>
> I have made a few other changes to the implementation to use sub!
> method calls to hopefully remove unneeded duplication of large buffers.
> I dug through the CVS history and it appears it was originally using
> such a sub! call, but later when to "buf = buf.sub" calls, is there a
> reason why?  If so I will revert back to "buf = buf.sub".
>
> Ignoring a few minor style changes in my implementation and or my
> implementation all together, the main point of discussion I would like
> to start is if the current 10 kilobyte implementation is correct of if
> the intended goal of my modification is correct?  If my modification
> is not too offensive I can integrate it with cgi.rb and post a patch.
>
> Best Regards,
> Zev Blut

Hi Zev,

I've had some big headaches when uploading files in a Rails app of mine. 
I'm not sure if my issue is related to what you're seeing, but I figured 
I would describe my issue just in case. My issue occurs on a form that 
has many form elements and possibly many file uploads (max is about 
20).  I run my Rails app under FastCGI, and every now and again a 
FCGI::Stream::ProtocolError will be thrown when processing a file upload 
that will leave my app unresponsive after this. I have to manually kill 
the fcgi process that the Exception was thrown from, since it seems to 
go zombie afterward. Here's what the exception trace looks like, it 
seems to originate in the read_multipart method in cgi.rb which is why I 
figured this may be related:

[Wed Jul 06 15:35:46 EDT 2005] Dispatcher failed to catch: protocol 
error (FCGI::Stream::ProtocolError)
  /opt/local/lib/ruby/1.8/cgi.rb:1015:in `read'
  /opt/local/lib/ruby/1.8/cgi.rb:1015:in `read_multipart'
  /opt/local/lib/ruby/1.8/cgi.rb:983:in `loop'
  /opt/local/lib/ruby/1.8/cgi.rb:983:in `read_multipart'
  
/opt/local/lib/ruby/gems/1.8/gems/actionpack-1.8.1/lib/action_controller/cgi_ext/raw_post_data_fix.rb:11:in 
`initialize_query'
  /opt/local/lib/ruby/1.8/cgi.rb:2269:in `initialize'
  (eval):16:in `initialize'
  /opt/local/lib/ruby/gems/1.8/gems/fcgi-0.8.6.1/./fcgi.rb:600:in `new'
  /opt/local/lib/ruby/gems/1.8/gems/fcgi-0.8.6.1/./fcgi.rb:600:in `each_cgi'
  /opt/local/lib/ruby/gems/1.8/gems/fcgi-0.8.6.1/./fcgi.rb:597:in `each'
  /opt/local/lib/ruby/gems/1.8/gems/fcgi-0.8.6.1/./fcgi.rb:597:in `each_cgi'
  /Library/WebServer/Documents/intranet/dispatch.fcgi:18
FCGI process 18352 killed by this error

On a side note, my /tmp dir usually has a bunch of TempFiles laying 
around in it, that I manually clean up from time to time. Are TempFiles 
supposed to stick around after an upload is completed? Maybe they never 
get cleaned up when this Exception is thrown.

Cheers,
Gianni