NLnetLabs / domain

A DNS library for Rust.
https://nlnetlabs.nl/projects/domain/about/
BSD 3-Clause "New" or "Revised" License
332 stars 56 forks source link

`HeaderSection`: Change slices to fixed-length arrays #329

Open tertsdiepraam opened 1 month ago

tertsdiepraam commented 1 month ago

HeaderSection, Header and HeaderCounts all have for_message_slice which takes a slice and panics if the slice is too short. This is annoying because the compiler cannot help us enforce that constraint at compile-time and users of domain would need to read the docs carefully to use those functions correctly.

This PR changes those methods to instead take a &[u8; N] with the length of the internal representation of these types. The advantage of this can be seen by the tests this PR removes: instead of panicking they now no longer compile.

Additionally, I changed the as_slice methods for these types to as_array. Since a reference to an array derefs to a slice, these are mostly compatible.

There's another notable change: HeaderCounts does not take a slice of the entire message anymore, but only of the counts part (so just 8 bytes). This makes that function simpler, which I think is better because that makes it easier to guarantee its correctness. The burden of finding the right offset to get it from is now on the caller. This means that users probably do not want to use HeaderCounts::for_message_chunk but instead HeaderSection::for_message_chunk followed by .counts(). This makes it clearer what information should be passed in. There's even a case to be made that {Header, HeaderCounts}::for_message_chunk should not be public.

We can now do the conversions with mem::transmute, but there's a pattern that makes the transmutes even safer. mem::transmute statically guarantees that the types it converts have the same size. That's nice, but not enough in our case since we want to guarantee that the size of the referenced types are the same. Therefore, I add a second unused mem::transmute call like this:

let _ = || {
    unsafe { mem::transmute::<[u8; N], Self>(*s) };
};

This does not compile if [u8; N] and Self have different sizes, therefore guaranteeing safety. It's a bit strange, but I think it works.

This is a draft PR because:

  1. It's a breaking change and we might not want this or deprecate some of the functions I changed or removed first.
  2. We might want to remove the non-mut conversions and returned owned (copied) values instead.
  3. ~This currently requires Rust 1.77, so if we downgrade to 1.76, I'd have to change some things before we can merge this.~

This is a breaking change.