Skip to content

Commit

Permalink
Fix integer overflow in QP reduce_options_if_needed (#5406)
Browse files Browse the repository at this point in the history
  • Loading branch information
SimonSapin authored Jun 11, 2024
1 parent a0b9d34 commit acb1bdb
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 9 deletions.
37 changes: 28 additions & 9 deletions apollo-federation/src/query_plan/query_planning_traversal.rs
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
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
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);
}
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
}

0 comments on commit acb1bdb

Please sign in to comment.