Skip to content

Fix memory access violation in partition_with_ram_budget #654

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ninjaasmoke
Copy link

  • Does this PR have a descriptive title that could go in our release notes?
  • Does this PR add any new dependencies?
  • Does this PR modify any existing APIs?
    • Is the change to the API backwards compatible?
  • Should this result in any changes to our documentation, either updating existing docs or adding new ones?

Reference Issues/PRs

What does this implement/fix? Briefly explain your changes.

Fixes a potential crash in partition_with_ram_budget function when k_base parameter exceeds the number of partitions created.

Issue:
When the partitioning algorithm creates fewer partitions than the requested k_base value, the code attempts to access cluster centers that don't exist, leading to memory access violations and crashes.

Example error (intermittent):

Processing global k-means (kmeans_partitioning Step)
Residuals unchanged: 175.863 becomes 175.863. Early termination.
ERROR: k (5) > num_center(3)
unknown location(0): fatal error: memory access violation at address: 0x20ebdbf40: no mapping at fault address

This occurs when k_base=5 is passed but the k-means algorithm converges to only 3 clusters, causing out-of-bounds access in the cluster size estimation step. The issue is intermittent due to the non-deterministic nature of k-means clustering.

Solution:
Added a bounds check to ensure k_base never exceeds the actual number of partitions:

k_base = std::min(k_base, static_cast<size_t>(num_parts));

This prevents out-of-bounds access during cluster size estimation while preserving the intended functionality.

Any other comments?

This is a minimal, safe fix that maintains backward compatibility. The change ensures robust behavior when the partitioning algorithm determines that fewer partitions are needed than initially requested via k_base.

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.

1 participant