Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tidb_query_vec_executors: Fixed stack-borrowing undefined-behavior #7709

Merged
merged 3 commits into from
Apr 30, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
tidb_query_vec_executors: Fixed stack-borrowing undefined-behavior
I don't understand the stacked borrowing rules enough to understand why the
existing code is a violation. It is creating a mutable unsafe pointer; then
taking some immutable borrows to the same data while doing nothing with that
pointer; then later using the pointer.

Simply moving the creation of the unsafe pointer to immediately before it
is used removes the error.

Found with miri, which reports:

```
error: Undefined Behavior: trying to reborrow for SharedReadOnly, but parent tag <1121039> does not have an appropriate item in the borrow sta
ck
   --> components/tidb_query_vec_executors/src/slow_hash_aggr_executor.rs:281:32
    |
281 |             let offset_begin = self.group_key_buffer.len();
    |                                ^^^^^^^^^^^^^^^^^^^^^ trying to reborrow for SharedReadOnly, but parent tag <1121039> does not have an ap
propriate item in the borrow stack
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
```

Signed-off-by: Brian Anderson <[email protected]>
  • Loading branch information
brson committed Apr 30, 2020
commit ff6b8bfa9ebe55d0eaaae7e05b55ffe2f3f5b5e1
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,6 @@ impl<Src: BatchExecutor> AggregationExecutorImpl<Src> for SlowHashAggregationImp
)?;
}

let buffer_ptr = (&*self.group_key_buffer).into();
for logical_row_idx in 0..logical_rows_len {
let offset_begin = self.group_key_buffer.len();

Expand Down Expand Up @@ -361,6 +360,7 @@ impl<Src: BatchExecutor> AggregationExecutorImpl<Src> for SlowHashAggregationImp
}
}

let buffer_ptr = (&*self.group_key_buffer).into();
// Extra column is not included in `GroupKeyRefUnsafe` to avoid being aggr on.
let group_key_ref_unsafe = GroupKeyRefUnsafe {
buffer_ptr,
Expand Down