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

[v0.22.1] Fix serial memory allocation issue. #2179

Merged
merged 4 commits into from Oct 14, 2020

Conversation

sandreim
Copy link
Contributor

Reason for This PR

Fixes #2177

Description of Changes

Provided in commit messages.

  • This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any API changes are reflected in firecracker/swagger.yaml.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Iulian Barbu and others added 3 commits October 12, 2020 12:40
Signed-off-by: Iulian Barbu <iul@amazon.com>
Signed-off-by: Iulian Barbu <iul@amazon.com>
Suggested-by: Andreea Florescu <fandree@amazon.com>
The security db (https://github.com/RustSec/cargo-audit)
has broken compatiblity with cargo audit v10. A fixed cargo
audit version v12 is available, but  it requires an upgrade of
the rust toolchain on top of the current release. This commit
is a temporary fix until we unshare toolchain dependency
of Firecracker and cargo audit binary.

Signed-off-by: Andrei Sandu <sandreim@amazon.com>
@sandreim sandreim added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Oct 14, 2020
@sandreim sandreim self-assigned this Oct 14, 2020
Signed-off-by: Andrei Sandu <sandreim@amazon.com>
fn avail_buffer_capacity(&self) -> usize {
FIFO_SIZE.checked_sub(self.in_buffer.len()).unwrap_or_else(||
panic!(
"Errored out due to serial device buffer size greater than the maximum expected size: {} > {}.",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "errored" is not a real word.

Suggested change
"Errored out due to serial device buffer size greater than the maximum expected size: {} > {}.",
"Error: Serial device buffer size greater than the maximum expected size: {} > {}.",

Ok(0)
}
});
let mut out = vec![0u8; avail_cap];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a member of the struct as the constant allocation and zeroing of the memory is costly, but this is an optimization which we can implement later and is not related to the security issue.

e
),
// Handle EOF if the event came from the input source.
if input_fd == event.fd() && count == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we handle the case where input_fd != event.fd()?

after_size = get_total_mem_size(microvm.jailer_clone_pid)
assert before_size == after_size, "The memory size of the " \
Copy link
Contributor

Choose a reason for hiding this comment

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

this is prone to false positives: process used memory can slightly change due to unrelated reasons

we can address this incrementally since worst that can happen are false positives - not blocking this PR because of it

Copy link
Contributor

@georgepisaltu georgepisaltu left a comment

Choose a reason for hiding this comment

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

A few nits, can merge and come back to them later.

@sandreim sandreim merged commit 1c25f5c into firecracker-microvm:fix-v0.22.1 Oct 14, 2020
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants