appnexus / pyrobuf

A Cython alternative to Google's Python Protobuf library
Other
555 stars 72 forks source link

Segmentation Fault When Creating Cyclic Embedded Messages #69

Closed steventang2013 closed 6 years ago

steventang2013 commented 7 years ago

When adding an object to a repeated field, pyrobuf tosses a seg fault.

Environment:

Python 2.7.10
pyrobuf 0.5.11
libprotoc 2.6.1

Proto definition file:

message AResponse{
        repeated A a = 1;
}

message B {
        required A a = 1;
}

message A {
        required string text = 1;
        optional B b = 2;
}

Output error:

In [8]: a_response = AResponse()
In [9]: a = a_response.a.add()

Segmentation fault: 11

When I compile the exact same definition file with protoc instead of pyrobuf, it runs successfully.

tburmeister commented 7 years ago

Ran under gdb and quit out when the backtrace hit frame 12000 or something...

The underlying issue is that we create empty sub messages on message initialization, even for optional sub messages, as opposed to setting them as None. In this instance, we continue recursively initializing sub messages until we finally explode. I have to think a little about the cleanest solution.

tburmeister commented 7 years ago

Just put up and attached a proposed solution for this problem, which is to lazy init sub messages so we only instantiate them the first time we attempt to access them. I can now do the following:

In [1]: from issue_69_proto import *

In [2]: a = A()

In [3]: a.b
Out[3]: <issue_69_proto.B at 0x104430f50>

In [4]: a.b.a
Out[4]: <issue_69_proto.A at 0x104195bf0>

In [5]: a.b.a.b.a
Out[5]: <issue_69_proto.A at 0x1041958f0>

In [6]: b = B()

In [7]: b.a.b.a
Out[7]: <issue_69_proto.A at 0x1041956b0>
steventang2013 commented 7 years ago

Thanks for applying the fix!

Originally, I thought that protobuf lacked cycle support, but that wasn't the case when I compiled it with protoc. Any idea when this fix will be merged?

tburmeister commented 6 years ago

Late response here, but just merged in the fix.