Mandate static string for host and method passed to grpc_channel_register #19263
Conversation
soheilhy
added
the
release notes: yes
| rc->path = grpc_mdelem_from_slices( | ||
| GRPC_MDSTR_PATH, | ||
| grpc_slice_intern(grpc_slice_from_static_string(method))); | ||
| rc->path = grpc_mdelem_from_slices(GRPC_MDSTR_PATH, |
There was a problem hiding this comment.
I am worried that we might end up using the slices beyond the lifetime of the channel. Since we aren't explicitly requiring static strings that might end up leading to hard to debug use-after-free bugs
There was a problem hiding this comment.
Right now, when the channel is done, it unrefs the slices, right?
Which implies that if someone manages to use it after the channel is done, then it had called ref before the channel called unref, right?
There was a problem hiding this comment.
A possible compromise, then, would be to use grpc_slice_from_copied_string? That I believe gives us a malloc'd, refcounted slice.
There was a problem hiding this comment.
(We'd be doing more atomics than the static_string() case, but we'd still win out by not needing to go through the relatively heavy handed intern implementation).
There was a problem hiding this comment.
We are changing the API via gRFC to make this static string. I don't think we need any extra copies or allocations.
soheilhy
force-pushed
the
core-static-method
branch
from
564c380
to
a85c6d0
Compare
|
Where's the major version bump? |
soheilhy
force-pushed
the
core-static-method
branch
from
a85c6d0
to
2a10e83
Compare
|
Apologies @vjpai I wasn't aware of the major version bump process. I updated the PR. Could you PTAL? Thanks! |
soheilhy
force-pushed
the
core-static-method
branch
2 times, most recently
from
5cd0511
to
318a580
Compare
src/core/lib/gprpp/memory.h
Outdated
| @@ -30,6 +30,9 @@ | |||
|
|
|||
| // Add this to a class that want to use Delete(), but has a private or | |||
| // protected destructor. | |||
| // Should not be used in new code. | |||
| // TODO(juanlishen): Remove this macro, and instead comment that the public dtor | |||
| // should not be used directly. | |||
There was a problem hiding this comment.
Thank you for catching this, but I can't find this in my commit locally. It's weird.
I pull+rebased again and it looks gone now.
…ster_call This is for the gRFC being prepared. Bump the major version of core to 8.0.0.
soheilhy
force-pushed
the
core-static-method
branch
from
318a580
to
13896f8
Compare
|
Thanks for the approval! |
|
Known issue #19819 |
This is the implementation of grpc/proposal#148
It requires a major version bump.