spanification: automatically spanify sandbox/linux/services/credentials.cc [chromium/src : main]
Yuki Shiino (Gerrit)
Yuki Shiino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuki Shiino
| Code-Review | +1 |
Related details
- Keishi Hattori
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keishi Hattori
| Code-Review | +1 |
1 comment
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 1 comment![]( "Open in Gerrit")
Matthew, would you review this patch as an owner?
Related details
- Keishi Hattori
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton added 1 comment![]( "Open in Gerrit")
void* stack = base::span<char>(stack_buf)
.subspan(base::SpanificationSizeofForStdArray(stack_buf))
.data();This seems very verbose, is there no better way to get a pointer to the "one past the end" of an array?
Related details
- Keishi Hattori
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 2 comments![]( "Open in Gerrit")
Thank you for the quick review. I addressed the review comment.
void* stack = base::span<char>(stack_buf)
.subspan(base::SpanificationSizeofForStdArray(stack_buf))
.data();This seems very verbose, is there no better way to get a pointer to the "one past the end" of an array?
- Keishi Hattori
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthew Denton
| Code-Review | +1 |
1 comment
- Keishi Hattori
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton added 1 comment![]( "Open in Gerrit")
void* stack = base::span<char>(stack_buf)
.subspan(base::SpanificationSizeofForStdArray(stack_buf))
.data();Yuki ShiinoThis seems very verbose, is there no better way to get a pointer to the "one past the end" of an array?
I introduced a helper function. Does this look fine?
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori voted and added 2 comments![]( "Open in Gerrit")
Votes added by Keishi Hattori
| Code-Review | +1 |
2 comments
LGTM
constexpr ElementMaybeConst* SpanificationEndPtrOf(one past the end pointers are scary so it would be nice to have a helper function to make them stand out.
Related details
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton added 1 comment![]( "Open in Gerrit")
constexpr ElementMaybeConst* SpanificationEndPtrOf(one past the end pointers are scary so it would be nice to have a helper function to make them stand out.
Perhaps if we don't want this, this particular part of the code could be a good candidate for UNSAFE_BUFFERS() and safety comment since there is really no danger of unsafety here, and the pointer addition is pretty clear.
Related details
- Keishi Hattori
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton added 1 comment![]( "Open in Gerrit")
constexpr ElementMaybeConst* SpanificationEndPtrOf(Matthew Dentonone past the end pointers are scary so it would be nice to have a helper function to make them stand out.
Perhaps if we don't want this, this particular part of the code could be a good candidate for UNSAFE_BUFFERS() and safety comment since there is really no danger of unsafety here, and the pointer addition is pretty clear.
It could also return a subspan of zero length like the original change was doing, and then we just call data() on that.
Yuki Shiino (Gerrit)
Yuki Shiino added 2 comments![]( "Open in Gerrit")
Thank you both. Would you take another look?
+R=nuskos@ for owner review of auto_spanification_helper.h.
constexpr ElementMaybeConst* SpanificationEndPtrOf(Matthew Dentonone past the end pointers are scary so it would be nice to have a helper function to make them stand out.
Matthew DentonPerhaps if we don't want this, this particular part of the code could be a good candidate for UNSAFE_BUFFERS() and safety comment since there is really no danger of unsafety here, and the pointer addition is pretty clear.
It could also return a subspan of zero length like the original change was doing, and then we just call data() on that.
Would you folks take another look?
Taking your advice, I annotated the helper functions with UNSAFE_BUFFER_USAGE (which requires UNSAFE_BUFFERS on call sites), and added UNSAFE_BUFFERS to the call site. PRECONDITION: and SAFETY: comments are added, too.
Related details
- Keishi Hattori
- Matthew Denton
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthew Denton
| Code-Review | +1 |
1 comment
constexpr ElementMaybeConst* SpanificationEndPtrOf(Matthew Dentonone past the end pointers are scary so it would be nice to have a helper function to make them stand out.
Matthew DentonPerhaps if we don't want this, this particular part of the code could be a good candidate for UNSAFE_BUFFERS() and safety comment since there is really no danger of unsafety here, and the pointer addition is pretty clear.
Yuki ShiinoIt could also return a subspan of zero length like the original change was doing, and then we just call data() on that.
Would you folks take another look?
Taking your advice, I annotated the helper functions with UNSAFE_BUFFER_USAGE (which requires UNSAFE_BUFFERS on call sites), and added UNSAFE_BUFFERS to the call site. PRECONDITION: and SAFETY: comments are added, too.
- Keishi Hattori
- Stephen Nusko
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
// SpanificationEndPtrOf was introduced temporarily in order to help the autoIf this is temporary what do we expect people to actually write?
```
// Safety: stack is never referenced?
void* stack = UNSAFE_BUFFER(stack_buf.data() + stack_buf.size());
```
I see in the other comment chain @kei...@chromium.org was suggesting another function to make it clear, but then we expect that to be the end result no?
So I think either
1) We keep the function but remove the mention of temporary and perhaps call it EndPtrOf(), then we expect UNSAFE_BUFFER(EndPtrOf(stack_buf)) with a safety promising it won't be referenced.
2) We just keep it as UNSAFE_BUFFER(stack_buf.data() + stack_buf.size()) because that is what we expect long term people to write.
Thoughts? I think we should be consistent and give advice for folks in this scenario.
Related details
- Keishi Hattori
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 2 comments![]( "Open in Gerrit")
The patch became so simple now. Would you take yet another look?
// SpanificationEndPtrOf was introduced temporarily in order to help the autoIf this is temporary what do we expect people to actually write?
```
// Safety: stack is never referenced?
void* stack = UNSAFE_BUFFER(stack_buf.data() + stack_buf.size());
```I see in the other comment chain @kei...@chromium.org was suggesting another function to make it clear, but then we expect that to be the end result no?
So I think either
1) We keep the function but remove the mention of temporary and perhaps call it EndPtrOf(), then we expect UNSAFE_BUFFER(EndPtrOf(stack_buf)) with a safety promising it won't be referenced.
2) We just keep it as UNSAFE_BUFFER(stack_buf.data() + stack_buf.size()) because that is what we expect long term people to write.
Thoughts? I think we should be consistent and give advice for folks in this scenario.
I agree that this will be the end state in this specific case. For other cases, we should encourage the use of iterators instead of raw pointers.
I go with Option 2) UNSAFE_BUFFERS(stack_buf.data() + stack_buf.size()) with a SAFETY: comment.
Related details
- Keishi Hattori
- Matthew Denton
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
// SpanificationEndPtrOf was introduced temporarily in order to help the autoYuki ShiinoIf this is temporary what do we expect people to actually write?
```
// Safety: stack is never referenced?
void* stack = UNSAFE_BUFFER(stack_buf.data() + stack_buf.size());
```I see in the other comment chain @kei...@chromium.org was suggesting another function to make it clear, but then we expect that to be the end result no?
So I think either
1) We keep the function but remove the mention of temporary and perhaps call it EndPtrOf(), then we expect UNSAFE_BUFFER(EndPtrOf(stack_buf)) with a safety promising it won't be referenced.
2) We just keep it as UNSAFE_BUFFER(stack_buf.data() + stack_buf.size()) because that is what we expect long term people to write.
Thoughts? I think we should be consistent and give advice for folks in this scenario.
I agree that this will be the end state in this specific case. For other cases, we should encourage the use of iterators instead of raw pointers.
I go with Option 2) UNSAFE_BUFFERS(stack_buf.data() + stack_buf.size()) with a SAFETY: comment.
- Keishi Hattori
- Matthew Denton
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
- Keishi Hattori
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino voted and added 1 comment![]( "Open in Gerrit")
Related details
- Keishi Hattori
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuki Shiino
| Commit-Queue | +2 |
1 comment
constexpr ElementMaybeConst* SpanificationEndPtrOf(Matthew Dentonone past the end pointers are scary so it would be nice to have a helper function to make them stand out.
Matthew DentonPerhaps if we don't want this, this particular part of the code could be a good candidate for UNSAFE_BUFFERS() and safety comment since there is really no danger of unsafety here, and the pointer addition is pretty clear.
Yuki ShiinoIt could also return a subspan of zero length like the original change was doing, and then we just call data() on that.
Matthew DentonWould you folks take another look?
Taking your advice, I annotated the helper functions with UNSAFE_BUFFER_USAGE (which requires UNSAFE_BUFFERS on call sites), and added UNSAFE_BUFFERS to the call site. PRECONDITION: and SAFETY: comments are added, too.
Thanks, still +1
Done
Related details
- Keishi Hattori
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
spanification: automatically spanify sandbox/linux/services/credentials.cc
This is the result of running the automatic spanification on linux and
updating code to use and pass spans where size is known.
The original patch was fully automated using script:
//tools/clang/spanify/rewrite-multiple-platforms.sh -platforms=linux
Then refined with gemini-cli
gemini-run/batch-run-1755453491/group_100
- M sandbox/linux/services/credentials.cc
Code-Review: +1 by Yuki Shiino, +1 by Matthew Denton, +1 by Stephen Nusko
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |