Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaced StringValuePtr to StringValueCStr #5

Closed
wants to merge 1 commit into from

Conversation

ymmtmdk
Copy link

@ymmtmdk ymmtmdk commented Sep 28, 2014

On ruby-head environment, I got the following error:

PG::CharacterNotInRepertoire: ERROR: invalid byte sequence for encoding "UTF8": 0xe0 0x0c 0xf7

I researched the code and found it may occured by not terminated C string.
In Ruby’s README.ext,

You can also use the macro named StringValueCStr(). This is just
like StringValuePtr(), but always add nul character at the end of
the result. If the result contains nul character, this macro causes
the ArgumentError exception.
StringValuePtr() doesn't guarantee the existence of a nul at the end
of the result, and the result may contain nul.

So, I made this patch and solved the error.
Is this right?

@larskanis
Copy link
Collaborator

Thank you for the bug report!

You're right in respect to StringValuePtr(), but we can not blindly replace all occurrences by StringValueCStr(), because we do handle binary data on several places as well.

Could you please post some code that triggered the error message for you? We need a test cast before we do any changes.

@ymmtmdk
Copy link
Author

ymmtmdk commented Oct 3, 2014

Thank you for you comment.
Agreeing, this patch is too blindly.
I can provide the code, but it is using Rails..
I challenged to reproduce the bug without Rails, but I unfortunately couldn't.

If you could read Rails, this is very simple scaffold App.
https://github.com/ymmtmdk/emailapp
Thank you.

@larskanis
Copy link
Collaborator

OK, I'll give it a try. Which OS are you using? And how did you install Ruby?

@ymmtmdk
Copy link
Author

ymmtmdk commented Oct 3, 2014

Thank you. I reproduced this in OS X and Amazon Linux. Both Ruby were Installed by "rvm install ruby-head". I updated the README.doc slightly. Please read it.

@larskanis
Copy link
Collaborator

Your rails project is working great, but unfortunately I'm not able to reproduce the error on Ubuntu-14.04 running on a fresh ruby version installed by rvm install ruby-head. Valgrind throws a lot of failures because of uninitialized variables, but none of them are related to this issue. I also couldn't find a single case in MRI's string.c, that creates a String object without 0-termination. So I currently don't have a good idea, how we could implement correct String usage without being able to test it.

@ymmtmdk
Copy link
Author

ymmtmdk commented Oct 6, 2014

Thank you for your try. I pushed a project to Github. I tried this on ubuntu14.04(and PostgreSQL9.3 and today's Ruby) in AWS, and this showed the error. Could you try this please?

https://github.com/ymmtmdk/sample_app_rails

@ymmtmdk
Copy link
Author

ymmtmdk commented Oct 6, 2014

might need to run "rake" twice.

larskanis added a commit to larskanis/ruby-pg that referenced this pull request Oct 6, 2014
…nters

are expected.

This fixes issues with ruby-2.2.0-dev described in github pull request ged#5:
ged#5
@larskanis
Copy link
Collaborator

@ymmtmdk Now I get the same error raised on Ubuntu-14.04. I couldn't find out easily, which Ruby construct builds non zero terminated String objects, nor whether this behavior is really desired. But the README.ext is quite clear about zero termination, so I scanned my type cast branch for interesting cases and fixed it accordingly. I hope we can merge it soon, or we need to backport the commit elsewise.

@ymmtmdk
Copy link
Author

ymmtmdk commented Oct 7, 2014

I asked how to create not null terminated string at ruby-dev ML. Comitter naruse-san said, it can not be in Ruby but can do it in an extension. And he showed to me the code http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-dev/48598

So, I have created a Gem https://github.com/ymmtmdk/not_null_terminated
Usage example:

require 'pg'
require 'not_null_terminated'

def nnt(sql)
  inst = "; INSERT INTO users (email) VALUES ('ymmt')"
  str = NotNullTerminatedString.new(sql+inst)
  inst.size.times{
    str.chop_without_terminate
  }
  str
end

slct = "SELECT * FROM users"
nnt_slct = nnt(slct)
p nnt_slct == slct #true
p nnt_slct =~ /INSERT/ #nil

conn = PG.connect( dbname: 'emailapp_test' )
conn.exec(nnt_slct)
conn.exec(slct){|result|
  result.each{|row| p row}
}

@larskanis
Copy link
Collaborator

Cool! I'll use your gem to write proper test cases for ruby-pg.

@ymmtmdk
Copy link
Author

ymmtmdk commented Oct 8, 2014

Thank you. Naruse-san and bundler are really cool. : )

@meoooh
Copy link

meoooh commented Apr 29, 2015

Is there any update?

@larskanis
Copy link
Collaborator

This is fixed in commit https://bitbucket.org/ged/ruby-pg/commits/f0b7f99 and released as part of pg-0.18.0.

@larskanis larskanis closed this Apr 29, 2015
@meoooh
Copy link

meoooh commented Apr 29, 2015

thank you! @larskanis

@mtarnovan
Copy link

I'm still seeing this error in a Rails project using 0.18.1. Any ideas?

@JacobEvelyn
Copy link

Same here.

@larskanis
Copy link
Collaborator

@mtarnovan , @JacobEvelyn could you please put a stacktrace of the failure into a gist?

@JacobEvelyn
Copy link

Looks like I was only getting this error when trying to save binary data to a VARCHAR field rather than a BYTEA. Changing the field type worked for me.

larskanis added a commit to larskanis/nokogiri that referenced this pull request Nov 23, 2015
Ruby-2.2 changes the String handling in such a way, that strings can
no longer be expected to be zero terminated. When zero terminated
strings are expected in the C code, StringValueCStr() must be used.
See: https://github.com/ruby/ruby/blob/v2_2_0/NEWS#L319

Using StringValuePtr() without a length check, can also become a
security issue, because it can leak the memory after the string.

This was already an issue in the pg.gem: ged/ruby-pg#5
larskanis added a commit to larskanis/nokogiri that referenced this pull request Jan 6, 2016
Ruby-2.2 changes the String handling in such a way, that strings can
no longer be expected to be zero terminated. When zero terminated
strings are expected in the C code, StringValueCStr() must be used.
See: https://github.com/ruby/ruby/blob/v2_2_0/NEWS#L319

Using StringValuePtr() without a length check, can also become a
security issue, because it can leak the memory after the string.

This was already an issue in the pg.gem: ged/ruby-pg#5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants