You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by Sinan Taifour <st...@gmail.com> on 2009/08/26 23:13:48 UTC
Segmentation Fault when using BinaryProtocolAccelerated in Ruby (with
patch)
Greetings!
I have noticed that when using the BinaryProtocolAccelerated in Ruby, and
expecting a map<string,string> and getting something else, a segmentation
fault occurs. I was able to trace the segmentation fault back to
binary_protocol_accelerated.c, in r807972, the problem is in
RSTRING_LEN(str) inside write_string_direct(...).
Notice that, in the Python implementation, or when using BinaryProtocol as
opposed to BinaryProtocolAccelerated, an exception is raised when a value
has a type other than string in a map<string, string>.
Below is a patch that solved the problem for me, please advise on whether or
not this would be the right way to solve it (it appears to work properly in
different cases).
Index: lib/rb/ext/binary_protocol_accelerated.c
===================================================================
--- lib/rb/ext/binary_protocol_accelerated.c (revision 807972)
+++ lib/rb/ext/binary_protocol_accelerated.c (working copy)
@@ -76,6 +76,9 @@
}
static void write_string_direct(VALUE trans, VALUE str) {
+ if (TYPE(str) != T_STRING) {
+ rb_raise(rb_eStandardError, "Value should be a string");
+ }
write_i32_direct(trans, RSTRING_LEN(str));
rb_funcall(trans, write_method_id, 1, str);
}
Also, below you can find a set of files that can be used to reproduce the
segmentation fault mentioned:
test.thrift
===================================================================
service Test {
i32 c1(1: i32 p1, 2: i32 p2, 3: map<string,string> p3)
}
server.rb
===================================================================
require 'rubygems'
require 'thrift'
require 'extlib'
CURDIR = File.dirname(__FILE__)
$: << CURDIR / "gen-rb"
Dir[CURDIR / "gen-rb" / "**" / "*"].each { |f| require f unless
File.directory?(f) }
class TestHandler
def c1(p1, p2, p3)
p1 + p2
end
end
handler = TestHandler.new()
processor = Test::Processor.new(handler)
transport = Thrift::ServerSocket.new(9090)
transportFactory = Thrift::BufferedTransportFactory.new()
protocolFactory = Thrift::BinaryProtocolAcceleratedFactory.new
server = Thrift::ThreadPoolServer.new(processor, transport,
transportFactory, protocolFactory)
server.serve()
client.rb
===================================================================
require 'rubygems'
require 'thrift'
require 'extlib'
CURDIR = File.dirname(__FILE__)
$: << CURDIR / "gen-rb"
Dir[CURDIR / "gen-rb" / "**" / "*"].each { |f| require f unless
File.directory?(f) }
transport = Thrift::BufferedTransport.new(Thrift::Socket.new('localhost',
9090))
protocol = Thrift::BinaryProtocolAccelerated.new(transport)
srv = Test::Client.new(protocol)
transport.open()
p srv.c1(1,1,{'asfd' => true}) # Causes segmentaion fault without the patch,
raises an error with the patch
Thank you!
-- Sinan Taifour
Re: Segmentation Fault when using BinaryProtocolAccelerated in Ruby
(with patch)
Posted by Sinan Taifour <st...@gmail.com>.
I have posted it, it is THRIFT-569. This is my first ever patch to an open
source project, so I'm not sure I did it right. If there is anything else I
need to do, please let me know.
-- Sinan
On Thu, Aug 27, 2009 at 2:14 AM, Bryan Duxbury <br...@rapleaf.com> wrote:
> Sinan - This is a great find. Can you open a JIRA ticket (http://
> issues.apache.org/jira/browse/THRIFT) and attach the patch so all the
> legal stuff is squared away? Thanks!
>
> -Bryan
>
>
> On Aug 26, 2009, at 2:13 PM, Sinan Taifour wrote:
>
> Greetings!
>>
>> I have noticed that when using the BinaryProtocolAccelerated in Ruby, and
>> expecting a map<string,string> and getting something else, a segmentation
>> fault occurs. I was able to trace the segmentation fault back to
>> binary_protocol_accelerated.c, in r807972, the problem is in
>> RSTRING_LEN(str) inside write_string_direct(...).
>>
>> Notice that, in the Python implementation, or when using BinaryProtocol as
>> opposed to BinaryProtocolAccelerated, an exception is raised when a value
>> has a type other than string in a map<string, string>.
>>
>> Below is a patch that solved the problem for me, please advise on whether
>> or
>> not this would be the right way to solve it (it appears to work properly
>> in
>> different cases).
>>
>> Index: lib/rb/ext/binary_protocol_accelerated.c
>> ===================================================================
>> --- lib/rb/ext/binary_protocol_accelerated.c (revision 807972)
>> +++ lib/rb/ext/binary_protocol_accelerated.c (working copy)
>> @@ -76,6 +76,9 @@
>> }
>>
>> static void write_string_direct(VALUE trans, VALUE str) {
>> + if (TYPE(str) != T_STRING) {
>> + rb_raise(rb_eStandardError, "Value should be a string");
>> + }
>> write_i32_direct(trans, RSTRING_LEN(str));
>> rb_funcall(trans, write_method_id, 1, str);
>> }
>>
>> Also, below you can find a set of files that can be used to reproduce the
>> segmentation fault mentioned:
>>
>> test.thrift
>> ===================================================================
>> service Test {
>> i32 c1(1: i32 p1, 2: i32 p2, 3: map<string,string> p3)
>> }
>>
>> server.rb
>> ===================================================================
>> require 'rubygems'
>> require 'thrift'
>> require 'extlib'
>> CURDIR = File.dirname(__FILE__)
>> $: << CURDIR / "gen-rb"
>> Dir[CURDIR / "gen-rb" / "**" / "*"].each { |f| require f unless
>> File.directory?(f) }
>>
>> class TestHandler
>> def c1(p1, p2, p3)
>> p1 + p2
>> end
>> end
>>
>> handler = TestHandler.new()
>> processor = Test::Processor.new(handler)
>> transport = Thrift::ServerSocket.new(9090)
>> transportFactory = Thrift::BufferedTransportFactory.new()
>> protocolFactory = Thrift::BinaryProtocolAcceleratedFactory.new
>> server = Thrift::ThreadPoolServer.new(processor, transport,
>> transportFactory, protocolFactory)
>>
>> server.serve()
>>
>> client.rb
>> ===================================================================
>> require 'rubygems'
>> require 'thrift'
>> require 'extlib'
>> CURDIR = File.dirname(__FILE__)
>> $: << CURDIR / "gen-rb"
>> Dir[CURDIR / "gen-rb" / "**" / "*"].each { |f| require f unless
>> File.directory?(f) }
>>
>> transport = Thrift::BufferedTransport.new(Thrift::Socket.new('localhost',
>> 9090))
>> protocol = Thrift::BinaryProtocolAccelerated.new(transport)
>> srv = Test::Client.new(protocol)
>> transport.open()
>>
>> p srv.c1(1,1,{'asfd' => true}) # Causes segmentaion fault without the
>> patch,
>> raises an error with the patch
>>
>>
>> Thank you!
>>
>> -- Sinan Taifour
>>
>
>
Re: Segmentation Fault when using BinaryProtocolAccelerated in Ruby (with patch)
Posted by Bryan Duxbury <br...@rapleaf.com>.
Sinan - This is a great find. Can you open a JIRA ticket (http://
issues.apache.org/jira/browse/THRIFT) and attach the patch so all the
legal stuff is squared away? Thanks!
-Bryan
On Aug 26, 2009, at 2:13 PM, Sinan Taifour wrote:
> Greetings!
>
> I have noticed that when using the BinaryProtocolAccelerated in
> Ruby, and
> expecting a map<string,string> and getting something else, a
> segmentation
> fault occurs. I was able to trace the segmentation fault back to
> binary_protocol_accelerated.c, in r807972, the problem is in
> RSTRING_LEN(str) inside write_string_direct(...).
>
> Notice that, in the Python implementation, or when using
> BinaryProtocol as
> opposed to BinaryProtocolAccelerated, an exception is raised when a
> value
> has a type other than string in a map<string, string>.
>
> Below is a patch that solved the problem for me, please advise on
> whether or
> not this would be the right way to solve it (it appears to work
> properly in
> different cases).
>
> Index: lib/rb/ext/binary_protocol_accelerated.c
> ===================================================================
> --- lib/rb/ext/binary_protocol_accelerated.c (revision 807972)
> +++ lib/rb/ext/binary_protocol_accelerated.c (working copy)
> @@ -76,6 +76,9 @@
> }
>
> static void write_string_direct(VALUE trans, VALUE str) {
> + if (TYPE(str) != T_STRING) {
> + rb_raise(rb_eStandardError, "Value should be a string");
> + }
> write_i32_direct(trans, RSTRING_LEN(str));
> rb_funcall(trans, write_method_id, 1, str);
> }
>
> Also, below you can find a set of files that can be used to
> reproduce the
> segmentation fault mentioned:
>
> test.thrift
> ===================================================================
> service Test {
> i32 c1(1: i32 p1, 2: i32 p2, 3: map<string,string> p3)
> }
>
> server.rb
> ===================================================================
> require 'rubygems'
> require 'thrift'
> require 'extlib'
> CURDIR = File.dirname(__FILE__)
> $: << CURDIR / "gen-rb"
> Dir[CURDIR / "gen-rb" / "**" / "*"].each { |f| require f unless
> File.directory?(f) }
>
> class TestHandler
> def c1(p1, p2, p3)
> p1 + p2
> end
> end
>
> handler = TestHandler.new()
> processor = Test::Processor.new(handler)
> transport = Thrift::ServerSocket.new(9090)
> transportFactory = Thrift::BufferedTransportFactory.new()
> protocolFactory = Thrift::BinaryProtocolAcceleratedFactory.new
> server = Thrift::ThreadPoolServer.new(processor, transport,
> transportFactory, protocolFactory)
>
> server.serve()
>
> client.rb
> ===================================================================
> require 'rubygems'
> require 'thrift'
> require 'extlib'
> CURDIR = File.dirname(__FILE__)
> $: << CURDIR / "gen-rb"
> Dir[CURDIR / "gen-rb" / "**" / "*"].each { |f| require f unless
> File.directory?(f) }
>
> transport = Thrift::BufferedTransport.new(Thrift::Socket.new
> ('localhost',
> 9090))
> protocol = Thrift::BinaryProtocolAccelerated.new(transport)
> srv = Test::Client.new(protocol)
> transport.open()
>
> p srv.c1(1,1,{'asfd' => true}) # Causes segmentaion fault without
> the patch,
> raises an error with the patch
>
>
> Thank you!
>
> -- Sinan Taifour