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

Jump to bottom

Fix additional integer overflow issues #139

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Fix additional integer overflow issues #139

merged 3 commits into from
Jan 29, 2025

Conversation

lkeegan
Copy link
Contributor

@lkeegan lkeegan commented Jan 29, 2025

  • correct the condition for using 32bit int indices
    • previous condition: n_points < UINT_MAX
    • however the data array in the C code has size n_points * n_dim and is indexed using a 32bit int type (see e.g. the PA macro)
    • this means results were incorrect for the case where n_points < UINT_MAX and n_points * n_dim >= UINT_MAX
    • new condition: n_points * n_dim < UINT_MAX
    • resolves Another integer overflow issue #138
  • render mako template
    • the generated _kdtree_core.c did not contain latest changes from _kdtree_core.c.mako (sorry my mistake)
    • this meant results were incorrect for the case n_points < UINT_MAX and n_query * k >= UINT_MAX
    • resolves Mako template needs to be re-rendered #137
  • use signed 64bit ints for the OpenMP loop on all platforms
    • to support OpenMP versions < 3 which only allow signed integers as loop counters

@lkeegan
Fix additional integer overflow issues
587e2f3 
- correct the condition for using 32bit int indices
  - previous condition: `n_points < UINT_MAX`
  - however the data array in the C code has size `n_points * n_dim` and is indexed using a 32bit int type (see e.g. the PA macro)
  - this means results were incorrect for the case where `n_points < UINT_MAX` and `n_points * n_dim >= UINT_MAX`
  - new condition: `n_points * n_dim < UINT_MAX`
  - resolves storpipfugl#138
- render mako template
  - the generated _kdtree_core.c did not contain latest changes from _kdtree_core.c.mako (sorry my mistake)
  - this meant results were incorrect for the case `n_points < UINT_MAX` and `n_query * k >= UINT_MAX`
  - resolves storpipfugl#137
- use signed 64bit ints for the OpenMP loop on all platforms
  - to support OpenMP versions < 3 which only allow signed integers as loop counters
djhoese
djhoese reviewed Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these things. I really should have checked the rendered .c file. One question for you inline, otherwise this looks good enough to me to get a bug fix release out.

pykdtree/kdtree.pyx Outdated
@@ -162,7 +162,7 @@ cdef class KDTree:

# Get tree info
self.n = <uint64_t>data_pts.shape[0]
self._use_int32_t = self.n < UINT32_MAX
self._use_int32_t = self.n * <uint64_t>data_pts.shape[1] < UINT32_MAX
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the cast to uint64 needed? I suppose it is a Python int otherwise?

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 think it's needed, I might be getting a bit paranoid about integer overflows!

@djhoese
Copy link
Collaborator

djhoese commented Jan 29, 2025

Are there more tests that should be added to verify some of this functionality?

@djhoese djhoese added the bug label Jan 29, 2025
@lkeegan
Update tests
3ee7f71 
- `n_points < UINT32_MAX` and `n_query * k > UINT32_MAX`
- `n_points < UINT32_MAX` and `n_points * n_dim > UINT32_MAX`
- skip by default as they require ~50GB RAM to run
@lkeegan
Copy link
Contributor Author

lkeegan commented Jan 29, 2025

Are there more tests that should be added to verify some of this functionality?

Good point, I've updated the tests

djhoese
djhoese approved these changes Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

From what I understand and from what you've said this looks good to me. I assume the tests run and pass on a system with enough memory?

@lkeegan
Copy link
Contributor Author

lkeegan commented Jan 29, 2025

From what I understand and from what you've said this looks good to me. I assume the tests run and pass on a system with enough memory?

Yes they ran & passed on a linux machine with plenty of RAM

@djhoese djhoese merged commit 8a48f3e into storpipfugl:master Jan 29, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djhoese djhoese djhoese approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Another integer overflow issue Mako template needs to be re-rendered
2 participants
@lkeegan @djhoese