These functions only skipped the header, but what they should have
done is cut the packet at its total length and skip the header.
Otherwise, if the buffer the IP packet is constructed from contains
padding, such as Ethernet packets, the payload would be too long.
The use of this type has several drawbacks:
* It does not allow distinguishing between different error
conditions. In fact, we wrongly conflated some of them
before this commit.
* It does not allow propagation via ? and requires manual use
of map_err, which is especially tiresome for downstream code.
* It prevents us from expanding the set of error conditions
even if right now we have only one.
* It prevents us from blanket using Result<T> everywhere
(a nitpick at most).
Instead, use Result<T, Error> everywhere, and differentiate error
conditions where applicable.
This is a form of an uninitialized read bug; although safe it caused
panics. In short, transmit buffers received from the network stack
should be considered uninitialized (in practice they will often
contain previously transmitted packets or parts thereof). Wrapping
them with the only method we had (e.g. Ipv4Packet) treated the buffer
as if it contained a valid incoming packet, which can easily fail
with Error::Truncated.
This commit splits every `fn new(buffer: T) -> Result<Self, Error>`
method on a `Packet` into three smaller ones:
* `fn check_len(&self) -> Result<(), Error>`, purely a validator;
* `fn new(T) -> Self`, purely a wrapper;
* `fn new_checked(T) -> Result<Self, Error>`, a validating wrapper.
This makes it easy to process ingress packets (using `new_checked`),
egress packets (using `new`), and, if needed, maintain the invariants
at any point during packet construction (using `check_len`).
Fixes#17.
Before this commit, IP payload length was calculated by subtracting
the IP header length from the total underlying buffer length, which
fails if the underlying buffer has padding, e.g. like Ethernet
does.
whitequark
Steffen Butzer
Egor Karavaev