python CopyFrom segmentation fault on version 4.24.0

[sfc-gh-mussakli]

What version of protobuf and what language are you using?
Version: v4.24.0
Language: Python upb

What operating system (Linux, Windows, ...) and version?
Repros on Linux and MacOS

What runtime / compiler are you using (e.g., python version or gcc version)
python 3.8
Installed Apple clang version 14.0.3 (clang-1403.0.22.14.1)

What did you do?
We have a proto message that fundamentally looks like this:

syntax = "proto3";

package message;

message TestMessage {
    optional int64 field_one = 1;
    optional int64 field_two = 2;
    map<int32, double> map_field = 3;
}

And we have a piece of python that tries to copy an empty version of one of these messages into the other:

message = message_pb2.TestMessage()
message2 = message_pb2.TestMessage()
message2.CopyFrom(message)

This is resulting on a segmentation fault on exactly the CopyFrom line.

This failure started with us picking up version 4.24.0 with yesterday's release. Reverting back to any version prior fixes the issue.

Unfortunately building the above example proto as an MVP did not repro the segmentation fault for me. If there are any diagnostics I can provide that would make root causing this easier, happy to help.

What did you expect to see
No seg fault, behavior as before.

What did you see instead?
Segmentation fault failure on CopyFrom

Make sure you include information that can help us debug (full error message, exception listing, stack trace, logs).

Anything else we should know about your project / environment

[arcra]

I've seen this issue with python 3.9 and 3.11 as well. It's blocking our project's CI runs.

[anandolee]

Not sure why but I can NOT reproduce in google3 internal CL with

message TestMessage {
    optional int64 field_one = 1;
    optional int64 field_two = 2;
    map<int32, double> map_field = 3;
}

message = message_pb2.TestMessage()
message2 = message_pb2.TestMessage()
message2.CopyFrom(message)

Nor opensource on my linux:

$ pip show protobuf
Name: protobuf
Version: 4.24.0
$ python
Python 3.11.4 (main, Jun  7 2023, 10:13:09) [GCC 12.2.0] on linux
>>> from google.protobuf.internal import api_implementation
>>> print(api_implementation.Type())
upb
>>> from google.protobuf import timestamp_pb2
>>> msg = timestamp_pb2.Timestamp()
>>> msg2 = timestamp_pb2.Timestamp()
>>> msg2.CopyFrom(msg)
>>> from google.protobuf import struct_pb2
>>> msg = struct_pb2.Struct()
>>> msg2 =  struct_pb2.Struct()
>>> msg2.CopyFrom(msg)

[arcra]

Just a thought, could it be that the protos that we're intending to use are "compiled" with a previous protoc version, and the issue is that this new version throws in that case? In our project, it was failing when trying to use some protos from a TensorFlow dependency (which was released before this new protobuf release from yesterday).

[anandolee]

Can you help to verify:
1, Does the segment fault still raise if not depend on TensorFlow.
2, Does the segment fault still raise for the message that do not have map:

message TestMessage {
    optional int64 field_one = 1;
    optional int64 field_two = 2;
}

message = message_pb2.TestMessage()
message2 = message_pb2.TestMessage()
message2.CopyFrom(message)

3, Does the segment fault still raise for struct_pb2.Struct()

from google.protobuf import struct_pb2
msg = struct_pb2.Struct()
msg2 =  struct_pb2.Struct()
msg2.CopyFrom(msg)

[sfc-gh-mussakli]

Just a thought, could it be that the protos that we're intending to use are "compiled" with a previous protoc version, and the issue is that this new version throws in that case? In our project, it was failing when trying to use some protos from a TensorFlow dependency (which was released before this new protobuf release from yesterday).

This might actually be possibly how it's happening, as I could not produce a simple repro outside of our internal product.

Can you help to verify: 1, Does the segment fault still raise if not depend on TensorFlow. 2, Does the segment fault still raise for the message that do not have map:

