limit cursor lookup to values_count in Target bound helpers

[isl-Ramzi]

GetAscendingBound and GetDescendingBound walk order_bys_ but index the cursor's position()->values array with the same counter, and a cursor is allowed to carry fewer components than the query has order-bys, so query.orderBy("a").orderBy("b").startAt(["a1"]) served by an index on (a asc, b asc) reads one google_firestore_v1_Value past the end of that array. The garbage struct goes straight into LowerBoundCompare/UpperBoundCompare, which dereferences whichever union member its uninitialized which_value_type selects; in a debug build it trips the Invalid type value: 0 assertion in GetTypeOrder, and otherwise the value can end up in the returned bound. Everywhere else that reads a cursor iterates values_count instead (Bound::CompareToDocument even asserts values_count <= order_by.size()), so this clamps the two loops the same way and adds regression tests for both bound directions.

[gemini-code-assist]

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

[ncooke3]

/gemini review

[gemini-code-assist]

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.

[gemini-code-assist]

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.

Suggested change

	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];

[isl-Ramzi]

Good point, that reference was bound to a temporary. Switched both sites to hold the SharedMessage by value and index through it, so the pointee stays alive for the whole loop.

[gemini-code-assist]

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.

Suggested change

	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];

[isl-Ramzi]

Same fix here, holding the position by value instead of binding a reference to the dereferenced temporary.

[paulb777]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates Target::GetAscendingBound and Target::GetDescendingBound to handle cursors with fewer components than the target's order-bys by limiting the comparison loop to the minimum of the two counts, and adds corresponding unit tests. The review feedback suggests using const auto& instead of auto when retrieving the bound's position to avoid unnecessary copying of the SharedMessage and eliminate atomic reference count overhead.

[gemini-code-assist]

Since bound.value().position() returns a SharedMessage by value, using const auto& instead of auto will bind the temporary to a reference and extend its lifetime to the end of the block. This avoids the overhead of copying the shared pointer (which involves atomic reference count increments and decrements).

Suggested change

	auto position = bound.value().position();
	const auto& position = bound.value().position();

[gemini-code-assist]

Similarly, using const auto& here avoids copying the SharedMessage returned by value, eliminating unnecessary atomic reference count operations.

Suggested change

	auto position = bound.value().position();
	const auto& position = bound.value().position();