Packet timestamp for 2.1.13 release

[trondn]

Added support for packet timestamp

[jimwwalker]

These were from a broader review so hopefully the end of these staggeringly hard to follow flows 😆

[jimwwalker]

AI suggested this may not work in the case when bufferevent_setfd is used to replace the FD associated with the buffer (i don't think couchbase does that, but the library permits?)

Setup phase — where the flag gets set

1. bufferevent_socket_new(base, fd1, BEV_OPT_RECV_TIMESTAMPS) — bufferevent_sock.c:419-424: since fd1 >= 0, it calls be_socket_enable_timestamps_(fd1).
2. be_socket_enable_timestamps_ — bufferevent_sock.c:99-119: does setsockopt(fd1, SOL_SOCKET, SO_TIMESTAMPNS, ...) (falling back to SO_TIMESTAMP). On success, back in the constructor: bufev_p->recv_timestamps_enabled = 1 (bufferevent_sock.c:421-423).

Note the asymmetry created here: the setsockopt applied to fd1; the flag is stored on the bufferevent and nothing ever ties it back to which fd it was armed for.

Switch phase — where the new fd escapes arming

3. App replaces the socket: bufferevent_setfd(bev, fd2) — bufferevent.c:865-872: packs d.fd = fd2 and dispatches bev->be_ops->ctrl(bev, BEV_CTRL_SET_FD, &d) (872).
4. be_socket_ctrl — bufferevent_sock.c:756-761: case BEV_CTRL_SET_FD: → be_socket_setfd(bev, fd2) (761).
5. be_socket_setfd — bufferevent_sock.c:675-707: re-assigns the read/write events onto fd2 (688-691), then reaches the re-arm block at 697:
if ((bufev_p->options & BEV_OPT_RECV_TIMESTAMPS) && !bufev_p->recv_timestamps_enabled) {
5. The first condition is true, but recv_timestamps_enabled is still 1 from fd1 — so !bufev_p->recv_timestamps_enabled is false and the whole block is skipped. setsockopt(fd2, SO_TIMESTAMPNS) is never issued. fd2 has no timestamping enabled in the kernel, while the bufferevent believes it does.

[jimwwalker]

An issue here?

Background — how an SSL gets distinct read/write BIOs. SSL_set_fd() / BIO_new_socket() create one socket BIO used for both directions — the common case, and the only one this code contemplates. But OpenSSL explicitly supports split pairs: SSL_set_rfd() / SSL_set_wfd() (different fds for read and write — e.g. a pipe pair), or SSL_set_bio(ssl, rbio, wbio) with different BIOs — e.g. a read-side buffering/filter chain (BIO_push(BIO_new(BIO_f_buffer()), sock_bio)) with the bare socket BIO on the write side. bufferevent_openssl_socket_new takes a caller-constructed SSL, so any of these can arrive here.

The code path — bufferevent_openssl.c:1782-1822

1. Line 1783: bio = SSL_get_wbio(ssl) — only the write BIO is ever fetched. SSL_get_rbio() is never consulted in this function.
2. Lines 1787-1801: have_fd = BIO_get_fd(bio, NULL) — the fd is learned from the wbio — then reconciled with the fd parameter.
3. Lines 1803-1807: be_socket_enable_timestamps_(fd) succeeds → recv_timestamps_enabled = 1. Note this is the write-side fd if the pair is split.
4. Line 1808: if (recv_timestamps_enabled && BIO_method_type(bio) == BIO_TYPE_SOCKET) — the type check is against the wbio only.
5. Line 1809: close_flag = BIO_get_close(bio) — old wbio's close flag captured.
6. Line 1810: new_bio = BIO_new_socket_recvmsg((int)fd, close_flag) — new BIO wrapping the write-side fd.
7. Line 1814: BIO_set_close(bio, BIO_NOCLOSE) — so the old wbio won't close the fd when it's freed in the next step.
8. Line 1815: SSL_set_bio(ssl, new_bio, new_bio) — this is where the clobber happens. Per OpenSSL semantics, SSL_set_bio releases the SSL's references on both the current rbio and the current wbio, freeing each whose refcount hits zero, and installs new_bio in both slots.

When rbio == wbio (the SSL_set_fd case), step 8 frees the one old socket BIO and everything is correct. When they're distinct:

Consequence A — the read-side BIO chain is destroyed, with its state. The old rbio is freed as a side effect of line 1815, silently (SSL_set_bio returns void; nothing here can fail). If it was a buffering filter, any ciphertext bytes it had already pulled from the socket and was holding are destroyed with it — from the peer's perspective those TLS records were delivered; from this SSL's perspective they never existed. That's a mid-stream data loss → decrypt failures or a hang, far removed from the constructor call that caused it.

Consequence B — reads move to the wrong fd. In the SSL_set_rfd(r)/SSL_set_wfd(w) configuration, new_bio wraps the write fd (that's where have_fd came from, line 1788). After line 1815 both directions run on fd w; fd r is orphaned. And if the old rbio had its close flag set, freeing it at line 1815 closes fd r — an fd the application still considers open, which is how you get the classic "closed fd number gets reused by an unrelated connection" class of bug.

Consequence C (nit, already noted) — the comment lies about ownership. Line 1809 threads close_flag into the new BIO and the comment at 1812-1813 says "new_bio now owns the fd with the original close_flag" — but lines 1819-1822 immediately execute BIO_set_close(bio, 0) on the new BIO, discarding the transferred flag. The behavior is right and matches the pre-patch code (the bufferevent owns the fd; be_openssl_destruct's BEV_OPT_CLOSE_ON_FREE path at 1532-1541 closes it), but the comment describes intent the code cancels four lines later.

Contrast with the pre-patch code. Before this change the block was a single BIO_set_close(bio, 0) — it flipped a flag on the wbio and replaced nothing, so a split rbio/wbio pair passed through untouched. The regression is specifically that a configuration-inspection step became a configuration-replacement step gated on inspecting only half the configuration.

Suggested fix. Gate the replacement at line 1808 on the pair actually being one shared socket BIO:

if (recv_timestamps_enabled &&
    SSL_get_rbio(ssl) == SSL_get_wbio(ssl) &&
    BIO_method_type(bio) == BIO_TYPE_SOCKET) {

For split pairs the replacement is simply skipped and behavior falls back gracefully: recv_timestamps_enabled may still be set by new_impl (bufferevent_openssl.c:1713-1717), but do_read only consults the BIO data when the rbio is actually the recvmsg type (BIO_method_type(rbio) == BIO_TYPE_LIBEVENT_RECVMSG at 846-850), so it degrades to "no timestamps" rather than misbehaving — the same silent-degradation contract the feature already has on unsupported platforms. If that silence is a concern, the author could document that BEV_OPT_RECV_TIMESTAMPS requires the SSL to use a single socket BIO for both directions (worth a sentence in the bufferevent.h doc block either way).