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

redo neighbor discovery option parsing API #94

Merged
merged 3 commits into from
May 11, 2020
Merged

Conversation

drunkirishcoder
Copy link
Contributor

@drunkirishcoder drunkirishcoder commented May 7, 2020

Description

The new API offers better memory safety around the NDP options. The options cannot be mutated separate from the main NDP packet. Another fix included here is the ability to immutably read through the options. This is useful when used in combination with Packet::peek.

Fixes #83

Type of change

  • Bug fix
  • Breaking change
  • Documentation update

Checklist

  • Associated tests
  • Associated documentation
  • Impl of Redirect Message (w/ Redirect Option)

@drunkirishcoder drunkirishcoder force-pushed the djin/ndp-redux branch 4 times, most recently from d832bab to 7f7da41 Compare May 8, 2020 14:04
@drunkirishcoder drunkirishcoder force-pushed the djin/ndp-redux branch 2 times, most recently from 2a46238 to fffb6f0 Compare May 8, 2020 14:35
@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #94 into master will increase coverage by 0.55%.
The diff coverage is 83.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   66.50%   67.05%   +0.55%     
==========================================
  Files          60       62       +2     
  Lines        4621     5260     +639     
==========================================
+ Hits         3073     3527     +454     
- Misses       1548     1733     +185     
Impacted Files Coverage Δ
core/src/packets/icmp/v4/echo_reply.rs 72.15% <0.00%> (-5.94%) ⬇️
core/src/packets/icmp/v4/echo_request.rs 81.01% <0.00%> (-6.66%) ⬇️
core/src/packets/icmp/v4/mod.rs 82.30% <ø> (-4.62%) ⬇️
core/src/packets/icmp/v6/echo_reply.rs 72.15% <0.00%> (-5.94%) ⬇️
core/src/packets/icmp/v6/echo_request.rs 72.15% <0.00%> (-5.94%) ⬇️
core/src/packets/icmp/v6/mod.rs 80.15% <ø> (-2.63%) ⬇️
core/src/packets/icmp/v6/ndp/router_advert.rs 83.33% <0.00%> (-8.63%) ⬇️
core/src/packets/icmp/v6/ndp/router_solicit.rs 54.76% <0.00%> (-4.22%) ⬇️
core/src/packets/icmp/v6/time_exceeded.rs 68.65% <60.00%> (-7.62%) ⬇️
core/src/packets/icmp/v6/too_big.rs 70.27% <60.00%> (-8.20%) ⬇️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d17746c...bcb70ef. Read the comment docs.

Copy link
Member

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

