serialization of service response message when response was failed

[canalteks]

Dear developers,

we suspect that ros::serialization::serializeServiceResponse had a bug to assign unused memory unsafely.

when service response returned false (argument "ok" == false), else statement assigned (sizeof(message) + 1) bytes. the following codes in the else-statement, serializatio of "ok" flag will be safe, but unfortunately, the next serialization of message should be unsafe because lack of message size information before that data.

if roscpp sent a message data in the else-statement, we thought that it's neccesary to serialize size of message like in "if-true-statement". if not, we thought it's better to set m.num_bytes to "1" and remove this sentence.

we hope it could be a help.

[peci1]

Could you provide a reproducible example where this problem manifests?

[canalteks]

@peci1 if user's service server callback returned false, the above code would be called. for example, see code or code.

[peci1]

What is important is what the deserialization code is doing. It happens in ServiceServerLink::onResponseOkAndLength():

https://github.com/ros/ros_comm/blob/030e132884d613e49a576d4339f0b8ec6f75d2d8/clients/roscpp/src/libros/service_server_link.cpp#L196-L197

uint8_t ok = buffer[0];
uint32_t len = *((uint32_t*)(buffer.get() + 1));

And further, https://github.com/ros/ros_comm/blob/noetic-devel/clients/roscpp/src/libros/service_server_link.cpp#L219-L226

if (len > 0)
{
  connection_->read(...);
}
else
{
  onResponse(conn, boost::shared_array<uint8_t>(), 0, true);
}

Next, as you pointed out, downstream callers always do this in case of service failure:

SerializedMessage res = serialization::serializeServiceResponse<uint32_t>(false, 0);

Now, this might seem a bit weird and misuse of the API, but I think it works.

Instead of writing a zero-length message, the code writes a message containing a 4-byte zero. And this 4-byte zero is read by the deserializer as length of the message. And if it finds a zero-length message, it does not read anything else.

[canalteks]

@peci1 as you mentioned, it works and it's the case to misuse API. and as we commented above, for ros::serialization::serializeServiceResponse, there are two ways to address that case. excuse me but, we consider that misuse is a theoretical bug.

[peci1]

I agree with you. I created #138 where a comment is added explaining this misuse. Could you please give a review to the PR?

The misuse will definitely not get fixed in Noetic. As the last ROS 1 release, it should not receive any big changes to such a core part of the system. It is safer to keep the weird behavior and have it documented than risking to break existing code with unexpected changes.

Thanks for figuring this out and creating this issue.

[canalteks]

@peci1 thank you for the PR. we also agree with what you said.