message TestMessage {
    optional int64 field_one = 1;
    optional int64 field_two = 2;
}

message = message_pb2.TestMessage()
message2 = message_pb2.TestMessage()
message2.CopyFrom(message)

3, Does the segment fault still raise for struct_pb2.Struct()

from google.protobuf import struct_pb2
msg = struct_pb2.Struct()
msg2 =  struct_pb2.Struct()
msg2.CopyFrom(msg)

I am working to answer your other questions, but I can confirm (1) that we do not depend on TensorFlow.

[sfc-gh-mussakli]

Can you help to verify: 1, Does the segment fault still raise if not depend on TensorFlow. 2, Does the segment fault still raise for the message that do not have map:

message TestMessage {
    optional int64 field_one = 1;
    optional int64 field_two = 2;
}

message = message_pb2.TestMessage()
message2 = message_pb2.TestMessage()
message2.CopyFrom(message)

3, Does the segment fault still raise for struct_pb2.Struct()

from google.protobuf import struct_pb2
msg = struct_pb2.Struct()
msg2 =  struct_pb2.Struct()
msg2.CopyFrom(msg)

Confirming for (3) that this does not fail with struct_pb2.Struct() and removing for (2), the segmentation fault still occurs with no map.

[anandolee]

Thank you for these info. As both of you mentioned that the code is depending on old code gen, can you try if upgrade these gen files with newest protoc to see if it can resolve the issue?

[sfc-gh-mussakli]

Thank you for these info. As both of you mentioned that the code is depending on old code gen, can you try if upgrade these gen files with newest protoc to see if it can resolve the issue?

This gives us some more insights, that might inform debugging this issue.

We use rules_proto Bazel library to build our protobufs. The latest version uses protobuf 21.10: https://rules-proto-grpc.com/en/latest/changelog.html#id5.

Upgrading to the latest version of rules_proto and building the proto with that still causes the segmentation fault when CopyFrom is called. My suspicion is that this is the root cause. Perhaps you can repro by compiling the proto with an older version like 21.10, and then running python with protobuf library version 4.24.0.

[arcra]

Thank you for these info. As both of you mentioned that the code is depending on old code gen, can you try if upgrade these gen files with newest protoc to see if it can resolve the issue?

Are you asking this just to validate that the issue is not present if we do that? Or as a proposed solution for users encountering this issue?

Note that a lot of times (like in our case with the TensorFlow dependency) we're depending on a package that is not owned by us, generally, I don't thin it's feasible to require that from every user that gets broken by this change. Rather, this would restrict projects from using newer versions of protobuf libraries unless all dependencies also migrate to the latest version.

For validation, given that the protos are from dependencies we don't own, I expect you would have more context on what a "packaged" version of a proto would look like, or how to produce and use it, so it should be easier for you to find a way to produce some minimal dependency with the older version, and use it with a newer version, using virtualenv or something like this?

[anandolee]

Understand that you may not able to upgrade the protoc version. The "produce some minimal dependency with the older version" under design currently.

Mert mentioned "This failure started with us picking up version 4.24.0 with yesterday's release. Reverting back to any version prior fixes the issue." Can you help to confirm that if 4.23.4 do not produce the segment fault and also make sure 4.23.4 is using upb python.

from google.protobuf.internal import api_implementation
print(api_implementation.Type())

If 4.23.4 works, it will make us easier to identify which change may trigger the issue

[arcra]

For us, version 4.23.4 works fine.

Our work around was to restrict the protobuf dependency to be < 4.24: tensorflow/tensorboard@734988d, and I can see in the logs of our CI runs that indeed, protobuf==4.23.4 was used.

[anandolee]

If it is using protoc 4.21.10, I think it is not related to old gen code. protoc 4.21.10 and 4.24.0 do not have differences on upb python generated code.

