[Code health] Fix unsafe buffer usage in lib/bindings_internal.h [chromium/src : main]
Daniel Soromou (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou voted Commit-Queue+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Daniel Cheng
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
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")
if (!base_addr.IsValid()) {
return nullptr;
}
So this still doesn't prove that the offset is withing the object, just that it hasn't overflowed. So it's still unsafe to smuggle a pointer in this manner,
Related details
- Arthur Sonzogni
- Daniel Cheng
- Daniel Soromou
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")
inline const void* DecodePointer(const uint64_t* offset) {@ajgo - how do we validate the result of this before use?
Related details
- Alex Gough
- Arthur Sonzogni
- Daniel Cheng
- Daniel Soromou
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. |
Alex Gough (Gerrit)
Alex Gough added 1 comment![]( "Open in Gerrit")
inline const void* DecodePointer(const uint64_t* offset) {@ajgo - how do we validate the result of this before use?
mojo does a validation pass before its decoding pass e.g. https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/bindings/lib/map_data_internal.h;drc=a416f377ead3d8c361b97126daf78381344937dd;bpv=1;bpt=1;l=52 so adding more validation here might not be appropriate
Related details
Alex Gough (Gerrit)
Alex Gough added 1 comment![]( "Open in Gerrit")
Generated with gemini-cliwhat was the prompt and how did you teach it about mojo serialization?
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
inline const void* DecodePointer(const uint64_t* offset) {Alex Gough@ajgo - how do we validate the result of this before use?
mojo does a validation pass before its decoding pass e.g. https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/bindings/lib/map_data_internal.h;drc=a416f377ead3d8c361b97126daf78381344937dd;bpv=1;bpt=1;l=52 so adding more validation here might not be appropriate
Ah. So in theory, we could write:
```
UNSAFE_BUFFER_USAGE inline const void* DecodePointer(const uint64_t* offset) {
if (!*offset)
return nullptr;
// SAFETY: required from caller, enforced by UNSAFE_BUFFER_USAGE.
return UNSAFE_BUFFERS(reinterpret_cast<const char*>(offset) + *offset);
}
```
but then you play the game of writing // PRECONDITIONS: comments in the callers.
Daniel Soromou (Gerrit)
Daniel Soromou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Soromou
| Commit-Queue | +1 |
1 comment
Generated with gemini-cliwhat was the prompt and how did you teach it about mojo serialization?
The prompt and tools for the fix generation can be found here: agents/prompts/projects/spanification/. There is not a specific mojo serialization knowledge. Please, is there any specification consideration for mojo spanification?
Related details
- Alex Gough
- Arthur Sonzogni
- 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")
inline const void* DecodePointer(const uint64_t* offset) {Alex Gough@ajgo - how do we validate the result of this before use?
Tom Sepezmojo does a validation pass before its decoding pass e.g. https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/bindings/lib/map_data_internal.h;drc=a416f377ead3d8c361b97126daf78381344937dd;bpv=1;bpt=1;l=52 so adding more validation here might not be appropriate
Ah. So in theory, we could write:
```
UNSAFE_BUFFER_USAGE inline const void* DecodePointer(const uint64_t* offset) {
if (!*offset)
return nullptr;
// SAFETY: required from caller, enforced by UNSAFE_BUFFER_USAGE.
return UNSAFE_BUFFERS(reinterpret_cast<const char*>(offset) + *offset);
}
```
but then you play the game of writing // PRECONDITIONS: comments in the callers.
I think it would be reasonable to mark `DecodePointer()` (and maybe `EncodePointer()`) as unsafe and similarly force `Pointer::Get()` and `Pointer::Set()` to be unsafe as well.
Related details
- Alex Gough
- Arthur Sonzogni
- Daniel Soromou
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. |