Currently there are at least two (1, 2) independent implementations of fallible allocation in rust code. If nothing else, we should unify the duplication. Ideally, we should make all our rust code use a consistent approach to allocation fallibility.

Additionally, neither implementation is a complete solution to the problem of allocations whose size is derived from untrusted input potentially causing crashes. The current approaches rely on the consumer remembering to call the special fallible functions rather than the normal stdlib ones. Additionally, there are many other facilities within stdlib which allocate but have no fallible alternative.

Unlike C++, the ergonomics of rust allow for making potentially allocating functions fallible by the unobtrusive addition of a Result type to the return value. This is conveniently checked by the compiler and must be used, so there is little chance of the failure being silently discarded. Furthermore, if infallible allocation (i.e., panicking on failure) is desired in a particular piece of code, it can be achieved through the addition of unwrap/expect, which makes the intentions clear.

The details are free to change, but my initial suggestion for a solution would be to create a new crate (or perhaps contribute to the existing fallible_collections) which provides wrapper types around all the stdlib allocating types that ensures all operations which potentially allocate communicate their fallibility in their API. See this PR on mp4parse-rust as a proof of concept.

One additional benefit of creating new wrapper types is that it provides complete control over the allocation/reallocation/deallocation cycle, rather than changing just the allocation/reallocation behind std::vec::Vec's back. The current approach requires us to ensure the allocator we use matches the one the stdlib does; a requirement that goes away when we handle all allocations and deallocations in a value's lifetime.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Sounds good overall!

complete control over the allocation/reallocation/deallocation cycle, rather than changing just the allocation/reallocation behind std::vec::Vec's back

I don’t understand this part, could you explain some more?

Note that Box explicitly documents that it is valid to use Box::from_raw with a pointer you allocated yourself, as long as it was with the same allocator as Box::new would use. We haven’t yet added similar docs for Vec but there is some consensus to do so.

The current approach requires us to ensure the allocator we use matches the one the stdlib does; a requirement that goes away when we handle all allocations and deallocations in a value's lifetime.

Yes, using the same allocator is important. It is (now) easy to do by calling std::alloc::realloc instead of libc::realloc. (std::alloc may not have existed or been stable when mp4parse_fallible was first created.)

I don’t understand this part, could you explain some more?

It's probably not that relevant since I don't see us using anything but the (potentially overridden, as it is in Gecko) global allocator. But if we controlled all allocation and deallocation, I think it would be sound for the allocator to differ from the global one. But, since we now have the ability to defer to whatever the global allocator is via stdlb std::alloc:alloc, there's no need and we can avoid worrying about the deallocation side of things.

Yes, using the same allocator is important. It is (now) easy to do by calling std::alloc::realloc instead of libc::realloc. (std::alloc may not have existed or been stable when mp4parse_fallible was first created.)

Exactly this, we originally created mp4parse_fallible (now moved to the mozilla org) in September 2017 and global allocators weren't available until 1.28.0 in August 2018.

It looks like the author of the fallible_collections crate is amenable to accepting the additions I'm proposing here, so I'll plan to go that route and provide some updates on the progress in this bug. Ultimately the "fix" will then be removing a bunch of code from our tree in favor of vendoring that crate.

The issues to track the work on the fallible_collections crate are:

• Support stable rust
• Add types which require fallible allocation

Supporting stable is often a trade-off. fallible_collections uses a large number of unstable features, so making them all optional might involve non-trivial complexity. I feel that having two crates with similar functionality but different goals might make more sense than dealing with that complexity, but we’ll see what the maintainer of fallible_collections says.

I'm going to hack on it a bit. For the most part, I think the unstable feature will be easy to work around, but if it seems like too much effort relative to just creating a separate crate of our own, I'll do that.

Just a little status update: the contributions to fallible_collections have been accepted and a new version released to crates.io, so it looks like we should be able to move forward with this approach. It's not my top priority, but I think we should be able to complete this in the relatively near term.