Just compared with runtime 4.23.4 and 4.24.0:
In 4.23.4 the CopyFrom() is a wrapper that calls SerializeToString() And ParseFromString(). For performance, 4.24.0 changed CopyFrom() to copy from memories. I think this is a cause.

However we still do not have enough info to repo which makes us hard to go forward....

As Mert said "I could not produce a simple repro outside of our internal product." Wondering if Adrian can provide a repo?

[sfc-gh-mussakli]

For us, version 4.23.4 works fine.

Our work around was to restrict the protobuf dependency to be < 4.24: tensorflow/tensorboard@734988d, and I can see in the logs of our CI runs that indeed, protobuf==4.23.4 was used.

+1'ing this that 4.23.4 worked fine on our end. This code is stressed daily, and the immediate hours after 4.24.0 release is when failures started happening.

@anandolee, sounds like you have a good clue about what might be causing the crash.

[freewilll]

I am having this problem too.

[sfc-gh-mussakli]

@anandolee, please let us know how we can help with getting a fix for this issue. We are happy to test private releases on our end to verify a hypothetical fix.

[anandolee]

We can rollback the change to serialize/parse wrap in 24.1 release to temporary unblock projects. But main will keep the same to use memory copy and 25.0 release will back to memory copy. This means the bug will return in 25.0 if we don't find an adequate repro in time.

Wondering if you can continue help us with repro for this issue?

Also, can you help to verify if protocolbuffers/upb#1486 work?

[sfc-gh-mussakli]

@anandolee, I can try to verify if that fix works. Can you share instructions on how to install a private release containing that fix?

[sfc-gh-mussakli]

The change has been merged and it will be release in 24.1 soon.

Main will keep to use memory copy and 25.0 release will back to memory copy

OK, we will test if 24.1 fixes this bug and that can be the verification that this is the root cause of the crash.

[sfc-gh-mussakli]

@anandolee We have recently tested the crashing test on version 4.24.1 and I can confirm that the crash does not repro with this CopyFrom optimization disabled.

You said that 4.25.0 will re-introduce this memory copy. Does that mean that the bug is slated to be re-introduced as-is, or are you working on an implementation that won't seg fault?

[fowles]

We are hoping to figure out the root cause and fix it in the intervening time. If you can try to repro against protobuf mainline that would be helpful

[sfc-gh-mussakli]

We are hoping to figure out the root cause and fix it in the intervening time. If you can try to repro against protobuf mainline that would be helpful

Happy to help, I'm assuming you're suggesting building the upb backend locally and testing against that. Are there dev instructions anywhere to guide with that?

[sfc-gh-mussakli]

Good news, I have a repro 🎉 .

Turns out, the issue isn't to do with copying one empty initialized message into another. It's calling CopyFrom on an unset optional field.

Here is what I have:

syntax = "proto3";

package message;

message BigMessage {
    optional SurroundingMessage surrounding_message = 1;
}

message SurroundingMessage {
    optional TestMessage test_message = 1;
}

message TestMessage {
    optional int64 field_one = 1;
    optional int64 field_two = 2;
    map<int32, double> map_field = 3;
}

I have built this via protoc -I=. --python_out=. ./message.proto

Then, I execute the following test:

import message_pb2

big_message = message_pb2.BigMessage(
    surrounding_message=message_pb2.SurroundingMessage(),
)

test_message = message_pb2.TestMessage()
test_message.CopyFrom(big_message.surrounding_message.test_message)

You should get a segmentation fault on 4.24.0, and successful execution on any other version.

[anandolee]

Thank you Mert! I can reproduce the error with your new example! Will work on a real fix!

[sfc-gh-psteijn]

Thank you Mert! I can reproduce the error with your new example! Will work on a real fix!

Thanks for the update Jie and Matt!

[anandolee]

The issue should been fixed by #1514
25.0 will back to memory copy and no crash for copy the unsigned empty sub message

[sfc-gh-mussakli]

@anandolee Thank you, this is great to hear!