Canvas unsafe code in the wild

Can you clarify what you mean by a "safety issue"?

Roughly speaking, yes.

definedness issue = may cause UB if the UB rules are not defined finely enough. safety issue = may cause UB if called incorrectly.

For example, Vec::unchecked_get is a "pure-safety" issue - it is obvious whether a particular execution trace is defined (read: it does not access a vector beyond its length), but it might not be obvious whether there exists some unsafe trace, but e.g. aliasing issues are definedness issues - it might not be obvious whether a particular trace is defined or not.

I see, I thought this is what you might have meant. However, I'd draw a distinction between unchecked_get and prohibiting closures that not return normally (i.e., which may use setjmp) -- it seems important to me to highlight things that unsafe code assumes about the universe of all other code.

In particular, I think that eventually we are going to want to enumerate sets of capabilities -- somewhat like I described in this blog post on observational equivalence, though probably not exactly like that -- that unsafe code is allowed to make use of. Or at least enumerate things that we know it cannot do.

As a simple example, if you are writing unsafe code for some application that specifically targets a simple embedded processor which does not support multi-threading, that might allow you to make stronger assumptions in your unsafe code -- but that code would not necessarily be 'composable' with other unsafe code.

(To be clear, I don’t disagree with the distinction you’re drawing or your definitions.)

I thought of a better way to draw the distinction. =)

unchecked_get is an unsafe function. So it’s perfectly valid for it to impose arbitrary constraints on its callers, as long as they are documented, and I agree it’s not a big concern for the UCG if they are violated.

join() is safe. This means that it is leaning on the Rust type system to enforce its constraints. And that is of interest. One of those constraints is that, when we invoke a closure, it will either return or panic.

This transmute works around something that looks like a failure of borrow checker. Intuitively I think it’s safe and should work, but even with a significant refactoring of these methods I could not find a way to avoid the transmute:

I thought I’d chime in with some comments on Abomonation; I assume it is the same one we are talking about as no one else should spell it that way (where is the “5” from? typo? lots of secret forks doing cool things? :D).

Abomonation does several horrible things, and I wouldn’t be surprised if the correct answer is not “this should be defined behavior”. But, I thought I would point out some of the things that it does, as different from the problem it thinks it has to solve. I have an interest in something with similar properties being possible, even if the pile of code that is Abomonation needs to be deleted and zeroed over several times.

Abomonation was an attempt to deal with the issue that for data intensive compute with communication (ie multiple workers) one really wants some sort of region allocation in order to efficiently bundle up data, ship them to other workers, and unpack the data on the other end. The “competition” in the HPC space just memcpys plain old data around, the competition in the JVM space takes comically large amounts of time to serialize data (or roll their own off-heap serialized representations), and the competition in the C/C++ space does a heap crawl but then has to strongly warn the recipient that they should not use methods that expect to mutate anything about the interpreted data. Rust seems well positioned to do the efficient thing, safely.

Abomonation’s implementation was the optimistic take that maybe the minimum amount of work would be enough: memcpy on each pointer you find into a new region on the encoding side, and some pointer correction and casting on the receiving side. Other options exist, like CapnProto, but there is an ergonomic hit where types in use need some macro love (this is less a problem if you are serializing your own few types, and more of a problem if you are writing middleware for other people who wouldn’t otherwise know you have to do this for them).

There are a few concrete things I know of that are presently UB with respect to Rust’s guidelines:

  1. Abomonation calls memcpy to read from structs which may have padding bytes. The last version of the docs that I read said that reads of undefined values is undefined behavior. This is not what LLVM says (such reads result in undef values, which may become UB if they are used badly, e.g. in division denominators), but I can understand why Rust would be more restrictive (perhaps it doesn’t need to be in this case, though).

  2. Abomonation totally ignores alignment, and could produce references to types that are not aligned with properties that their initial allocation had. This can be fixed, but it may mean a copy to shift everything by just a few bytes, or only reading data into known aligned locations.

  3. Abomonation totally ignores widths, with the assumption that you are using the same binary to encode and decode the data.

Abomonation also does some other weird things that I’m not even sure if they are clearly defined or undefined. The main one is to (i) make a copy of reachable memory from some typed reference &T, (ii) correct pointers in that copy to reference their corresponding locations in the copy, then (iii) cast the result to a &T and act as if it is valid. The UB spec says something about “it is UB for values to be invalid”, where clearly whether what Abomonation does being UB or not depends on what is or is not valid; I suspect the fact that the values are behind a reference is not a saving grace.

For example:

  1. I could imagine some version of this working out just fine for &[u64], which has a well-defined size and alignment and all of that.

  2. I would imagine no version should work out just fine for &Rc<T>, because the implementation of clone does some funny business with UnsafeCell that is very unlikely to be faithfully reproduced (maybe it works? maybe it just writes over the strong count location in the underlying &[u8], or maybe it does this and some NOALIAS assumptions elsewhere break).

  3. I don’t know whether and how this would work for Vec<T> or Option<T> or other composite types. Each of these have some baked in assumptions about their structure, and may not be valid just because you copied their exact bytes. At the moment it “seems to” work, in that I encode and decode a lot of data and don’t experience problems.

On the encode side of things, I have to imagine there is some version of “copy lots of bytes into a buffer” that isn’t undefined behavior, because there are no particular invariants about the resulting Vec<u8>. The decode side seems to be where things are terrifying, because the intent is to let otherwise oblivious Rust code use a &T as if it were such a thing.

So, it is all a bit scary, and a bit of a mess. I’d love to get some guidance (perhaps as a part of this process, or perhaps as the result) about how to make it UB conformant.

I want to double-extra stress that from my point of view it is important to be able to have some sort of correspondence between in-memory representations and “on-disk” representations, if you want performance from data-intensive computation. repr(C) is one way out, but it has a similar ergonomic overhead to CapnProto: either users need to use it on their custom what-would-otherwise-be- (String, usize) types, or accept that each type is wrapped by the system preventing them from using code that expects a &[(String, usize)], and having them re-write it as taking something like &[Wrapper<(String, usize)>]. Either of these approaches also increase the cost of moving data from a Vec<T> into a Vec<Wrapper<T>>, where we may not be able to just memcpy data any more.

Can I set repr(C) for an entire project? This is something I would like out of the UB spec: some way to reliably know (or insist on) something about the in-memory representation of types in the programs I run.

Edit: Just to re-iterate, it is not important that I insist on a specific in-memory representation, but rather consistency of the in-memory representation, whatever its layout. For example, Rust could hypothetically create behind the scenes a few specializations of types with different layouts, and convert between them with care. This would utterly crush Abomonation’s plan, and would stress me out greatly. Field orders that vary build-to-build (which is apparently in nightly?) are also stressful, but are apparently appealing enough to other people that they are worth doing (can I insist on Abomonation vs Serde serialization numbers in any perf benchmarks they produce?).

Edit 2: Awesome side information: Abomonation’s unsafe is literally the only unsafe code in timely dataflow (that I know of). I think that is pretty amazing for the language.

5 Likes

Also, in case folks haven’t seen it, there is a PLDI 2017 paper on cutting down UB in LLVM. I’m not sure it has immediate relevance here (it seems to be mostly about managing undef and poison in llvm), but it might be that the ideas are helpful in some way (and knowing more is better than knowing less).

1 Like

Okay so I had a closer look at Rc. I know I got assigned Arc, but I am more familiar with Rc because I looked very closely at it as part of the RustBelt research project.

Mostly, in terms of what we are discussing as part of the unsafe code guidelines, Rc seems to be fairly harmless. Here’s what I found noteworthy. There’s nothing big, I just felt I couldn’t come back without anything to report :wink:

Struct field offsets

