search

netty / netty-tcnative

A fork of Apache Tomcat Native, based on finagle-native
Apache License 2.0
239 stars 179 forks source link

Remove accesses to private field bio->num #864

Closed davidben closed 6 months ago

davidben commented 6 months ago

It looks like https://github.com/netty/netty-tcnative/pull/731 added access to the private bio->num field in netty-tcnative. As documented, BIO is a private struct. We hadn't gotten around to making it opaque at the time, but we will do so in the future.

Looking at the PR, this change is totally incoherent. A bytebuffer BIO doesn't use the file descriptor in the first place!

It seems this was done to be compatible with some code that reaches into internal structures and tries to grab the rbio output, and the further peeks around there to pull a file descriptor out of bio->num. https://github.com/pixie-io/pixie/blob/294d3f36214dece583dd499ebd578cecb7cc7425/src/stirling/source_connectors/socket_tracer/bcc_bpf/openssl_trace.c#L59-L78

This is broken for many, many reasons. First, these offsets are not part of BoringSSL (or OpenSSL's) public API and thus digging into offsets like this is not supported. Second, in both OpenSSL and BoringSSL, there is no guarantee that rbio is based on file descriptors. Indeed, in netty-tcnative, they are not.

As this kind of thing is not remotely supported by BoringSSL, we won't take it into account when making changes, namely hiding BIO and adjusting offsets. We do have a few more things to clear before making BIO opaque, so your build is not in immediate danger of breaking, but it will happen eventually.

Please revert #731, or at least remove BoringSSL from the ifdefs. (I imagine bioSetFd can't be removed due to API stability reasons, but I recommend turning it into a no-op.)

davidben commented 6 months ago

BTW if you all really need to support this thing, you can stash the fd, well, just about anywhere else. :-) There are no end of APIs to stick data onto things. For instance, you could have allocated some data and used BIO_set_data.

Of course, the design of this tool seems fundamentally unsupportable since it reaches into internal structs, but if you don't mind that tool potentially breaking on every update, I suppose that fine. But accessing bio->num will result in it breaking for everyone later.

normanmaurer commented 6 months ago

Will have a look