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

Use non-zero page size #258

Merged

Conversation

JonathanWoollett-Light
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light commented Oct 31, 2023

This avoids potential divide by zero error.

Fixes #268

Summary of the PR

Please summarize here why the changes in this PR are needed.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

Copy link
Contributor

@Ablu Ablu left a comment

Choose a reason for hiding this comment

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

This is breaking API and should have a CHANGELOG entry. While I see no problem with the approach, I wonder whether this could be reasoned a bit better in the commit message.

My immediate questions are: Where did this cause major problems? Why is making the user go through the NonZeroUsize conversion worth it?

Is this the only place where we deal with page_sizes? 🤔 Should we convert them all?

src/bitmap/backend/atomic_bitmap.rs Show resolved Hide resolved
@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Nov 1, 2023

This is breaking API and should have a CHANGELOG entry. While I see no problem with the approach, I wonder whether this could be reasoned a bit better in the commit message.

I've added more details to the commit message.

My immediate questions are: Where did this cause major problems? Why is making the user go through the NonZeroUsize conversion worth it?

Without this it is possible to trigger a divide by zero error, see https://github.com/rust-vmm/vm-memory/blob/main/src/bitmap/backend/atomic_bitmap.rs#L28

Is this the only place where we deal with page_sizes? 🤔 Should we convert them all?

Are there other places you see potential errors that you think should be fixed?

@Ablu
Copy link
Contributor

Ablu commented Nov 1, 2023

My immediate questions are: Where did this cause major problems? Why is making the user go through the NonZeroUsize conversion worth it?

Without this it is possible to trigger a divide by zero error, see https://github.com/rust-vmm/vm-memory/blob/main/src/bitmap/backend/atomic_bitmap.rs#L28

Sure! I was just wondering where that became a problem in "practise" :).

@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Nov 1, 2023

Sure! I was just wondering where that became a problem in "practise" :).

I understand your point, in practice I cannot see this occurring outside very exotic circumstances, but it is possible (as its a public function) so it should be mitigated.

src/bitmap/backend/atomic_bitmap.rs Outdated Show resolved Hide resolved
src/bitmap/backend/atomic_bitmap.rs Show resolved Hide resolved
This avoids potential divide by zero error when calling
`AtomicBitmap::new`.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
@JonathanWoollett-Light JonathanWoollett-Light merged commit 2d5afa0 into rust-vmm:main Nov 23, 2023
2 checks passed
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.

Missing Divide-by-Zero Check
5 participants