Ouroboros is unsound

[GRA0007]

The creator of Ouroboros has just posted about the unsoundness of ouroboros, and indicated that if you're currently using it you should migrate to self_cell instead where possible.

GitHub issue: someguynamedjosh/ouroboros#88
Security advisory: GHSA-87mf-9wg6-ppf8

[billy1624]

Hey @GRA0007, thanks for letting us know. We will investigate it right now. And then perform the migration :)

[tyt2y3]

To be precise, ouroboros is only used in our stream() interface.

In short, we need a way to tell the compiler that the Stream depends on the Connection and it cannot outlive the connection, while we are passing the Stream around. So it creates a self-referencing scenario.

I reckon, an ugly solution might be to use something like https://docs.rs/async-stream/latest/async_stream/ to implement the Stream interface? But then we'd have to Box them once again.

@nappa85 what's your thought?

[nappa85]

ouroboros is needed because the stream contains also the connection it originates from, hence it's a self-referential struct.

I don't know async_stream, but reading the docs it seems to use a channel to send stream results to the awaiter.

Just writing my thoughts...

It would be easy to do the same with tokio::spawn and a channel that implements Stream, but I think we want to avoid the channel to have a buffer, or we could end up having the entire Stream contents allocated inside the buffer. We should find a channel type that, on send, awaits that the single slot buffer is emptied by the receiver

Could spawning the stream in a separate task cause problems in some situations? Do we still need to be compatible with async-std?

[tyt2y3]

Do we still need to be compatible with async-std?

Yes, I think we are still supporting async-std

We should find a channel type that, on send, awaits that the single slot buffer is emptied by the receiver

I am aware of flume::bounded(0), and have been using it a lot in SeaStreamer

It would be easy to do the same with tokio::spawn

I have made a very basic async runtime generic spawn.

Would this work?

[billy1624]

I just tried self_cell. I was blocked by the fact that I cannot take ownership of the connection pool. It used to be possible in ouroboros

impl QueryStream {
    #[instrument(level = "trace", skip(metric_callback))]
    fn build(
        stmt: Statement,
        conn: InnerConnection,
        metric_callback: Option<crate::metric::Callback>,
    ) -> QueryStream {
        QueryStream::new(
            QueryStreamInner(stmt, conn, metric_callback),
            |QueryStreamInner(stmt, conn, _metric_callback)| match conn { // This `conn` is borrowed (in "self_cell") not owned (in "ouroboros")
                #[cfg(feature = "sqlx-mysql")]
                InnerConnection::MySql(c) => {
                    let query = crate::driver::sqlx_mysql::sqlx_query(stmt);
                    let _start = _metric_callback.is_some().then(std::time::SystemTime::now);
                    let stream = c // We need to take ownership here
                        .fetch(query)
                        .map_ok(Into::into)
                        .map_err(sqlx_error_to_query_err);
                    let elapsed = _start.map(|s| s.elapsed().unwrap_or_default());
                    MetricStream::new(_metric_callback, stmt, elapsed, stream)
                }

[nappa85]

Ok, with runtime abstraction and flume we should be able to do something like this (really high level):

        let (tx, rx) = flume::bounded(0);
        spawn(async move {
                let query = crate::driver::sqlx_mysql::sqlx_query(stmt);
                let mut stream = conn
                    .fetch(query)
                    .map_ok(Into::into)
                    .map_err(sqlx_error_to_query_err);
                while let Some(row) = stream.next().await {
                  if tx.send_async(row).is_err() {
                    break;
                  }
                }
            }
          }
        });
        rx.into_stream()

Main problem I see from that snippet is that we lose metric support, I need to better think about it...

I'm open to proposals for a better solution

[billy1624]

Seems ouroboros team found a solution to the unsound issue? someguynamedjosh/ouroboros#88 (comment)

[tyt2y3]

Well, that is a good news! Let me re-read the issue and see if this issue may be closeable too (at least we have to migrate to 0.16 on master)

[nappa85]

It would be useful to know if the unsoundness has hit us, because I never had a single problem with our streams, so or the bug is really difficult to trigger, or we were using the crate the right way and we weren't affected

[tyt2y3]

Ref: https://github.com/SeaQL/sea-orm/security/dependabot/2