Use non-zero page size - Githubissues

rust-vmm / vm-memory

Virtual machine's guest memory crate

Apache License 2.0

299 stars 97 forks source link

Use non-zero page size #258

Closed JonathanWoollett-Light closed 9 months ago

JonathanWoollett-Light commented 9 months ago

This avoids potential divide by zero error.

Fixes https://github.com/rust-vmm/vm-memory/issues/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:

[x] 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.
[x] All added/changed functionality has a corresponding unit/integration test.
[x] All added/changed public-facing functionality has entries in the "Upcoming Release" section of CHANGELOG.md (if no such section exists, please create one).
[x] Any newly added unsafe code is properly documented.

JonathanWoollett-Light commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.