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.

Sign up for GitHub

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

Fix integer overflow in QP reduce_options_if_needed #5406

Merged
merged 2 commits into from
Jun 11, 2024
Merged

Fix integer overflow in QP reduce_options_if_needed #5406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f5b870
Add a test reproducing panic in multiplication overflow
SimonSapin Jun 11, 2024
ad6f022
Fix integer overflow in QP reduce_options_if_needed
SimonSapin Jun 11, 2024
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
37 changes: 28 additions & 9 deletions apollo-federation/src/query_plan/query_planning_traversal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,10 +744,11 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> {
// We sort branches by those that have the most options first.
self.closed_branches
.sort_by(|b1, b2| b1.0.len().cmp(&b2.0.len()).reverse());
let mut plan_count = self
.closed_branches
.iter()
.try_fold(1, |product, branch| {

/// Returns usize::MAX for integer overflow
fn product_of_closed_branches_len(closed_branches: &[ClosedBranch]) -> usize {
let mut product: usize = 1;
for branch in closed_branches {
if branch.0.is_empty() {
// This would correspond to not being to find *any* path
// for a particular queried field,
Expand All @@ -758,12 +759,18 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> {
// is exactly to ensure we can never run into this path.
// In any case, we will throw later if that happens,
// but let's just return the proper result here, which is no plan at all.
None
return 0;
} else {
Some(product * branch.0.len())
let Some(new_product) = product.checked_mul(branch.0.len()) else {
return usize::MAX;
};
product = new_product
}
})
.unwrap_or(0);
}
product
}

let mut plan_count = product_of_closed_branches_len(&self.closed_branches);
// debug!("Query has {plan_count} possible plans");

let max_evaluated_plans =
Expand All @@ -776,7 +783,19 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> {
break;
}
Self::prune_and_reorder_first_branch(&mut self.closed_branches);
plan_count -= plan_count / first_branch_len;
if plan_count != usize::MAX {
// We had `old_plan_count == first_branch_len * rest` and
// reduced `first_branch_len` by 1, so the new count is:
//
// (first_branch_len - 1) * rest
// = first_branch_len * rest - rest
// = (first_branch_len * rest) - (first_branch_len * rest) / first_branch_len
// = old_plan_count - old_plan_count / first_branch_len
plan_count -= plan_count / first_branch_len;
} else {
// Previous count had overflowed, so recompute the reduced one from scratch
plan_count = product_of_closed_branches_len(&self.closed_branches)
}

// debug!("Reduced plans to consider to {plan_count} plans");
}
Expand Down
103 changes: 103 additions & 0 deletions ...ration/tests/query_plan/build_query_plan_tests/debug_max_evaluated_plans_configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,3 +540,106 @@ fn avoid_considering_indirect_paths_from_the_root_when_a_more_direct_one_exists(
// key) is never exactly a good idea.
assert_eq!(plan.statistics.evaluated_plan_count.get(), 4);
}

/// https://apollographql.atlassian.net/browse/FED-301
#[test]
fn multiplication_overflow_in_reduce_options_if_needed() {
let planner = planner!(
// default config
Subgraph1: SUBGRAPH,
Subgraph2: SUBGRAPH,
);
let plan = assert_plan!(
&planner,
r#"
{
t {
f00: id f01: id f02: id f03: id f04: id f05: id f06: id f07: id
f08: id f09: id f10: id f11: id f12: id f13: id f14: id f15: id
f16: id f17: id f18: id f19: id f20: id f21: id f22: id f23: id
f24: id f25: id f26: id f27: id f28: id f29: id f30: id f31: id
f32: id f33: id f34: id f35: id f36: id f37: id f38: id f39: id
f40: id f41: id f42: id f43: id f44: id f45: id f46: id f47: id
f48: id f49: id f50: id f51: id f52: id f53: id f54: id f55: id
f56: id f57: id f58: id f59: id f60: id f61: id f62: id f63: id
}
}
"#,
@r###"
QueryPlan {
Fetch(service: "Subgraph1") {
{
t {
f14: id
f15: id
f16: id
f17: id
f18: id
f19: id
f20: id
f21: id
f22: id
f23: id
f24: id
f25: id
f26: id
f27: id
f28: id
f29: id
f30: id
f31: id
f32: id
f33: id
f34: id
f35: id
f36: id
f37: id
f38: id
f39: id
f40: id
f41: id
f42: id
f43: id
f44: id
f45: id
f46: id
f47: id
f48: id
f49: id
f50: id
f51: id
f52: id
f53: id
f54: id
f55: id
f56: id
f57: id
f58: id
f59: id
f60: id
f61: id
f62: id
f63: id
f00: id
f13: id
f01: id
f02: id
Comment on lines +622 to +626
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know why the original ordering is not preserved. The overall Router is not affected since format_response well reorder data to what the original supergraph client expects, but should we still consider this a bug?

f03: id
f04: id
f05: id
f06: id
f07: id
f08: id
f09: id
f10: id
f11: id
f12: id
}
}
},
}
"###
);
// max_evaluated_plans defaults to 10_000
assert_eq!(plan.statistics.evaluated_plan_count.get(), 8192);
}
60 changes: 60 additions & 0 deletions .../tests/query_plan/supergraphs/multiplication_overflow_in_reduce_options_if_needed.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# Composed from subgraphs with hash: fd2cfde36cc3d0a981e6c3636aaeea3a6aad4424
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION)
{
query: Query
}

directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE

directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

directive @join__graph(name: String!, url: String!) on ENUM_VALUE

directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE

directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR

directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION

directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA

scalar join__FieldSet

enum join__Graph {
SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none")
SUBGRAPH2 @join__graph(name: "Subgraph2", url: "none")
}

scalar link__Import

enum link__Purpose {
"""
`SECURITY` features provide metadata necessary to securely resolve fields.
"""
SECURITY

"""
`EXECUTION` features provide metadata necessary for operation execution.
"""
EXECUTION
}

type Query
@join__type(graph: SUBGRAPH1)
@join__type(graph: SUBGRAPH2)
{
t: T
}

type T
@join__type(graph: SUBGRAPH1, key: "id")
@join__type(graph: SUBGRAPH2, key: "id")
{
id: ID!
v1: Int
v2: Int
v3: Int
v4: Int
}
Loading