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.

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

Binomial Tree Generators Panic for order argument values too large #457

Closed
mtreinish opened this issue Oct 2, 2021 · 0 comments · Fixed by #458
Closed

Binomial Tree Generators Panic for order argument values too large #457

mtreinish opened this issue Oct 2, 2021 · 0 comments · Fixed by #458
Assignees
Labels
bug Something isn't working

Comments

@mtreinish
Copy link
Member

Information

  • retworkx version: 0.10.2
  • Python version: 3.9
  • Rust version: 1.55
  • Operating system: Linux

What is the current behavior?

When running the binomial tree generator functions retworkx.generators.binomial_tree or retworkx.generators.directed_binomial_tree_graph passing in a large order argument which is far too large for the system will result in a panic either from an overflow or allocation error. For example, on my local system:

import retworkx
graph = retworkx.generators.directed_binomial_tree_graph(64)

yields:

thread '<unnamed>' panicked at 'index out of bounds: the len is 0 but the index is 0', src/generators.rs:1055:28
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
pyo3_runtime.PanicException: index out of bounds: the len is 0 but the index is 0

import retworkx
graph = retworkx.generators.directed_binomial_tree_graph(63)

yields

thread '<unnamed>' panicked at 'capacity overflow', library/alloc/src/raw_vec.rs:560:5
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
pyo3_runtime.PanicException: capacity overflow

import retworkx
>>> graph = retworkx.generators.directed_binomial_tree_graph(45)

yields

memory allocation of 140737488355328 bytes failed

What is the expected behavior?

These graphs exceed the size of system memory or the max size of a u64/usize and this should be reported to the user instead. We can detect the available memory or the size of order and determine up front if things will overflow or are too large for system memory and return appropriate user facing exceptions.

Steps to reproduce the problem

import retworkx
graph = retworkx.generators.directed_binomial_tree_graph(63)
graph = retworkx.generators.directed_binomial_tree_graph(64)
graph = retworkx.generators.directed_binomial_tree_graph(45)
@mtreinish mtreinish added the bug Something isn't working label Oct 2, 2021
@mtreinish mtreinish self-assigned this Oct 2, 2021
mtreinish added a commit to mtreinish/retworkx that referenced this issue Oct 2, 2021
This commit fixes issues with the allocation of memory in the binomial
graph generators. Previously there were 2 issues in the binomial graph
generator functions when it came to allocating memory. The first was a
temporary Vec was used to store all the node indices in the graph which
would eat num_nodes * sizeof(usize) bytes of memory. This Vec isn't
actually needed as the NodeIndex is just a usize integer wrapper and we
know the indices of the nodes. This commit removes this and just
iterates over the range of nodes instead of creating a Vec reducing the
amoount of memory required. The second issue was that it was easily
possible to overflow the max Vec size (either inside the graph objects
or from the Vec previously created to store the node indices) by using
an order where 2**order > isize::MAX or if order > 64 causing an
overflow of a 64bit usize which would result in a panic. This fixes
this issue by checking the input of order and seeing if it will overflow
or exceed the max Vec size and raise an OverflowError.

Fixes Qiskit#457
mtreinish added a commit to mtreinish/retworkx that referenced this issue Oct 2, 2021
This commit fixes issues with the allocation of memory in the binomial
graph generators. Previously there were 2 issues in the binomial graph
generator functions when it came to allocating memory. The first was a
temporary Vec was used to store all the node indices in the graph which
would eat num_nodes * sizeof(usize) bytes of memory. This Vec isn't
actually needed as the NodeIndex is just a usize integer wrapper and we
know the indices of the nodes. This commit removes this and just
iterates over the range of nodes instead of creating a Vec reducing the
amoount of memory required. The second issue was that it was easily
possible to overflow the max Vec size (either inside the graph objects
or from the Vec previously created to store the node indices) by using
an order where 2**order > isize::MAX or if order > 64 causing an
overflow of a 64bit usize which would result in a panic. This fixes
this issue by checking the input of order and seeing if it will overflow
or exceed the max Vec size and raise an OverflowError.

Fixes Qiskit#457
mtreinish added a commit that referenced this issue Oct 10, 2021
* Fix memory allocation issues in binomial graph generators

This commit fixes issues with the allocation of memory in the binomial
graph generators. Previously there were 2 issues in the binomial graph
generator functions when it came to allocating memory. The first was a
temporary Vec was used to store all the node indices in the graph which
would eat num_nodes * sizeof(usize) bytes of memory. This Vec isn't
actually needed as the NodeIndex is just a usize integer wrapper and we
know the indices of the nodes. This commit removes this and just
iterates over the range of nodes instead of creating a Vec reducing the
amoount of memory required. The second issue was that it was easily
possible to overflow the max Vec size (either inside the graph objects
or from the Vec previously created to store the node indices) by using
an order where 2**order > isize::MAX or if order > 64 causing an
overflow of a 64bit usize which would result in a panic. This fixes
this issue by checking the input of order and seeing if it will overflow
or exceed the max Vec size and raise an OverflowError.

Fixes #457

* Use a constant value dependent on platform pointer width

* Update src/generators.rs

Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant