Update postgres hstore casting to handle null-terminated strings. #13827

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants

scosman commented Jan 24, 2014

This is needed because ruby doesn't null terminate strings, but the PG connection adapter does. Without this, trying to use a string containing null in an hstore will always result in invalid SQL ("Unexpected end of string").

I chose to truncate the string at the first \0 to be consistent with PG.connection.quote_string and the C PQescapeStringConn. This is the same behaviour AR silently applies to text/varchar so I think it's a good fit.

To test:
ARCONN=postgresql ruby -Itest test/cases/adapters/postgresql/hstore_test.rb

@scosman scosman Update postgres hstore casting to handle null-terminated strings.
This is needed because ruby doesn't null terminate strings, but the PG
connection adapter does. Without this, trying to use a string containing
null in an hstore will always result in invalid SQL (Unexpected end of
 string").

I chose to truncate the string at the first \0 to be consistent with
quote_string and the native PQescapeStringConn.
04eb36c
Member

chancancode commented Jan 26, 2014

Is this specific to hstore or is it a problem for postgres strings in general?

scosman commented Jan 27, 2014

Just hstore. Other fields (text/varchar) use the quote/quote_string methods which use PQescapeStringConn, a native method that truncates on the first nil char.

@rafaelfranca rafaelfranca modified the milestone: 4.0.5, 4.0.4, 4.0.6 Mar 10, 2014

Contributor

JonRowe commented Apr 14, 2014

Is there something blocking this @chancancode?

Member

chancancode commented Apr 15, 2014

@senny / @rafaelfranca do you happen to know if the behaviour on PG strings is expected?

unless File.exist?('Gemfile')
  File.write('Gemfile', <<-GEMFILE)
    source 'https://rubygems.org'
    gem 'rails', github: 'rails/rails'
    gem 'arel', github: 'rails/arel'
    gem 'sqlite3'
  GEMFILE

  system 'bundle'
end

require 'bundler'
Bundler.setup(:default)

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: 'arrays_test')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :title
    t.text   :body
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_association_stuff
    title = "My blog\u0000 post"
    body  = "This is a blog post.\n\nHello\u0000, welcome to my blog!"

    post = Post.create!(title: title, body: body)

    post.reload

    assert_equal 1, Post.count
    assert_equal title, post.title
    assert_equal body, post.body
  end
end
  1) Failure:
BugTest#test_association_stuff [bug.rb:43]:
Expected: "My blog\u0000 post"
  Actual: "My blog"

Also, if this behaviour is coming from quote_string, shouldn't we just call that on the keys/values on PG hstores? (and arrays too?)

Owner

matthewd commented Apr 15, 2014

@chancancode I think it's more un-accounted-for than expected.

I think we should be raising if there's a null character in the string we're quoting. In fact, I'd venture that it's a bug in pg: it's incorrectly using StringValuePtr where it wants StringValueCStr.

I deeply object to any behaviour that results in silently truncating data while we claim to have faithfully transferred it to the database.

matthewd added the PostgreSQL label Apr 15, 2014

scosman commented Apr 15, 2014

per this patch, I only implemented the truncate to be consistent. I agree that silent truncation isn't ideal.

We can't just use quote_string here. hstore values are quoted differently than typical strings. Only double quotes are escaped.

http://www.postgresql.org/docs/9.0/static/hstore.html

@chancancode chancancode added JRuby and removed JRuby labels Jun 26, 2014

@rafaelfranca rafaelfranca modified the milestone: 4.0.10, 4.1.7 Aug 21, 2014

@rafaelfranca rafaelfranca modified the milestone: 4.1.9, 4.0.13 Nov 19, 2014

@rafaelfranca rafaelfranca modified the milestone: 4.0.13, 4.1.9 Jan 2, 2015

aviv commented Apr 9, 2015

This patch solves an issue that we've been having:

  1. A null byte is included (possibly by malware) in a string a user submits via a web form.
  2. An exception is raised when we try to save that string in an hstore column.

So, thanks for the fix! We're curious if this needs anything in order to get merged.

Owner

matthewd commented Apr 9, 2015

We don't want this. pg has fixed the behaviour I described previously, so it's consistent for us to take no action, and leave it to raise.

The raise is correct, because we're talking to a database -- if we're asked to store something, and we can't do so, we're obliged to state that fact.. not mangle the value and then store something entirely different.

matthewd closed this Apr 9, 2015

scosman commented Apr 10, 2015

@matthewd That's not an accurate description of the bug or patch.

Postgres hstore can store stings with null characters. It's not a matter of "if we're asked to store something, and we can't do so, we're obliged to state that fact".

This patch just implemented truncate to be consistent. Now that the other bug has fixed in PG, it can be edited to store the whole string correctly.

[edited: corrected last paragraph]

Owner

matthewd commented Apr 10, 2015

Postgres hstore can store [strings] with null characters.

It can? I can't find anything in the documentation to support that, nor have I managed to experimentally find a way of spelling a null byte that will actually get it stored. (Which doesn't surprise me -- hstore is a text based type, and text can't contain null bytes.)

If you can help me find such a spelling, I'd be glad to see us support it. Let's just play with a raw query for now:

INSERT INTO h VALUES ('foo=>"bar"');
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment