You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2020/04/07 00:34:13 UTC
[GitHub] [mesos] cf-natali opened a new pull request #356: libprocess: check
protobuf (de)serialisation success.
cf-natali opened a new pull request #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356
Before the code didn't check at all the return value of
`Message::SerializeToString`, which can fail for various reasons,
e.g. out-of-memory, message too large, or invalid UTF-8 string.
Also, the way deserialisation was checked for error using
`Message::IsInitialized` doesn't detect errors such as the above,
we need to check `Message::ParseFromString` return value.
See example attached:
```
$ # message too large
$ make check
protoc test.proto --cpp_out=.
g++ -o test test.cpp test.pb.cc -lprotobuf -lpthread
./test
[libprotobuf ERROR google/protobuf/message_lite.cc:289] Exceeded maximum protobuf size of 2GB: 4294967298
SerializeToString: false
deserialising garbage
ParseFromString: false
IsInitialized: true
```
```
$ # invalid utf-8 string
$ make check
protoc test.proto --cpp_out=.
g++ -o test test.cpp test.pb.cc -lprotobuf -lpthread
./test
[libprotobuf ERROR google/protobuf/wire_format_lite.cc:626] String field 'test.Test.payload' contains invalid UTF-8 data when serializing a protocol buffer. Use the 'bytes' type if you intend to send raw bytes.
SerializeToString: true
[libprotobuf ERROR google/protobuf/wire_format_lite.cc:626] String field 'test.Test.payload' contains invalid UTF-8 data when parsing a protocol buffer. Use the 'bytes' type if you intend to send raw bytes.
ParseFromString: false
IsInitialized: true
deserialising garbage
ParseFromString: false
IsInitialized: true
```
[test.zip](https://github.com/apache/mesos/files/4441407/test.zip)
We noticed this at work because our custom executor had a bug causing it to send invalid/non-UTF8 mesos.TaskID, but it was successfully serialised by the executor (driver), and deserialised by the framework, which was blowing it to blow up at later point far from the original source of the problem.
More generally we want to catch such invalid messages - which can happen for a variety of reasons - as early as possible.
@bmahler
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] cf-natali commented on a change in pull request #356:
libprocess: check protobuf (de)serialisation success.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#discussion_r405139102
##########
File path: 3rdparty/libprocess/include/process/protobuf.hpp
##########
@@ -329,13 +332,11 @@ class ProtobufProcess : public process::Process<T>
{
google::protobuf::Arena arena;
M* m = CHECK_NOTNULL(google::protobuf::Arena::CreateMessage<M>(&arena));
- m->ParseFromString(data);
- if (m->IsInitialized()) {
+ if (m->ParseFromString(data)) {
(t->*method)(*m);
} else {
- LOG(WARNING) << "Initialization errors: "
- << m->InitializationErrorString();
+ LOG(WARNING) << "Error deserialising: " << data;
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] bmahler closed pull request #356: libprocess: check
protobuf (de)serialisation success.
Posted by GitBox <gi...@apache.org>.
bmahler closed pull request #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] bmahler commented on a change in pull request #356:
libprocess: check protobuf (de)serialisation success.
Posted by GitBox <gi...@apache.org>.
bmahler commented on a change in pull request #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#discussion_r405082745
##########
File path: 3rdparty/libprocess/include/process/protobuf.hpp
##########
@@ -329,13 +332,11 @@ class ProtobufProcess : public process::Process<T>
{
google::protobuf::Arena arena;
M* m = CHECK_NOTNULL(google::protobuf::Arena::CreateMessage<M>(&arena));
- m->ParseFromString(data);
- if (m->IsInitialized()) {
+ if (m->ParseFromString(data)) {
(t->*method)(*m);
} else {
- LOG(WARNING) << "Initialization errors: "
- << m->InitializationErrorString();
+ LOG(WARNING) << "Error deserialising: " << data;
Review comment:
I don't think we can just dump binary data in the log message here, and AFAICT it looks like there isn't any information we can get from protobuf about why the call failed.
How about just logging an ERROR level message like:
> Failed to deserialize 'UpdateSlaveMessage' from slave@192.168.0.1:5050
(you can just `<< to` to log the destination, and `m.GetTypeName()` for the name).
Maybe later we could consider also logging the hex encoded message itself, but it might be huge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] cf-natali commented on a change in pull request #356:
libprocess: check protobuf (de)serialisation success.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#discussion_r405139203
##########
File path: 3rdparty/libprocess/include/process/protobuf.hpp
##########
@@ -39,8 +39,11 @@ inline void post(const process::UPID& to,
const google::protobuf::Message& message)
{
std::string data;
- message.SerializeToString(&data);
- post(to, message.GetTypeName(), data.data(), data.size());
+ if (message.SerializeToString(&data)) {
+ post(to, message.GetTypeName(), data.data(), data.size());
+ } else {
+ LOG(WARNING) << "Error serialising: " << message.DebugString();
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] cf-natali commented on issue #356: libprocess: check
protobuf (de)serialisation success.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on issue #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#issuecomment-610644546
On a slightly-tangential subject: how do you deal with formatting?
I can see that there's a `support/clang-format` file, but applying it with e.g. clang-format version 7 results in many changes - I guess the code base hasn't been re-formatted to avoid churn?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] bmahler commented on issue #356: libprocess: check protobuf
(de)serialisation success.
Posted by GitBox <gi...@apache.org>.
bmahler commented on issue #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#issuecomment-610138094
Thanks for reporting this Charles! I will take a closer look soon to confirm the error handling in this patch is correct and complete.
A couple of logistical things in the meanwhile:
1. Would you be able to file a JIRA ticket? If you'd rather not, that's ok, I can file one for you, as we'll need one for this bug to be referenced and for this to make it into the CHANGELOG.
2. Can you extract the contributors.yaml change from this PR? We enforce that commits in the project do not mix unrelated changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] bmahler commented on issue #356: libprocess: check protobuf
(de)serialisation success.
Posted by GitBox <gi...@apache.org>.
bmahler commented on issue #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#issuecomment-610660551
> On a slightly-tangential subject: how do you deal with formatting?
I can see that there's a support/clang-format file, but applying it with e.g. clang-format version 7 results in many changes - I guess the code base hasn't been re-formatted to avoid churn?
I personally don't use clang-format but some others in the project do. At this point as long as your commit can pass the pre-commit hooks it's ok, but reviewers might have differing formatting preferences as we don't have a single way: http://mesos.apache.org/documentation/latest/c++-style-guide/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] bmahler commented on a change in pull request #356:
libprocess: check protobuf (de)serialisation success.
Posted by GitBox <gi...@apache.org>.
bmahler commented on a change in pull request #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#discussion_r405086348
##########
File path: 3rdparty/libprocess/include/process/protobuf.hpp
##########
@@ -39,8 +39,11 @@ inline void post(const process::UPID& to,
const google::protobuf::Message& message)
{
std::string data;
- message.SerializeToString(&data);
- post(to, message.GetTypeName(), data.data(), data.size());
+ if (message.SerializeToString(&data)) {
+ post(to, message.GetTypeName(), data.data(), data.size());
+ } else {
+ LOG(WARNING) << "Error serialising: " << message.DebugString();
Review comment:
I don't think we can log the message here since it may be huge (e.g. 2GB), how about an ERROR level message like:
> Failed to post 'UpdateSlaveMessage' to slave@192.168.0.1:5050: Failed to serialize
(you can just `<< to` to log the destination, and `m.GetTypeName()` for the name).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] cf-natali commented on issue #356: libprocess: check
protobuf (de)serialisation success.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on issue #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#issuecomment-610241483
> Can you extract the contributors.yaml change from this PR? We enforce that commits in the project do not mix unrelated changes.
Done.
> Would you be able to file a JIRA ticket? If you'd rather not, that's ok, I can file one for you, as we'll need one for this bug to be referenced and for this to make it into the CHANGELOG.
https://issues.apache.org/jira/browse/MESOS-10110
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] bmahler commented on issue #356: libprocess: check protobuf
(de)serialisation success.
Posted by GitBox <gi...@apache.org>.
bmahler commented on issue #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#issuecomment-610590957
Took a look at the protobuf code:
> Before the code didn't check at all the return value of
> Message::SerializeToString, which can fail for various reasons,
> e.g. out-of-memory, message too large, or invalid UTF-8 string.
As far as I see, `MessageLite::SerializeToString` boils down to [this code](https://github.com/protocolbuffers/protobuf/blob/v3.11.4/src/google/protobuf/message_lite.cc#L422-L436):
```
bool MessageLite::AppendPartialToString(std::string* output) const {
size_t old_size = output->size();
size_t byte_size = ByteSizeLong();
if (byte_size > INT_MAX) {
GOOGLE_LOG(ERROR) << GetTypeName()
<< " exceeded maximum protobuf size of 2GB: " << byte_size;
return false;
}
STLStringResizeUninitialized(output, old_size + byte_size);
uint8* start =
reinterpret_cast<uint8*>(io::mutable_string_data(output) + old_size);
SerializeToArrayImpl(*this, start, byte_size);
return true;
}
```
This will only return false for the message being too large? Where do you see out of memory being handled? It also looks like invalid UTF-8 does not fail serialization, but rather only logs an error for "proto3" always and logs an error in debug mode for "proto2", see [this example of a proto3 message](https://github.com/protocolbuffers/protobuf/blob/v3.11.4/src/google/protobuf/any.pb.cc#L214).
On the parsing side, looks like the possible cases for a false return are: `!IsInitialized()` [[1]](https://github.com/protocolbuffers/protobuf/blob/v3.11.4/src/google/protobuf/message_lite.h#L529)[[2]](https://github.com/protocolbuffers/protobuf/blob/v3.11.4/src/google/protobuf/message_lite.h#L475-L479), didn't consume entire message [[3]](https://github.com/protocolbuffers/protobuf/blob/v3.11.4/src/google/protobuf/message_lite.cc#L135), invalid message data (e.g. invalid varint, length of a string / submessage goes out of bounds). For invalid UTF-8, it looks [pretty complicated](https://github.com/protocolbuffers/protobuf/blob/e667bf6eaaa2fb1ba2987c6538df81f88500d030/src/google/protobuf/compiler/cpp/cpp_helpers.cc#L1034-L1101):
* If the message is "proto3", it seems to always validate UTF-8 by directly calling `WireFormatLite::VerifyUtf8String` and will then strictly fail parsing if not UTF-8.
* For "proto2" code, it seems to call [`WireFormat::VerifyUTF8StringNamedField`](https://github.com/protocolbuffers/protobuf/blob/df6e3a2f54ca893635119e98688a47ad85ecca26/src/google/protobuf/wire_format.h#L355-L381) and **only log** if it's invalid UTF-8, not fail. Also the code seems to be within a `#ifdef GOOGLE_PROTOBUF_UTF8_VALIDATION_ENABLED` which in turns is [only enabled if in a debug build i.e. `!NDEBUG`](https://github.com/protocolbuffers/protobuf/blob/e667bf6eaaa2fb1ba2987c6538df81f88500d030/src/google/protobuf/wire_format_lite.h#L54-L57). I'm a little puzzled at how you saw the logging, is your libprotobuf using debug mode? If you're using protoc from mesos I wonder if the way it's built in mesos is hitting the same issue as https://github.com/Homebrew/legacy-homebrew/issues/9279.
Here's a summary of my findings:
**Serialization of invalid UTF-8**:
* "proto3": serialization will succeed but invalid UTF-8 will be logged.
* "proto2": serialization will succeed. Invalid UTF-8 will only be logged in debug mode (NDEBUG not defined).
**De-serialization of invalid UTF-8**:
* "proto3": de-serialization will fail and it will be logged.
* "proto2": de-serialization will succeed. Invalid UTF-8 will only be logged in debug mode (NDEBUG not defined).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [mesos] cf-natali commented on issue #356: libprocess: check
protobuf (de)serialisation success.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on issue #356: libprocess: check protobuf (de)serialisation success.
URL: https://github.com/apache/mesos/pull/356#issuecomment-610643627
> This will only return false for the message being too large? Where do you see out of memory being handled?
Agreed. I *thought* it would handle it, but it looks like it just lets `std::bad_alloc` propagate.
> It also looks like invalid UTF-8 does not fail serialization, but rather only logs an error for "proto3" always and logs an error in debug mode for "proto2", see [this example of a proto3 message](https://github.com/protocolbuffers/protobuf/blob/v3.11.4/src/google/protobuf/any.pb.cc#L214).
Indeed, it's a bit of a mess.
Basically what happened in our case is that the framework re-serialises a subset of the messages received from the master into an event log, but uses proto3 - so serialisation would work, but then deserialising this would fail...
It's a bit sad that with proto3 serialising invalid uft8 doesn't fail but deserialising fails, it's contrary to Postel's principle :).
> I'm a little puzzled at how you saw the logging, is your libprotobuf using debug mode?
Interesting - we actually build our own libprotobuf - I'll ask our IT to double-check.
> **Serialization of invalid UTF-8**:
>
> * "proto3": serialization will succeed but invalid UTF-8 will be logged.
>
> * "proto2": serialization will succeed. Invalid UTF-8 will only be logged in debug mode (NDEBUG not defined).
>
>
> **De-serialization of invalid UTF-8**:
>
> * "proto3": de-serialization will fail and it will be logged.
>
> * "proto2": de-serialization will succeed. Invalid UTF-8 will only be logged in debug mode (NDEBUG not defined).
Yes, it also matches my testing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services