First set of pretty minor comments. Will continue reviewing the finished redirect and ndp/options/mod once more.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@@ -80,14 +80,14 @@ impl<E: Ipv6Packet> PacketTooBig<E> {
if let Ok(data) = self.icmp().mbuf().read_data_slice(offset, len) {
unsafe { &*data.as_ptr() }
} else {
unreachable!()
&[]
Copy link
Member

Choose a reason for hiding this comment

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

👍 the move here to an empty array of bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah unreachable!() is quite dangerous. if it's hit for whatever reason, it's a runtime panic. did not realize that.

Copy link
Member

Choose a reason for hiding this comment

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

oh, i could have told you that haha, but i thought it was unreachable hah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, just to be extra safe. 😃

core/src/packets/icmp/v6/ndp/redirect.rs Show resolved Hide resolved
/// that describes how to run IP over the
/// particular link type). *MAY* be sent on
/// other links.
/// - *MTU*: SHOULD be sent on links that have a variable MTU
Copy link
Member

Choose a reason for hiding this comment

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

seems like all possible options, in this PR, start their comment on the next line, but this one doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's all spacing. if the label is too long rather than widen the line with more tabs, they just start the description on the new line to keep the width more compact.

Copy link
Member

Choose a reason for hiding this comment

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

I get that, but all the other options start on the next line, but not this one. Just popped out in review is all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, 👍 I maintained the line breaks and spacing from the rfc.

core/src/packets/icmp/v6/ndp/router_advert.rs Outdated Show resolved Hide resolved
///
/// [`NdpOptions::iter`]: NdpOptions::iter
#[inline]
fn options(&self) -> ImmutableNdpOptionsIterator<'_> {
Copy link
Member

Choose a reason for hiding this comment

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

@drunkirishcoder didn't we mention the move to options_iter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep and I forgot.

Copy link
Member

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

super minor stuff w/ some questions attached that are more general comments.

core/Cargo.toml Outdated
@@ -47,6 +46,7 @@ proptest = { version = "0.9", default-features = false, features = ["default-cod

[features]
default = ["metrics"]
compile_failure = []
Copy link
Member

Choose a reason for hiding this comment

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

minor: maybe an internal comment about this feature?

let _ = self.mbuf.truncate(trim_to);
}

self.fields_mut().length = (len / 8) as u8;
Copy link
Member

Choose a reason for hiding this comment

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

double-checking: Rust's integer divide always floors the value. Guessing we're banking on that here instead of using the adjusted len after the trim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes integer division floors. fairly sure on that, it's definitely not a float.

Copy link
Member

Choose a reason for hiding this comment

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

It def floors.

assert_eq!(&data, header.data());

// this will truncate the extra data byte
header.set_length();
Copy link
Member

Choose a reason for hiding this comment

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

should we notate/comment that creating a packet doesn't explicitly update the length field, and that you have to call the set_length function?

feel like we made this explicit in similar cases (srh?)? I could be wrong. I do know you mention it more generally here: https://github.com/capsule-rs/capsule/pull/94/files#diff-f5850ec0e4de1b426c07d3c990bf8b34R488.

}

#[inline]
fn reconcile(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

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

other reconciles, (during our doctopia) received nice comments about what reconcile fn captures for that packet type. Can we do that here?

/// field in the Neighbor Solicitation message that
/// prompted this advertisement. For an unsolicited
/// advertisement, the address whose link-layer address
/// has changed. The Target Address MUST NOT be a
Copy link
Member

Choose a reason for hiding this comment

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

minor: Goes for everything here, though I'm ok w/ reformatting away from italics (and bolds in some cases by accident?) for these cap MUSTS, SHOULDS, etc..., shouldn't we make this consistent across the other packet comments? I'm guessing it'll be inconsistent now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah they are just result of copy-pasta. we will get around to fix the rest of the doc eventually. I just wanted to trim them down some while I'm touching all the ndp stuff. it's somewhat arbitrary for now what's trimmed. didn't really touch the formatting much other than cutting down the sentences copied over. keeping enough to give you a rough idea what the fields represent but not too tmi.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Just formatting consistency is all.


/// Prepends a new option `T` at the beginning of the options.
#[inline]
pub fn prepend<'a, T: NdpOption<'a>>(&'a mut self) -> Fallible<T> {
Copy link
Member

Choose a reason for hiding this comment

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

i know we have tests below, but may example these?


/// Appends a new option `T` at the end of the options.
#[inline]
pub fn append<'a, T: NdpOption<'a>>(&'a mut self) -> Fallible<T> {
Copy link
Member

Choose a reason for hiding this comment

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

👆

/// In other words, remove all options `o` such that f(o) returns false.
/// If an error occurs, all removals done prior to the error cannot be
/// undone.
pub fn retain<F>(&mut self, mut f: F) -> Fallible<()>
Copy link
Member

Choose a reason for hiding this comment

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

I'd say, def example doc retain at least (if you don't want to for prepend/append).

}
}

/// A trait all NDP options must implement.
Copy link
Member

Choose a reason for hiding this comment

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

A trait that all

NdpOptionTypes::PrefixInformation => prefix = true,
NdpOptionTypes::Mtu => mtu = true,
NdpOptionTypes::SourceLinkLayerAddress => source = true,
_ => other = true,
Copy link
Member

Choose a reason for hiding this comment

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

minor: do we notate anywhere, interior comment, that this other is an option we don't currently handle? Maybe just for posterity?

@zeeshanlakhani zeeshanlakhani self-requested a review May 11, 2020 14:29
Copy link
Member

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

think we decided to punt on anything involving length in 8 octets, right? With that, great work.

@drunkirishcoder
Copy link
Contributor Author

right punting on * 8.

@drunkirishcoder drunkirishcoder merged commit 6b0ab55 into master May 11, 2020
@drunkirishcoder drunkirishcoder deleted the djin/ndp-redux branch May 11, 2020 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Provide stronger memory safety guarantee around NDP options
2 participants