ruby-core

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

Attachments (1)

CGIFIX.rb (3.75 KB, text/x-ruby)

# This is a quick fix to change CGI's read_mutipart method to not
# create lots of small Tempfiles.  It will now only create Tempfiles
# for data entries that are larger than 10KB.

class CGI

  module QueryExtension

    def read_multipart(boundary, content_length)
      params = Hash.new() { |h,k| h[k] = Array.new }
      boundary = "--" + boundary
      buf = ""
      bufsize = 10240 # 10KB

      # start multipart/form-data
      stdinput.binmode if defined? stdinput.binmode
      boundary_size = boundary.size + EOL.size
      content_length -= boundary_size

      status = stdinput.read(boundary_size)
      if nil == status
        raise EOFError, "no content body"
      elsif boundary + EOL != status
        raise EOFError, "bad content body"
      end

      loop do
        head = nil
        body = MorphingBody.new

        until head and /#{boundary}(?:#{EOL}|--)/n.match(buf)

          if (not head) and /#{EOL}#{EOL}/n.match(buf)
            buf.sub!(/\A((?:.|\n)*?#{EOL})#{EOL}/n) do
              head = $1.dup
              ""
            end
            next
          end

          if head and ( (EOL + boundary + EOL).size < buf.size )
            body.print buf[0 ... (buf.size - (EOL + boundary + EOL).size)]
            buf[0 ... (buf.size - (EOL + boundary + EOL).size)] = ""
          end

          # should be c = if, but changed for broken emacs indent
          if bufsize < content_length
            c = stdinput.read(bufsize)
          else
            c = stdinput.read(content_length)
          end

          if c.nil?
            raise EOFError, "bad content body"
          end
          buf.concat(c)
          content_length -= c.size
        end

        # change to ! to remove excessive object creation
        buf.sub!(/\A((?:.|\n)*?)(?:[\r\n]{1,2})?#{boundary}([\r\n]{1,2}|--)/n) do
          body.print $1
          if "--" == $2
            content_length = -1
          end
          ""
        end

        body = body.extract
        body.rewind

        /Content-Disposition:.* filename="?([^\";]*)"?/ni.match(head)
        filename = ($1 or "")
        if /Mac/ni.match(env_table['HTTP_USER_AGENT']) and
            /Mozilla/ni.match(env_table['HTTP_USER_AGENT']) and
            (not /MSIE/ni.match(env_table['HTTP_USER_AGENT']))
          filename = CGI::unescape(filename)
        end

        /Content-Type: (.*)/ni.match(head)
        content_type = ($1 or "")

        (class << body; self; end).class_eval do
          alias local_path path
          define_method(:original_filename) {filename.dup.taint}
          define_method(:content_type) {content_type.dup.taint}
        end

        /Content-Disposition:.* name="?([^\";]*)"?/ni.match(head)
        name = $1.dup

        params[name].push(body)
        break if buf.size == 0
        break if content_length == -1 # removed === ?
      end

      params
    end # read_multipart

    require "stringio"
    require "tempfile"

    # A wrapper class to use a StringIO object as the body and switch
    # to a TempFile when the passed threshold is passed.
    class MorphingBody

      def initialize(morph_threshold = 10240)
        @threshold = morph_threshold
        @body = StringIO.new
        @cur_size = 0
        @morph_check = true

        @body.binmode if defined? @body.binmode
      end

      def print(data)
        if @morph_check && (@cur_size + data.size > @threshold)
          convert_body
        end
        @body.print data
      end

      # returns the true body object.
      def extract
        @body
      end

      private
      def convert_body
        new_body = TempFile.new("CGI")
        new_body.binmode if defined? new_body.binmode

        @body.rewind
        new_body.print @body.read
        @body = new_body
        @morph_check = false
      end
    end

  end

end

Thread

Prev Next

In This Thread

Prev Next

[#5711] Lexic confusion: method/local variable distinction works strange — noreply@...

[#5721] uninitialized constant REXML::Parsers::SAX2Parser::Text — noreply@...

[#5724] SystemExit & abort_on_exception in latest "ruby_1_8" — Johan Holmberg <holmberg@...>

[#5727] executing linux shell commands with ruby?? — Pranith Kumar Karampuri <praneeth147@...>

[#5729] Valgrind support — Tilman Sauerbeck <tilman@...>

[#5731] Ruby development issue tracking will go to basecamp — Yukihiro Matsumoto <matz@...>

[#5732] Re: Ruby development issue tracking will go to basecamp — ville.mattila@...

[#5737] returning strings from methods/instance_methods — TRANS <transfire@...>

[#5747] YAML.load hang-up — Kazuhiro NISHIYAMA <zn@...>

[#5750] File.split edge cases — "Berger, Daniel" <Daniel.Berger@...>

[#5752] assert_raises — Eric Hodel <drbrain@...7.net>

[#5753] Wilderness: AutoLoad Feature (variable.c)- Need General Description — "Charles E. Thornton" <ruby-core@...>

[#5766] Re: File.split edge cases — "Berger, Daniel" <Daniel.Berger@...>

[#5768] Wilderness: AutoLoad Feature (variable.c)- Need General Description — "Charles E. Thornton" <ruby-core@...>

[#5774] YAML broken — Kazuhiro NISHIYAMA <zn@...>

[#5779] Strange 'ambiguous arguments' warning — Nathaniel Talbott <ntalbott@...>

[#5781] array sharing — Eric Mahurin <eric_mahurin@...>

[#5784] Documentation for 1.8.3 — Eric Hodel <drbrain@...7.net>

[#5786] Difference between class declarations — Peter Vanbroekhoven <calamitas@...>

[#5787] Wilderness: How does __autoload__ work?? — "Charles E. Thornton" <ruby-core@...>

[#5793] syck.so compile error — URABE Shyouhei <s-urabe@...>

[#5796] proposed attr writer patch — Daniel Berger <Daniel.Berger@...>

[#5797] Re: proposed attr writer patch — "Berger, Daniel" <Daniel.Berger@...>

[#5798] Makefile error in OpenSLL extension (on Windows) — noreply@...

[#5804] Wilderness: Is there any way to invoke rb_define_class() from the Ruby Level — "Charles E. Thornton" <ruby-core@...>

[#5806] 5**(-1/2) gives incorrect results under mathn. — C Erler <erlercw@...>

[#5807] Wliderness: Ruby Code to force AUTOLOAD sequence to run — "Charles E. Thornton" <ruby-core@...>

[#5818] Weird SIGCHLD-related problem — Ben Schumacher <benschumacher@...>

[#5820] patch for O(1) performance for Array#unshift and Array#shift — Eric Mahurin <eric_mahurin@...>

[#5826] YAML bug - dump string terminating with ':' — "H.Yamamoto" <ocean@...2.ccsnet.ne.jp>

[#5837] Re: RubyGems in Ruby HEAD — TRANS <transfire@...>

[#5838] Re: RubyGems in Ruby HEAD — David Heinemeier Hansson <david.heinemeier@...>

[#5839] Forward: [ruby-dev:27237] Re: yaml warnig in verbose mode — nobu.nokada@...

[#5840] Re: RubyGems in Ruby HEAD — Aredridel <aredridel@...>

[#5842] Re: RubyGems in Ruby HEAD — Eric Hodel <drbrain@...7.net>

[#5844] Ruby 1.8.3 released — Yukihiro Matsumoto <matz@...>

[#5848] Re: RubyGems in Ruby HEAD — Hugh Sasse <hgs@...>

[#5851] Re: RubyGems in Ruby HEAD — Paul van Tilburg <paul@...>

[#5854] Warnings in YAML — noreply@...

[#5858] FileUtils#cd doesn't allow :noop in options anymore, but still queries it — noreply@...

[#5861] another array patch - performance boosts all over the place — Eric Mahurin <eric_mahurin@...>

[#5863] Re: another array patch - performance boosts all over the place — "Berger, Daniel" <Daniel.Berger@...>

[#5871] Wilderness: What is ID_JUNK Used for? — "Charles E. Thornton" <ruby-core@...>

[#5879] Re: RubyGems TODO — Austin Ziegler <halostatue@...>

[#5882] Re: RubyGems TODO — Austin Ziegler <halostatue@...>

[#5888] Re: RubyGems TODO — Mauricio Fern疣dez <mfp@...>

[#5896] Re: RubyGems TODO — Tom Copeland <tom@...>

[#5898] Delegate and Forwardable Documentation — James Edward Gray II <james@...>

[#5899] Re: Delegate and Forwardable Documentation — "Berger, Daniel" <Daniel.Berger@...>

[#5901] Re: RubyGems TODO — "Jim Weirich" <jim@...>

[#5902] Vulnerability fixed in 1.8.3 — Yukihiro Matsumoto <matz@...>

[#5905] The standard library "openssl" does not get built/installed in ruby 1.8.3. — noreply@...

[#5906] Re: RubyGems TODO — Daigo Moriwaki <techml@...>

[#5910] 1.8.3, pleasant surprise. — Hugh Sasse <hgs@...>

[#5921] Mutually dependent libs double loading. — TRANS <transfire@...>

[#5932] patch for imap.rb -- error handling when waiting for continuation request response — noreply@...

[#5933] — "Andreas Farre" <d00farre@...>

[#5954] FileUtils::Verbose.{compare_file,uptodate?} are private in Ruby 1.8.3 — noreply@...

[#5955] Re: RubyGems TODO (the Second) — TRANS <transfire@...>

[#5966] $SAFE=4 is still dangerous to use as a sandbox — URABE Shyouhei <s-urabe@...>

[#5970] [BUG] unknown node type 0 -- on Linux, AMD Athlon 900, Ruby 1.8.3 — noreply@...

[#5975] segmentation fault on require 'yaml' — Ralph Amissah <ralph.amissah@...>

[#5976] basecamp RSS feed requires username and passwd — sheepman <sheepman@...>

[#5984] Wilderness: How to convert from Klass/Super References to Name — "Charles E. Thornton" <ruby-core@...>

[#5985] Finally an answer to my RubyGems question and some small suggestions — TRANS <transfire@...>

[#5992] New Ruby Mirror Ready — "Scott B." <scott@...>

[#5999] ruby 1.8.3 (2005-06-23) [i486-linux] sisu segfault — Ralph Amissah <ralph.amissah@...>

[#6001] Require Namepaces and RubyGems' effect on LoadPath problem — TRANS <transfire@...>

[#6004] Problem with 1.8.3, extensions — Daniel Berger <Daniel.Berger@...>

[#6009] Re: ruby 1.8.3 (2005-09-21) [i486-linux] sisu segfault — Ralph Amissah <ralph.amissah@...>

[#6021] $\ not escaped in IO#print documentation — noreply@...

[#6031] Undefined method error in net/http.rb — noreply@...

[#6038] make warning from 1.8.3 — Daniel Berger <Daniel.Berger@...>

[#6053] _'s not escaped in Object#send documentation — noreply@...

[#6057] YAML loading of quoted Symbols broken in 1.8.3 — noreply@...

[#6070] Ads by gooooooooogle on Ruby Home Page... — Austin Ziegler <halostatue@...>

[#6076] Question about cgi.rb's read_multipart method and possible fix — "Zev Blut" <rubyzbibd@...>

[#6089] irb segfault — "David A. Black" <dblack@...>

[#6108] Re: [sydney-devel] Threading Performance? — Evan Webb <evanwebb@...>

[#6109] Fix a warning in Syck — Tilman Sauerbeck <tilman@...>

[#5787] Wilderness: How does autoload work?? — "Charles E. Thornton" <ruby-core@...>