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.

Sign up for GitHub

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

Jump to bottom

Add compatibility with protobuf 28 #541

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Add compatibility with protobuf 28 #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8986a68
Add compatibility with protobuf 28
ramir0-sultanov Sep 19, 2024
ff01a7f
Add message verification after message down casts
ramir0-sultanov Oct 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion include/gz/transport/RepHandler.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,12 @@ namespace gz
return false;
}

#if GOOGLE_PROTOBUF_VERSION >= 4022000
#if GOOGLE_PROTOBUF_VERSION >= 5028000
const auto msgReq =
google::protobuf::DynamicCastMessage<Req>(&_msgReq);
auto msgRep =
google::protobuf::DynamicCastMessage<Rep>(&_msgRep);
#elif GOOGLE_PROTOBUF_VERSION >= 4022000
auto msgReq =
google::protobuf::internal::DownCast<const Req*>(&_msgReq);
auto msgRep = google::protobuf::internal::DownCast<Rep*>(&_msgRep);
Expand All @@ -154,6 +159,45 @@ namespace gz
auto msgRep = google::protobuf::internal::down_cast<Rep*>(&_msgRep);
#endif

// Verify the dynamically casted messages are valid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to move this block of verifications after all the ifdefs? That way we could validate all versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is me being extra cautious to not break parts of code that already work. After checking the source code of protobuf, I found that google::protobuf::Message::GetDescriptor() and google::protobuf::Descriptor::full_name() functions were in the initial commit. Assuming, they were not removed after that, I think, it is safe to validate messages using the specified block of code. The changes can be found in the corresponding commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caguero any thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is safe.

if (msgReq == nullptr || msgRep == nullptr)
{
if (msgReq == nullptr)
{
if (_msgReq.GetDescriptor() != nullptr)
{
std::cerr << "RepHandler::RunLocalCallback() error: "
<< "Failed to cast the request of the type "
<< _msgReq.GetDescriptor()->full_name()
<< " to the specified type" << '\n';
}
else
{
std::cerr << "RepHandler::RunLocalCallback() error: "
<< "Failed to cast the request of an unknown type"
<< " to the specified type" << '\n';
}
}
if (msgRep == nullptr)
{
if (_msgRep.GetDescriptor() != nullptr)
{
std::cerr << "RepHandler::RunLocalCallback() error: "
<< "Failed to cast the response of the type "
<< _msgRep.GetDescriptor()->full_name()
<< " to the specified type" << '\n';
}
else
{
std::cerr << "RepHandler::RunLocalCallback() error: "
<< "Failed to cast the response of an unknown type"
<< " to the specified type" << '\n';
}
}
std::cerr.flush();
return false;
}

return this->cb(*msgReq, *msgRep);
}

Expand Down
24 changes: 23 additions & 1 deletion include/gz/transport/SubscriptionHandler.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,14 +214,36 @@ namespace gz
if (!this->UpdateThrottling())
return true;

#if GOOGLE_PROTOBUF_VERSION >= 4022000
#if GOOGLE_PROTOBUF_VERSION >= 5028000
auto msgPtr = google::protobuf::DynamicCastMessage<T>(&_msg);
#elif GOOGLE_PROTOBUF_VERSION >= 4022000
auto msgPtr = google::protobuf::internal::DownCast<const T*>(&_msg);
#elif GOOGLE_PROTOBUF_VERSION >= 3000000
auto msgPtr = google::protobuf::down_cast<const T*>(&_msg);
#else
auto msgPtr = google::protobuf::internal::down_cast<const T*>(&_msg);
#endif

// Verify the dynamically casted message is valid
if (msgPtr == nullptr)
{
if (_msg.GetDescriptor() != nullptr)
{
std::cerr << "SubscriptionHandler::RunLocalCallback() error: "
<< "Failed to cast the message of the type "
<< _msg.GetDescriptor()->full_name()
<< " to the specified type" << '\n';
}
else
{
std::cerr << "SubscriptionHandler::RunLocalCallback() error: "
<< "Failed to cast the message of an unknown type"
<< " to the specified type" << '\n';
}
std::cerr.flush();
return false;
}

this->cb(*msgPtr, _info);
return true;
}
Expand Down