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

Atom soundness and usability fixes #100

Merged
merged 53 commits into from
Nov 29, 2021
Merged

Atom soundness and usability fixes #100

merged 53 commits into from
Nov 29, 2021

Conversation

prokopyl
Copy link
Member

While working on the options feature (#98), I realized there were many issues in the Atom APIs
(mainly in the Space APIs), and they needed quite a bit of work. The main issues were lifetime issues, and quite
a few unsoundness issues (due to methods not being marked unsafe, and alignment reading issues).

Turns out it was quite a lot in the end, but I think this makes a lot of the APIs clearer and easier to use as well. :)

  • Mark atom-reading methods as unsafe, as they have to blindly trust the contents of the input buffer to be containing atoms.
  • Renamed Space into AlignedSpace, and made it a Dynamically Sized Type. This fixes a good amount of lifetime issues, allow AlignedSpaces to also be writable. This type's only role is to ensure its contents are aligned for reading/writing a given type T.
  • Removed all of the use_padding parameters in atom writing methods, they are dropped in favor of internally using AlignedSpaces which align themselves automatically depending on the input type.
  • Added a SpaceReader struct, a cursor that contains many ergonomic methods for reading data from Atom buffers, to be used by custom Atom read implementations, also making them much safer.
  • Removed all the odd split_* methods on Space. The SpaceReader is to be used instead.
  • Added VecSpace, a Vec-backed Space buffer, that can grow when writing Atoms into it. It replaces the quirky SpaceList that used a linked list implementation.
  • Renamed FramedMutSpace into AtomSpaceWriter. Its implementation also has been refactored to use indexes instead of pointers,
    to be compatible with re-allocating writers such as VecSpace.
  • Renamed RootMutSpace into SpaceCursor.
  • Renamed the MutSpace trait into SpaceWriter. All the helper methods are now implemented directly onto
    this trait instead of on the obnoxious dyn MutSpace. An object-safe sub-trait SpaceWriterImpl (to be better named
    at some point) is what the allocators such as VecSpace, SpaceCusror and AtomSpaceWriter actually implement, and
    the SpaceWriter is automatically implemented for those via a blanket implementation.
  • Added the Terminated struct, a wrapper around any SpaceAllocator that adds a terminator byte at the very end of any
    allocation, overwriting any previous terminator bytes it may have written. This is very useful to implement things like
    the String of MidiEvent atom writers, where a terminator byte has to be inserted but multiple writes can be made.
    This also makes them not reliant on their Drop destructors running to write the final byte, removing a potential source of UB.
  • Made UnidentifiedAtom a Dynamically Sized Type, already containing a valid header. This allows to remove many
    unneeded extra checks, as the type itself guarantees there's a valid header in there.
  • Added the AtomHeader struct, a thin wrapper over the LV2_Atom header struct that allows to safely manipulate
    headers as a whole. Unlike LV2_Atom, it also has a required 64-bit alignment, as per the LV2 spec.
  • Removed lifetimes from the Atom types (and removed many extra lifetimes accordingly).
  • Removed ReadParameter and WriteParameter associated types. This massively reduces clutter in methods that read atoms,
    and makes intent clearer as to what operation is being done. Atom types that needed those parameters now return an intermediary struct
    using the typestate pattern.
  • Moved all base atom types to a sub-module. This is mainly to de-clutter the generated documentation, making it clearer what the lv2-atom crate is exposing.
  • Removed lifetimes in the Atom type. They are replaced with a GAT-like workaround, where the ReadHandle and
    WriteHandle now return family structs
    implementing a new family trait, AtomHandle<'a>. This approach is a bit more verbose when implementing atom types,
    but allows atom reading methods to be properly generic over lifetimes, both improving usability and removing UB (as
    incorrect lifetime assumptions could be made).
  • Removed Vector type parameter. The type parameter and urid check has been moved into its reader type. This also
    fixes the unneeded type parameters when trying to store a Vector's URID.
  • Moved Sequence unit type reading into a type parameter for its reader. This removes many subsequent checks, as
    well as the need for the TimeStampURID enum, as a sequence's timestamp unit type is known ahead of time now.
  • Added an AtomError error type, as well as sub-error types AtomReadError, AtomWriteError, and AlignmentError.
    Now all atom-manipulating methods return these errors in a Result instead of an Option, helping with semantics and
    making debugging a bit easier.
  • Added a special prelude for the atom-implementing side. This is mainly to de-clutter the main prelude, since the
    majority of users will not be implementing their own Atom types.
  • Added lots of extra documentation, especially in AlignedSpace.

I may be adding some extra documentation, but otherwise I consider this PR to be pretty complete!

@prokopyl prokopyl added 📖 Documentation An effort to improve the documentation 💥 Unsound API A bug that allows to trigger Undefined Behavior in safe Rust 💔 Breaking change Changes to the code that will lead to break one or more APIs 🌟 Ergonomics Little things that matter! Does not add functionality, but makes an API easier to use labels Sep 26, 2021
@prokopyl prokopyl added this to the Version 0.7.0 milestone Sep 26, 2021
@prokopyl prokopyl self-assigned this Sep 26, 2021
@prokopyl prokopyl merged commit 44aac00 into develop Nov 29, 2021
@prokopyl prokopyl deleted the atom-soundness branch November 29, 2021 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 Documentation An effort to improve the documentation 💥 Unsound API A bug that allows to trigger Undefined Behavior in safe Rust 💔 Breaking change Changes to the code that will lead to break one or more APIs 🌟 Ergonomics Little things that matter! Does not add functionality, but makes an API easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant