metadata: avoid change metadata ref #566
Conversation
Signed-off-by: Jay Lee <[email protected]>
After metadata is received, every entry should be owned by the grpc c core, application should clone the byte slice if necessary. In the past, we get around the problem by increasing the reference count of the slice. However, it's unsafe to do so as the slice is not guaranteed to be accessible in the first place. This PR fixes the problem by introducing an unowned metadata type. It works just like Metadata, but accessing its content requires manual check for the lifetime of associated call. Signed-off-by: Jay Lee <[email protected]>
Signed-off-by: Jay Lee <[email protected]>
|
Long time tests show that tikv/tikv#12202 and tikv/tikv#12198 are resolved by this patch. |
can you explain that? |
| unsafe { mem::transmute(Metadata::with_capacity(0)) } | ||
| } | ||
| #[inline] | ||
| pub unsafe fn assume_valid(&self) -> &Metadata { |
There was a problem hiding this comment.
maybe convert_to_owned better
There was a problem hiding this comment.
It's not a convertion. It's more like a as_ref/deref. The returned value is not owned, it just reuses the struct for accessing APIs. That's why a reference is returned.
| impl Drop for UnownedMetadata { | ||
| #[inline] | ||
| fn drop(&mut self) { | ||
| unsafe { grpcio_sys::grpcwrap_metadata_array_destroy_metadata_only(&mut self.0) } |
There was a problem hiding this comment.
why free here because I don't see any pointer init
There was a problem hiding this comment.
The order of handling batch result and destroying a call is uncertain. So it's possible that when the batch result is returned from the queue_next, the corresponding grpc call is already destroyed. In that case, referencing internal slices is UAF, and destroy the metadata later will dereference it, which is a double free. |
Signed-off-by: Jay Lee <[email protected]>
| @@ -147,7 +147,7 @@ impl MetadataBuilder { | |||
| /// | |||
| /// Metadata value can be ascii string or bytes. They are distinguish by the | |||
| /// key suffix, key of bytes value should have suffix '-bin'. | |||
| #[repr(C)] | |||
| #[repr(transparent)] | |||
There was a problem hiding this comment.
seem that all struct with single field using repr(C) can be replaced with repr(transparent).
Signed-off-by: Jay Lee <[email protected]>
After metadata is received, every entry should be owned by the grpc c
core, application should clone the byte slice if necessary.
In the past, we get around the problem by increasing the reference
count of the slice. However, it's unsafe to do so as the slice is not
guaranteed to be accessible in the first place.
This PR fixes the problem by introducing an unowned metadata type. It
works just like Metadata, but accessing its content requires manual
check for the lifetime of associated call.
I tried to add test case to cover the unsafe access, however it's too racy
and nearly impossible to introduce a case without touching the C core.