This project is archived and is in readonly mode.

#611 ✓resolved
azimux

cannot write certain binary data to postgresql bytea columns in 2.1.0

Reported by azimux | July 13th, 2008 @ 11:51 AM | in 2.x

The code in postgresql_adapter for binary_to_string checks the input against /\\\d{3}/ to determine if it's blob text or not.

This doesn't make sense because blob text could not have any octal strings in it, and likewise, non blob encoded binary data can easily have coincidental octal codes in it. For example, the attatched file gives a false positive.

If you upload it in a multipart form and try to store it into the database into a binary column, you will encounter a bug with this trace:

can't convert Fixnum into String

RAILS_ROOT: C:/Users/miles/Documents/svnlocal/RubyMaps/trunk/RubyMaps

Application Trace | Framework Trace | Full Trace

c:/ruby/lib/ruby/gems/1.8/gems/activerecord-2.1.0/lib/active_record/connection_adapters/postgresql_adapter.rb:116:in `<<' </p><p> c:/ruby/lib/ruby/gems/1.8/gems/activerecord-2.1.0/lib/active_record/connection_adapters/postgresql_adapter.rb:116:in `binary_to_string'

c:/ruby/lib/ruby/gems/1.8/gems/activerecord-2.1.0/lib/active_record/connection_adapters/postgresql_adapter.rb:126:in `binary_to_string'

c:/ruby/lib/ruby/gems/1.8/gems/activerecord-2.1.0/lib/active_record/connection_adapters/abstract/schema_definitions.rb:69:in `type_cast'

c:/ruby/lib/ruby/gems/1.8/gems/activerecord-2.1.0/lib/active_record/dirty.rb:150:in `field_changed?'

c:/ruby/lib/ruby/gems/1.8/gems/activerecord-2.1.0/lib/active_record/dirty.rb:128:in `write_attribute'

c:/ruby/lib/ruby/gems/1.8/gems/activerecord-2.1.0/lib/active_record/attribute_methods.rb:211:in `file_data='

app/models/picture.rb:12:in `from_file_data'

there's a little more info here: http://www.nopugs.com/binary-to-...

To create a test that fails, you can just try to write ("" << 92 << 53 << 53 << 53) to any model object's binary field. This will fail because it will interpret this binary data as \555 which is octal for 365, which is above 255 which causes binary_to_string to raise the above exception. A better but more involved test would be to use something that codes below 255 and have it fail when the saved value doesn't match the initial, if that indeed happens.

Comments and changes to this ticket

  • azimux

    azimux July 13th, 2008 @ 11:52 AM

    • no changes were found...
  • Kim Toms

    Kim Toms August 18th, 2008 @ 10:04 PM

    • Tag changed from 2.1, activerecord, bug to 2.1, activerecord, bug, tests

    This patch modifies the binary test to cause the postgres adapter to fail this test.

  • Kim Toms
  • Kim Toms

    Kim Toms August 19th, 2008 @ 12:57 AM

    • Tag changed from 2.1, activerecord, bug, tests to 2.1, activerecord, bug, patch, tests

    This patch fixes it, but may break compatibility with older version of the database.

  • azimux

    azimux August 19th, 2008 @ 09:04 AM

    Nice patch kim!

    I'm going to try applying it and giving it a whirl. One thing I notice right off the bat is your new code assumes unescape_bytea will be defined by the connection, but I know of at least one pg adapter that doesn't implement this method (postgres-pr)

    It might be worth keeping part of the old code in a new method, maybe like this:

    
    def non_native_unescape_bytea(value)
                      result = ''
                      i, max = 0, value.size
                      while i < max
                        char = value[i]
                        if char == ?\\
                          if value[i+1] == ?\\
                            char = ?\\
                            i += 1
                          else
                            char = value[i+1..i+3].oct
                            i += 3
                          end
                        end
                        result << char
                        i += 1
                      end
                      result
    end
    

    and then in your code you could do something like:

    
                    elsif res.ftype(cell_index) == BYTEA_COLUMN_TYPE_OID
                      row[cell_index] = if PGconn.respond_to?(:unescape_bytea)
                        PGconn.unescape_bytea(row[cell_index])
                      else
                        non_native_unescape_bytea(row[cell_index])
                      end
                    end
    

    I spent the better part of a weekend a while back battling with this bug. The solution I came up with was no where near as clean as your patch, so I'm pretty excited to try this out. Thanks again!

    I noticed you left string_to_binary alive, but took binary_to_string out. I'm guessing this code is used only when building queries? Is it safe to at least remove the regular expressions from binary_to_string?

    I remember I was also going to write a test for this as well, but when I ran the tests I got 6 failures from existing tests, which discouraged me from spending anymore time on it. Were you able to have every single test pass?

  • azimux

    azimux August 19th, 2008 @ 12:34 PM

    Everything I tested out with kim's patch seemed to work

    here's a patch that, if applied after kim's patches, allows it to work with postgres-pr

  • capellamusic

    capellamusic October 7th, 2008 @ 05:00 PM

    • Tag changed from 2.1, activerecord, bug, patch, tests to 2.1, activerecord, bug, patch, tests

    Hi, I've tried your patches but although the objects with bytea columns are correctly inserted into the DB, the listings (with will_paginate) aren't working anymore. Any idea about it?

  • Kim Toms

    Kim Toms November 3rd, 2008 @ 10:40 PM

    • Tag changed from 2.1, activerecord, bug, patch, tests to 2.1, activerecord, bug, patch, tests

    Do you have a specific failing test? That is the first thing to work out. I don't see any test with paginate in its text.

  • azimux

    azimux November 3rd, 2008 @ 10:49 PM

    Note to rails committers: It looks like postgres-pr is no longer supported by rails and only ruby-pg and ruby-postgres work anymore. So the patch I submitted to make Kim's patch work with postgres-pr is probably irrelevant at this point.

  • Kim Toms

    Kim Toms November 5th, 2008 @ 08:44 PM

    After reviewing will_paginate, I don't see any reason for it not to work, so I will require more details of the failure. If you'll be at rubyconf 5-8 Nov, I'll be there too.

  • Frederick Cheung

    Frederick Cheung December 21st, 2008 @ 05:24 PM

    • State changed from “new” to “resolved”

    Should have been resolved by #1063. Do reopen if it's still a problem

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages