Actix-web: SharedHttpInnerMessage has UB

[mitsuhiko]

Currently the SharedHttpInnerMessage has a const to mut cast which is quite unsafe and UB:

https://github.com/actix/actix-web/blob/2f917f37000a1ec72250aa5ec087df00f07e7538/src/server/helpers.rs#L77-L81

This needs to use UnsafeCell at the very last but I'm not even sure if this is safe in general as you can clone the Rc.

[fafhrd91]

Yes, change it to undafecell

[mitsuhiko]

So as far as fixing the UB goes I think this has highest priority as this is where actual soundness issues are. UnsafeCell won't work because there are too many mutable references flying off the http request which contain it. The pattern established here is spread quite well throughout the codebase. So whatever solution emerges for this could then be used in other places as well.

The most trivial case would be to clone the HttpRequest and then have two references to it, where one pokes around in some shared datastructure mutably.

One obvious solution would be to use RefCell and to remove get_mut and friends and have a with_mut that temporarily holds the ref cell as mutable and then passes the reference to a callback. This however is quite invasive to some APIs.

I think a crappy callback based api internally for the shared message is okay. We just need to find solutions for whatever leaks outside. The mutable ones right now are headers/headers_mut/extensions/extensions_mut and some internal uses such as prefix setting which are easier to work with as they are not using any collections.

[DanielJoyce]

Hmm, I guess I don't have enough insight as to WHY something like this needed at all? So I can't even take a stab at refactoring. What is sharedHttpInnerMessage, why does it need this? What does it do?

There is a severe lack of docs in parts and architecture. I think before even tackling unsoundness we need more code docs.

[mitsuhiko]

@DanielJoyce that's a very common pattern in Rust. It's a type (HttpRequest) that wraps an inner type so that inner type can be shared among multiple instances. It lets you cheaply clone the wrapper that holds it.

[DanielJoyce]

The 'callback style' is largely how the various iter apis work. So to me, it seems perfectly good to have an api like that. Callback/builder style apis can be very ergonomic.

Why does it need mutability? The request has already happened.

If middlewares are mutating HttpRequests to pass data to each other, then perhaps the mechanism needs to be changed. Or perhaps to me the naming is ambiguous. There is immutable data, what the client actually sent, modifications to that immutable data if we're having some form of pass-through filter, and middleware context data, which is stuff that is internal to actix and not really part of the http protocol.

[mitsuhiko]

Not sure what you mean with callback style.

Why does it need mutability? The request has already happened.

Because that's generally how middlewares work. It's not an actix specific use case. The problem is not the mutability of the request as such but the fact that immutable clones can exist alongside. A builder would not help here in itself.

[seanmonstar]

I haven't written any middlewares, so I may be missing an important pattern, but instead of questioning why it needs to mutable, which I get (its common in most webframeworks for middleware to inject things as a request goes down the stack), but instead: why does it need to be Clone?

Keeping around a clone of HttpRequest seems less useful. For any data that a middleware wants to keep a copy of, you can quite cheaply clone the single piece: Method, HeaderValue, Uri, etc all are backed by Bytes, which makes clones of that data cheap.

[davidarmstronglewis]

I'm not super familiar with this, but I'm curious what you think about using a Copy on Write approach - middlewares that need to mutate will force a copy of the structure, otherwise you can just pass on the original.

[seanmonstar]

@davidarmstronglewis that is definitely a useful technique in many places (for instance, Bytes is essentially a COW), but for the middleware case, I don't think it can work. You want to be able to insert new state onto the request, and if you got a COW request, your inserts wouldn't propagate down the middleware onion.

[davidarmstronglewis]

So it couldn't work with a middleware taking the state by value and then returning either the original value or a modified copy?

[seanmonstar]

Oh sure, that's the design approach of tower for instance, where middleware just receive ownership of the request, and it's their job to pass it along down the stack.

That pattern could be applied to Actix, instead of passing Middleware::start(&mut req). It'd be a larger change, though. It may be best just to not allow clones of HttpRequest (internally or publicly).

[davidarmstronglewis]

I believe Rack (Ruby) and Plug (Elixir) do the same thing - middleware gets treated like composeable functions where request comes in and request comes out (modified or not). I would be curious to know why that model doesn't work for Actix-Web.

In Rust's case we could actually have a Middleware trait that enforces the contract.

[fafhrd91]

Treating middleware as function complicates client implementation. Sure it is simpler for framework developers but it would make implementions of actual middleware much more complicated

[kosta]

In general, I fear that the design decision needs to be:

a) either make parts of the request/response cheap to clone by using wrapper types like Arc<T>, but that has a runtime cost and probably goes against the performance goals.

or b) have middlewares to actually clone the parts of the request that they need to retain (even if that cloning is expensive for e.g. byte slices etc.) because they usually shouldn't need to retain anything.

What kind of middleware actually needs to retain expensive-to-clone parts of the Request (or Response)? At least the documented ones should not (as far as I can tell). I can't currently think of one that needs to retain parts of a request, but there probably are use cases I'm currently not thinking about...

[DanielJoyce]

I dunno. Composable functions are about the simplest way to code and use middleware.

I was kinda suprised middleware wasn't an actor chain...

[ZerothLaw]

If I might make a suggestion - use a view and transaction pattern.

When the middleware "modifies" the request, what really happens internally is a Transaction is made that tracks the change.

Then at the end, create a view of the resulting data.

a short pseudo-code example:

class Table;
t = Table("table_name");
t.add_row(Row(column1 = "column_1", column2 = "column2"...));
t.add_row(Row(column1 = "column_a", column2 = "columnb"...));
t.remove_row(index=0);

Internally, its a vector or list of Transactions that record the action taken (Remove/Add) for example and the relevant data:

self.transactions = [Transaction(Add, data=&Row), Transaction(Add, data=&Row), Transaction(Remove, data=(index=0))]

If you were to iter through the rows through the table, the table would only offer the second row, because you removed the first one.

This lets the underlying data be immutable (relevant sometimes for web apis, such as taking a hash of the request that was sent vs the request as modified by some middleware), while still allowing mutability.

There may be some performance hits, but it seems to satisfy all the needs Actix has - support middleware and support conformant and safe Rust code.

[mitsuhiko]

@kosta i wrote one recently that benefits from a cheap clone. It only pulls data from the request if errors happen: https://github.com/getsentry/sentry-rust/blob/4b8e387b550a17faea5d5551db762deb954def6c/integrations/sentry-actix/src/lib.rs#L166-L189

[seanmonstar]

@mitsuhiko of the data needed from the request there, cloning each individually would be rather cheap.

• The method is 99.99% going to be one of the constant variants, so cheap clone.
• The header keys are either constants or Bytes, and the header values are Bytes, so those copies are cheap too. Cloning the HeaderMap is a little more, since besides the header names and values, it's also some space for the buckets.
• The URI is also Bytes, and the connection_info data could be changed in actix to likewise be Bytes, I suppose.

---

Whereas, having a look around at the source of sentry's Hub, it looks like a user could accidentally trigger UB because of that req.clone() the middleware is doing. That's because a user can get the &Arc<Hub> from the HttpRequest, and then it's safe to clone that Arc and send it, perhaps to another Actor (which seems to be always a different thread in Actix).

If that http Worker were to then panic, not allowing the SentryMiddleware::response function to flush that !Send event processor, then when the other actor drops the Hub later, the destructor doesn't check that it's Box<FnOnce> is on a different thread, allowing races with the Rc inside HttpRequest, and any other !Send extensions other middleware may have added.

Yes, it's possible to eventually find the holes and plug them, but the safety would be much easier to reason about if a) HttpRequest didn't implement Clone, and b) Hub didn't allow !Send event processors.

[mitsuhiko]

If that http Worker were to then panic, not allowing the SentryMiddleware::response function to flush that !Send event processor, then when the other actor drops the Hub later, the destructor doesn't check that it's Box is on a different thread, allowing races with the Rc inside HttpRequest, and any other !Send extensions other middleware may have added.

Sentry uses my fragile crate for that now so that particular issue should not arise.

Hub didn't allow !Send event processors.

That turns out to be really hard to achieve not just because of actix. There is a ton of non sendable stuff that one would want to capture. One solution would be to have a non sendable hub and I tried that, but it creates an incredibly awkward api.

[seanmonstar]

I don't mean to hijack this to discuss Hub, just a last comment:

Hub will currently double-drop the event processors, because of transmuting from FnOnce to FnMut. That means the data is "moved" (or dropped if only used in the closure, but not returned) once during the call, and then again when the Box<FnOnce> is dropped afterwards: playpen showing memory corruption: https://play.rust-lang.org/?gist=5e571436cd13f491579473d20086329e&version=stable&mode=debug

[mitsuhiko]

Good point thanks. Need to fix that. Hate unsafe :)

[fafhrd91]

fixed by #348