fix: avoid use-after-move StatusOr #7635
Conversation
google-cla
bot
added
the
cla: yes
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7635 +/- ##
==========================================
- Coverage 95.28% 95.27% -0.01%
==========================================
Files 1251 1251
Lines 112992 112994 +2
==========================================
- Hits 107659 107653 -6
- Misses 5333 5341 +8
Continue to review full report at Codecov.
|
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
google/cloud/stream_range.h
Outdated
| @@ -186,6 +186,7 @@ class StreamRange { | |||
| }; | |||
| auto v = reader_(); | |||
| absl::visit(UnpackVariant{*this}, std::move(v)); | |||
| current_ok_ = current_.ok(); | |||
There was a problem hiding this comment.
This one is a little strange in the "received final OK" case, where UnpackVariant didn't assign to current_, so we are possibly looking at a move-from StatusOr. Not that that matters if ok() is defined in that state, but perhaps it would be nicer to just make the sr.current_ assignment unconditional.
There was a problem hiding this comment.
Good observation. I can't unconditionally assign to sr.current_, because it's not allowed to assign an OK Status to a StatusOr. However, I did change the code a bit to be clearer and more correct by moving the sr.current_ok_ assignment into the UnpackVariant functor, so it can set it correctly in each case. PTAL.
There was a problem hiding this comment.
OK, sounds good.
Aside: In the bigger picture I still find the control flow confusing. For example, there are really only three states ...
{ => T } S1 [ => non-OK ] S2 [ => end ] S3
... but we currently use all four. To that end, I might make the initial state S1 ...
bool current_ok_ = true;
bool is_end_ = false;
and also stop S3 from ever calling reader_().
|
FWIW, you control |
Similar to: googleapis#7588 This code allowed callers to (rightfully) move-out the `StatusOr` that was held in `current_`. Therefore, it's a bug if we then consult the status of `current_.ok()`, because it may be in a moved-from state. The fix is to track the `current_ok_` bit separately, so that we only ever assign to `current_` and never read it after it could have been moved.
devjgm
force-pushed
the
fix-stream-range-moved-from
branch
from
1543f96
to
8477b5f
Compare
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
True. I'd prefer to avoid defining specifically what the moved-from state is for Status or StatusOr, if possible. Especially, for non-OK situations where we'd be losing the message, payload, etc. If we need to reach for that tool, we can, but avoiding it seems preferable, IMO. Plus, I have a PR right behind this one where preserving the |
Similar to: #7588
This code allowed callers to (rightfully) move-out the
StatusOrthatwas held in
current_. Therefore, it's a bug if we then consult thestatus of
current_.ok(), because it may be in a moved-from state.The fix is to track the
current_ok_bit separately, so that we onlyever assign to
current_and never read it after it could have beenmoved.
This change is