into_raw and from_raw do some pointer arithmetic based on offsets of struct fields, I think there is general agreement that this should be possible. The concrete implementation of this in the offset_of! macro uses mem::uninitialized() and then calls mem::forget() in the result. This means mem::forget can get called with a totally invalid argument, but that’s not too surprising – in fact, this is even suggested by the official documentation of mem::foget(). I was a little surprised that the offset is in fact computed at run-time; you would think that the field offset is a compile-time constant.

__from_str, __from_array

These are super-dubious, as even the comments in there say. If I read the code correctly, they assume that a fat pointer can be created by creatively casting a two-element array… However, they are private to rustc.

RcBoxPtr

Rc::drop calls RcBoxPtr::dec_weak after decrementing the strong count and droping the content of the RcBox. This is somewhat interesting because RcBoxPtr::dec_weak takes &Rc<T> as an argument and is a perfectly safe function, but it is actually called on an Rc that is in a bad state – it doesn’t satisfy the usual invariants that Rc typically satisfies. However, the Rc passed to dec_weak is still a valid instance of a "syntactic Rc", so I don’t think the compiler could exploit this in any way.

Funny enough, and unsurprisingly, you are essentially replicating here an example from a paper on control effects. That paper has examples of programs that are equivalent when there are no "first-class control effects" (i.e. no first-class continuations), but are no longer equivalent once you add call/cc or setjmp/longjmp.

But yeah, what we are fundamentally observing here is that the closure-based "trick" to get around the fact that destructors are not guaranteed to run, does not work on languages with first-class control effects.

Calling memcpy must always be valid, even if the memory is uninitialized. So I think you are good here. However, one interesting question is whether it is possible to implement memcpy in Rust by doing byte-wise copy of a [u8], or whether it will have to be a magical intrinsic. It's actually not easy to make memcpy implementable, and uninitialized bytes (e.g. padding) are part of the reason why (the other part is pointers).

Making assumptions about their structure is one part. The other interesting part is the tricks you are playing with the borrow checker. In fact, I think that part of Abomonation can be "distilled down" by considering a function fn fake_box(&&T) -> &Box<T> implemented as a transmute. I think this is very close to what happens with Vec<T> in Abomonation, except that we are side-stepping most of the discussion about struct layout. (Please correct me if I am wrong.) The good news is that we have actually proven such a conversion function to be safe in our restricted model of Rust. :slight_smile:

I think this is correct (approximating fake_box as "what I do with Vec" ;)). The lifetimes are for sure "scary" but I think I convinced myself that they are legit; if that is what you've proven safe, awesome. :smiley:

This is necessary because in the case of Rc<TraitObject>, the offset of the inner value depends on the alignment of the actual object. For trait objects, the alignment is a field in the vtable, which the compiler uses to calculate the offset.

1 Like

You are saying that for the same trait, the offset in Rc<Trait> can differ? Wow, I did not realize that is possible. Thanks for pointing it out!

In the compiler sprint, we felt that it would be possible, but using a union to account for the 'maybe uninitialized' nature of the data (that is, you could cast your &[u8] into a &[Uninit<u8>], where union Uninit<T> { init: T, uninit: () } (or something like that).

30 posts were split to a new topic: Role of UB / uninitialized memory

  • In ring, we sometimes need to treat a &[u64] slice as a &[u32] slice and/or a &[u8] slice. We use unsafe to do this, but we would prefer it to be built into libcore or the language instead.

  • For various reasons we benefit from converting a slice &[u8] to an array reference &[u8; n] after verifying that the slice has n elements, which requires us to use unsafe. This is another thing that we think should be built-in to the language and/or standard library instead.

We keep our hacks for things that should be built-in in one module:

2 Likes

Almost the same thing mentioned by Brian is done in the RustCrypto too. This code is stored in the separate crate called byte-tools. Essentially it’s a specialized version of byteorder without generic capabilities and with operations on slices.

I agree with Brian and think that such tools must be included in the standard library.

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.