limit cursor lookup to values_count in Target bound helpers#16420
limit cursor lookup to values_count in Target bound helpers#16420isl-Ramzi wants to merge 1 commit intofirebase:mainfirebase/firebase-ios-sdk:mainfrom isl-Ramzi:target-cursor-boundsisl-Ramzi/firebase-ios-sdk:target-cursor-boundsCopy head branch name to clipboard
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates Target::GetAscendingBound and Target::GetDescendingBound to safely handle query cursors that have fewer components than the target has order-bys, preventing out-of-bounds access. Corresponding unit tests have been added to verify this behavior. The review feedback identifies a potential lifetime issue where a reference is bound to a dereferenced temporary SharedMessage returned by value, and suggests storing the shared pointer wrapper by value instead to prevent undefined behavior.
| const auto& position = *bound.value().position(); | ||
| // A cursor may have fewer components than the target has order-bys; only | ||
| // the leading order-bys have a corresponding cursor value. | ||
| size_t count = std::min<size_t>(order_bys_.size(), position.values_count); | ||
| for (size_t i = 0; i < count; ++i) { | ||
| const auto& order_by = order_bys_[i]; | ||
| if (order_by.field() == segment.field_path()) { | ||
| auto cursor_value = bound.value().position()->values[i]; | ||
| auto cursor_value = position.values[i]; |
There was a problem hiding this comment.
The method bound.value().position() returns a temporary SharedMessage object by value. Binding a reference to the dereferenced temporary (const auto& position = *bound.value().position();) is fragile because the temporary SharedMessage is destroyed at the end of the full expression. While the underlying data remains allocated because bound keeps it alive, this dangling reference pattern is unsafe and can lead to undefined behavior if the context changes. Storing the SharedMessage by value (which is cheap as it is a shared pointer wrapper) is safer and cleaner.
| const auto& position = *bound.value().position(); | |
| // A cursor may have fewer components than the target has order-bys; only | |
| // the leading order-bys have a corresponding cursor value. | |
| size_t count = std::min<size_t>(order_bys_.size(), position.values_count); | |
| for (size_t i = 0; i < count; ++i) { | |
| const auto& order_by = order_bys_[i]; | |
| if (order_by.field() == segment.field_path()) { | |
| auto cursor_value = bound.value().position()->values[i]; | |
| auto cursor_value = position.values[i]; | |
| auto position = bound.value().position(); | |
| // A cursor may have fewer components than the target has order-bys; only | |
| // the leading order-bys have a corresponding cursor value. | |
| size_t count = std::min<size_t>(order_bys_.size(), position->values_count); | |
| for (size_t i = 0; i < count; ++i) { | |
| const auto& order_by = order_bys_[i]; | |
| if (order_by.field() == segment.field_path()) { | |
| auto cursor_value = position->values[i]; |
| const auto& position = *bound.value().position(); | ||
| // A cursor may have fewer components than the target has order-bys; only | ||
| // the leading order-bys have a corresponding cursor value. | ||
| size_t count = std::min<size_t>(order_bys_.size(), position.values_count); | ||
| for (size_t i = 0; i < count; ++i) { | ||
| const auto& order_by = order_bys_[i]; | ||
| if (order_by.field() == segment.field_path()) { | ||
| auto cursor_value = bound.value().position()->values[i]; | ||
| auto cursor_value = position.values[i]; |
There was a problem hiding this comment.
The method bound.value().position() returns a temporary SharedMessage object by value. Binding a reference to the dereferenced temporary (const auto& position = *bound.value().position();) is fragile because the temporary SharedMessage is destroyed at the end of the full expression. While the underlying data remains allocated because bound keeps it alive, this dangling reference pattern is unsafe and can lead to undefined behavior if the context changes. Storing the SharedMessage by value (which is cheap as it is a shared pointer wrapper) is safer and cleaner.
| const auto& position = *bound.value().position(); | |
| // A cursor may have fewer components than the target has order-bys; only | |
| // the leading order-bys have a corresponding cursor value. | |
| size_t count = std::min<size_t>(order_bys_.size(), position.values_count); | |
| for (size_t i = 0; i < count; ++i) { | |
| const auto& order_by = order_bys_[i]; | |
| if (order_by.field() == segment.field_path()) { | |
| auto cursor_value = bound.value().position()->values[i]; | |
| auto cursor_value = position.values[i]; | |
| auto position = bound.value().position(); | |
| // A cursor may have fewer components than the target has order-bys; only | |
| // the leading order-bys have a corresponding cursor value. | |
| size_t count = std::min<size_t>(order_bys_.size(), position->values_count); | |
| for (size_t i = 0; i < count; ++i) { | |
| const auto& order_by = order_bys_[i]; | |
| if (order_by.field() == segment.field_path()) { | |
| auto cursor_value = position->values[i]; |
GetAscendingBound and GetDescendingBound walk
order_bys_but index the cursor'sposition()->valuesarray with the same counter, and a cursor is allowed to carry fewer components than the query has order-bys, soquery.orderBy("a").orderBy("b").startAt(["a1"])served by an index on (a asc, b asc) reads onegoogle_firestore_v1_Valuepast the end of that array. The garbage struct goes straight into LowerBoundCompare/UpperBoundCompare, which dereferences whichever union member its uninitializedwhich_value_typeselects; in a debug build it trips theInvalid type value: 0assertion in GetTypeOrder, and otherwise the value can end up in the returned bound. Everywhere else that reads a cursor iteratesvalues_countinstead (Bound::CompareToDocument even assertsvalues_count <= order_by.size()), so this clamps the two loops the same way and adds regression tests for both bound directions.