[EXPLORATION] Avoid unowned pickles in IPC code [chromium/src : main]
Mikel Astiz (Gerrit)
Mikel Astiz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Commit-Queue | +1 |
2 comments
dch...@chromium.org WDYT or did I get too excited with modernizing base::Pickle?
virtual ~PickleIterator();I would be in favor of avoiding this virtual destructor to keep the size of PickleIterator small.
To do so, `ipc::MessageIterator` would need to fork the entire PickleIterator API with inline function bodies that invoke PickleIterator's corresponding APIs, using a member field `PickleIterator` instead of inheritance.
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
So legacy IPC is essentially dead, but we have some bits that still remain due to `[Native]` messages.
I think it would probably be better to try to get rid of it (https://issues.chromium.org/issues/393179188) and see how much API surface is left.
The allowlist is at https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/tools/bindings/checks/mojom_attributes_check.py;l=98;drc=e7bae9e99c6257ba4d08e7dc0dcf27eb5f2573e6 and there's almost certainly some low-hanging fruit on there.
Related details
- Mikel Astiz
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
I've wanted this for quite a while, however, the IPC usage was something I tried to remove some months ago, but left in-place until [native] serialization could be removed. So like Daniel says, let's not muck with the IPC code.
Fixing this in all other places seems reasonable. Maybe make the non-owning constructors private and "friend" in the IPC callers?
Related details
- Mikel Astiz
Mikel Astiz (Gerrit)
Mikel Astiz added 3 comments![]( "Open in Gerrit")
Thanks!
I've wanted this for quite a while, however, the IPC usage was something I tried to remove some months ago, but left in-place until [native] serialization could be removed. So like Daniel says, let's not muck with the IPC code.
Fixing this in all other places seems reasonable. Maybe make the non-owning constructors private and "friend" in the IPC callers?
Ack. Let's continue the conversation in the other thread. The private approach you propose seems reasonable (or a better variant IMO would make it protected, and let ipc::Message declare friendship), but personally I would rather delete it altogether.
So legacy IPC is essentially dead, but we have some bits that still remain due to `[Native]` messages.
I think it would probably be better to try to get rid of it (https://issues.chromium.org/issues/393179188) and see how much API surface is left.
The allowlist is at https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/tools/bindings/checks/mojom_attributes_check.py;l=98;drc=e7bae9e99c6257ba4d08e7dc0dcf27eb5f2573e6 and there's almost certainly some low-hanging fruit on there.
I fear the bug linked above could be another rabbit hole and I feel like unblocking the removal of `Pickle::WithUnownedBuffer` (which I find very error-prone) asap would be a very good thing; do you agree?
If legacy IPC is semi-dead, does anything speak against submitting this patch?
I don't particularly see downsides but I'm seeking guidance, and in particular there's the question of whether the new API introduced in `PickleIterator` (with IPC just being a first user) is good. See the next patch for trivially migrating most callers, and there's there one or two more calling sites that require a bit more attention and a dedicated patch.
Related details
- Daniel Cheng
- Tom Sepez
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Mikel AstizSo legacy IPC is essentially dead, but we have some bits that still remain due to `[Native]` messages.
I think it would probably be better to try to get rid of it (https://issues.chromium.org/issues/393179188) and see how much API surface is left.
The allowlist is at https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/tools/bindings/checks/mojom_attributes_check.py;l=98;drc=e7bae9e99c6257ba4d08e7dc0dcf27eb5f2573e6 and there's almost certainly some low-hanging fruit on there.
I fear the bug linked above could be another rabbit hole and I feel like unblocking the removal of `Pickle::WithUnownedBuffer` (which I find very error-prone) asap would be a very good thing; do you agree?
If legacy IPC is semi-dead, does anything speak against submitting this patch?
I don't particularly see downsides but I'm seeking guidance, and in particular there's the question of whether the new API introduced in `PickleIterator` (with IPC just being a first user) is good. See the next patch for trivially migrating most callers, and there's there one or two more calling sites that require a bit more attention and a dedicated patch.
Yes, I don't really want to take a CL that changes a thousand lines given how little use of WithUnownedBuffer there is, especially if it means we're adding a virtual or duplicating functions. It's not likely there will be more uses introduced anyway.
Related details
- Mikel Astiz
- Tom Sepez
Mikel Astiz (Gerrit)
Mikel Astiz added 2 comments![]( "Open in Gerrit")
Thanks for thinking this through.
Mikel AstizSo legacy IPC is essentially dead, but we have some bits that still remain due to `[Native]` messages.
I think it would probably be better to try to get rid of it (https://issues.chromium.org/issues/393179188) and see how much API surface is left.
The allowlist is at https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/tools/bindings/checks/mojom_attributes_check.py;l=98;drc=e7bae9e99c6257ba4d08e7dc0dcf27eb5f2573e6 and there's almost certainly some low-hanging fruit on there.
Daniel ChengI fear the bug linked above could be another rabbit hole and I feel like unblocking the removal of `Pickle::WithUnownedBuffer` (which I find very error-prone) asap would be a very good thing; do you agree?
If legacy IPC is semi-dead, does anything speak against submitting this patch?
I don't particularly see downsides but I'm seeking guidance, and in particular there's the question of whether the new API introduced in `PickleIterator` (with IPC just being a first user) is good. See the next patch for trivially migrating most callers, and there's there one or two more calling sites that require a bit more attention and a dedicated patch.
Yes, I don't really want to take a CL that changes a thousand lines given how little use of WithUnownedBuffer there is, especially if it means we're adding a virtual or duplicating functions. It's not likely there will be more uses introduced anyway.
To make sure I understand your stance: is your point about IPC code specifically (which IIUC you would rather not refactor even mechanically in light of upcoming code cleanups) or more generally about the idea of deprecating `Pickle::WithUnownedBuffer` in favor of introducing `PickleIterator::WithUnownedData()`?
If the latter, I will sadly abandon this patch series (and stick to what I started with, which is spanifying the public interface). The main downside IMO is that Pickle's implementation cannot be simplified to adopt a standard container (e.g. HeapBuffer, vector or InlinedVector) and hopefully avoid unsafe buffer annotations.
If your point is about IPC code specifically, I will consider the middle ground that Tom suggested (other thread in this CL), which is to allowlist IPC for this WithUnownedBuffer, but migrate other callers to PickleIterator. In this scenario, Pickle's internal simplifications would need to wait until legacy IPC (or at least attachment support) is cleaned up. That said, before embracing this blockage, I would make another attempt with this patch because there's another variant that may address the concerns you have listed so far (except for the mechanical changes being large).
Related details
- Daniel Cheng
- Tom Sepez
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Mikel AstizSo legacy IPC is essentially dead, but we have some bits that still remain due to `[Native]` messages.
I think it would probably be better to try to get rid of it (https://issues.chromium.org/issues/393179188) and see how much API surface is left.
The allowlist is at https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/tools/bindings/checks/mojom_attributes_check.py;l=98;drc=e7bae9e99c6257ba4d08e7dc0dcf27eb5f2573e6 and there's almost certainly some low-hanging fruit on there.
Daniel ChengI fear the bug linked above could be another rabbit hole and I feel like unblocking the removal of `Pickle::WithUnownedBuffer` (which I find very error-prone) asap would be a very good thing; do you agree?
If legacy IPC is semi-dead, does anything speak against submitting this patch?
I don't particularly see downsides but I'm seeking guidance, and in particular there's the question of whether the new API introduced in `PickleIterator` (with IPC just being a first user) is good. See the next patch for trivially migrating most callers, and there's there one or two more calling sites that require a bit more attention and a dedicated patch.
Mikel AstizYes, I don't really want to take a CL that changes a thousand lines given how little use of WithUnownedBuffer there is, especially if it means we're adding a virtual or duplicating functions. It's not likely there will be more uses introduced anyway.
To make sure I understand your stance: is your point about IPC code specifically (which IIUC you would rather not refactor even mechanically in light of upcoming code cleanups) or more generally about the idea of deprecating `Pickle::WithUnownedBuffer` in favor of introducing `PickleIterator::WithUnownedData()`?
If the latter, I will sadly abandon this patch series (and stick to what I started with, which is spanifying the public interface). The main downside IMO is that Pickle's implementation cannot be simplified to adopt a standard container (e.g. HeapBuffer, vector or InlinedVector) and hopefully avoid unsafe buffer annotations.
If your point is about IPC code specifically, I will consider the middle ground that Tom suggested (other thread in this CL), which is to allowlist IPC for this WithUnownedBuffer, but migrate other callers to PickleIterator. In this scenario, Pickle's internal simplifications would need to wait until legacy IPC (or at least attachment support) is cleaned up. That said, before embracing this blockage, I would make another attempt with this patch because there's another variant that may address the concerns you have listed so far (except for the mechanical changes being large).
I'd rather leave all the existing ParamTraits alone at this point if at all possible. I'm fine with spanifying Pickle's API, and I think it's OK (if suboptimal) it uses some `UNSAFE_BUFFERS()` internally until a time we don't have to deal with `ParamTraits`.
Related details
- Mikel Astiz
- Tom Sepez
Mikel Astiz (Gerrit)
Mikel Astiz added 2 comments![]( "Open in Gerrit")
Mikel AstizSo legacy IPC is essentially dead, but we have some bits that still remain due to `[Native]` messages.
I think it would probably be better to try to get rid of it (https://issues.chromium.org/issues/393179188) and see how much API surface is left.
The allowlist is at https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/tools/bindings/checks/mojom_attributes_check.py;l=98;drc=e7bae9e99c6257ba4d08e7dc0dcf27eb5f2573e6 and there's almost certainly some low-hanging fruit on there.
Daniel ChengI fear the bug linked above could be another rabbit hole and I feel like unblocking the removal of `Pickle::WithUnownedBuffer` (which I find very error-prone) asap would be a very good thing; do you agree?
If legacy IPC is semi-dead, does anything speak against submitting this patch?
I don't particularly see downsides but I'm seeking guidance, and in particular there's the question of whether the new API introduced in `PickleIterator` (with IPC just being a first user) is good. See the next patch for trivially migrating most callers, and there's there one or two more calling sites that require a bit more attention and a dedicated patch.
Mikel AstizYes, I don't really want to take a CL that changes a thousand lines given how little use of WithUnownedBuffer there is, especially if it means we're adding a virtual or duplicating functions. It's not likely there will be more uses introduced anyway.
Daniel ChengTo make sure I understand your stance: is your point about IPC code specifically (which IIUC you would rather not refactor even mechanically in light of upcoming code cleanups) or more generally about the idea of deprecating `Pickle::WithUnownedBuffer` in favor of introducing `PickleIterator::WithUnownedData()`?
If the latter, I will sadly abandon this patch series (and stick to what I started with, which is spanifying the public interface). The main downside IMO is that Pickle's implementation cannot be simplified to adopt a standard container (e.g. HeapBuffer, vector or InlinedVector) and hopefully avoid unsafe buffer annotations.
If your point is about IPC code specifically, I will consider the middle ground that Tom suggested (other thread in this CL), which is to allowlist IPC for this WithUnownedBuffer, but migrate other callers to PickleIterator. In this scenario, Pickle's internal simplifications would need to wait until legacy IPC (or at least attachment support) is cleaned up. That said, before embracing this blockage, I would make another attempt with this patch because there's another variant that may address the concerns you have listed so far (except for the mechanical changes being large).
I'd rather leave all the existing ParamTraits alone at this point if at all possible. I'm fine with spanifying Pickle's API, and I think it's OK (if suboptimal) it uses some `UNSAFE_BUFFERS()` internally until a time we don't have to deal with `ParamTraits`.
Understood that the ParamTraits should remain unchanged (definitely possible). It is also clear that spanifying Pickle's API is something you are fine with (gentle ping for https://crrev.com/c/7516607 in that case).
The remaining question is whether you would like to deprecate Pickle::WithUnownedBuffer() in favor of a newly-introduced equivalent API in PickleIterator, even if IPC code doesn't migrate to it (which is a temporary issue). To make this question more specific, whether or not I should pursue https://chromium-review.googlesource.com/c/chromium/src/+/7511849 (which now took the PickleIterator API from this patch, excluding the virtual destructor) and other remaining callers (excluding IPC code).
Thanks!
- Daniel Cheng
- Tom Sepez