Skip to content

fix: prevent division-by-zero and integer overflow in partition()#899

Open
hieuit095 wants to merge 1 commit into
sfu-db:mainfrom
hieuit095:bugfix/partition-div-zero
Open

fix: prevent division-by-zero and integer overflow in partition()#899
hieuit095 wants to merge 1 commit into
sfu-db:mainfrom
hieuit095:bugfix/partition-div-zero

Conversation

@hieuit095
Copy link
Copy Markdown

Summary

Fixes a crash bug in partition() (connectorx/src/partition.rs) where invalid inputs cause a panic via:

  1. Division by zero when num == 0 in the calculation (max - min + 1) / num
  2. Integer overflow when extreme min/max values cause overflow in (max - min + 1) or i * partition_size

Both issues can be triggered by a caller passing a malicious or erroneous partition_query with num == 0.

Changes

  • Guard num == 0: Returns a proper error instead of panicking on division by zero
  • Guard inverted range (max < min): Returns a single safe query rather than looping infinitely
  • Use checked_add/sub/mul: Prevents i64 overflow in partition boundary calculations
  • Use saturating_add: Safely handles edge cases where partition_size would be 0

Impact

  • Crash prevention (HIGH): Users calling partition_sql() or read_sql() with partition_query.num = 0 would previously get an unhandled panic
  • Integer overflow safety (MEDIUM): Extreme min/max values could cause silent wrong partition boundaries

Generated by git_pr bounty hunter agent.

- Guard num==0 to throw an error instead of panicking on division by zero
- Guard inverted/empty range (max < min) to return a single query safely
- Use checked_add/sub/mul to prevent i64 overflow in partition boundaries
- Use saturating arithmetic to handle edge cases gracefully

Fixes a crash when partition_query.num is set to 0 or when
min/max values cause overflow in (max - min + 1) / num computation.
let lower = match min.checked_add((i as i64).saturating_mul(partition_size)) {
Some(l) => l,
None => {
// Overflow computing lower bound; clamp to min.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't this result in multiple partitions with overlapping ranges? This may cause the wrong result.

Copy link
Copy Markdown
Contributor

@wangxiaoying wangxiaoying left a comment

Choose a reason for hiding this comment

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

Thanks @hieuit095 for the PR!

I think the partition num check and max-min+1 overflow looks good. But the clamping logic may cause the same range handled by multiple partitions and result in the wrong result. We could just throw an error on those cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants