Re: [PATCH] Avoid mixing custom and OpenSSL BIO functions

> On 11 Feb 2024, at 19:19, David Benjamin <davidben(at)google(dot)com> wrote:
>
> Hi all,
>
> I've attached a patch for the master branch to fix up the custom BIOs used by PostgreSQL, in light of the issues with the OpenSSL update recently. While c82207a548db47623a2bfa2447babdaa630302b9 (switching from BIO_get_data to BIO_get_app_data) resolved the immediate conflict, I don't think it addressed the root cause, which is that PostgreSQL was mixing up two BIO implementations, each of which expected to be driving the BIO.

Thanks for your patch, I'm still digesting it so will provide more of a review
later.

> It turns out the parts that came from the OpenSSL socket BIO were a no-op, and in fact PostgreSQL is relying on it being a no-op. Instead, it's cleaner to just define a custom BIO the normal way, which then leaves the more standard BIO_get_data mechanism usable. This also avoids the risk that a future OpenSSL will add a now BIO_ctrl to the socket type, with libssl calling into it, and then break some assumptions made by PostgreSQL.

+ case BIO_CTRL_FLUSH:
+ /* libssl expects all BIOs to support BIO_flush. */
+ res = 1;
+ break;

Will this always be true? Returning 1 implies that we have flushed all data on
the socket, but what if we just before called BIO_set_retry..XX()?

> I've attached a patch which does that. The existing SSL tests pass with it, tested on Debian stable. (Though it took me a few iterations to figure out how to run the SSL tests, so it's possible I've missed something.)

We've done a fair bit of work on making them easier to run, so I'm curious if
you saw any room for improvements there as someone coming to them for the first
time?

--
Daniel Gustafsson