[Backport][0.8 to 0.7] | fix(net,io): don't confuse a 0-length iovec element or an error with EOF (#1602) (#1603) (#1604) - #1607
Draft
photonlibos wants to merge 1 commit into
Conversation
…EOF (#1602) (#1603) (#1604) * fix(net): don't mistake 0-length iovec elements for EOF in writev() BufStepV drove doio_loop() by `iovcnt > 0` instead of the number of remaining bytes, and iovector_view::extract_front(0) pops nothing, so a 0-length element could stay at the head of the view and make the next iocb transfer 0 byte. doio_loop() takes a return of 0 for EOF, hence: - TLS streams silently truncated writev()/readv(): send(iov, iovcnt) only serves iov[0], so a 0-length head returned 0 right away. Of a 100+0+50 bytes writev() only 100 bytes got transferred, and only 0 when the empty element came first, with no error and errno intact. - ZeroCopy streams hung: the extra 0-byte sendmsg(MSG_ZEROCOPY) bumped m_num_calls, but no completion notification ever arrives for it, so zerocopy_confirm() waited forever, or until the stream timeout. - SASL streams returned 0 and discarded the bytes already transferred, as a 0-length segment made write() return 0, and `ret <= 0` dropped the accumulated `count`. Make BufStepV skip 0-length elements, so that the iocb is never invoked with a 0-byte request. This covers all ten BufStepV users, and also drops the useless 0-byte sendmsg on plain TCP sockets. Before the first iocb one element is kept, as iocbs serving only iov[0] need a valid one. Also guard ZeroCopySocketStream::do_sendmsg() against 0-byte messages, which BufStepV cannot reach when send(iov, iovcnt) is called directly, and fix SASL writev()/readv() separately, as they don't use BufStepV. Plain TCP sockets were not affected: sendmsg() returns 0 only once the whole remaining array sums to 0, at which point the accumulated count already equals the requested total. Add writev.empty_iov{,_tls,_zerocopy}, covering a 0-length element at the head, in the middle and at the tail, plus an all-empty array, checking both the return value and the bytes seen by the peer. Validated: the new tests fail without this fix (TLS 150 -> 100 and 150 -> 0, ZeroCopy timing out) and pass with it; the socket, TLS, RPC and HTTP suites are unaffected. The SASL change is not compile-tested here, as gsasl is unavailable and ENABLE_SASL is off. Fixes #1601. * fix(io): report errors of posixaio preadv()/pwritev() instead of 0 posixaio::preadv() / pwritev() turned a failed pread/pwrite into a return value of 0, which callers read as EOF or "nothing transferred" rather than as an error. LOG_ERRNO_RETURN(-1, 0, ...) also overwrote the errno set by the callee with the meaningless value -1, erasing the last clue about what actually went wrong. Return -1 when nothing has been transferred yet, and pass 0 as new_errno so that the callee's errno survives. When part of the iov[] array has already been transferred, return the accumulated count, as preadv(2) and pwritev(2) do; this is also the idiom the preadv() / pwritev() of windows_compat.cpp and iocp.cpp already follow. The 0-length elements of iov[] are not affected: the loop advances on `ret < iov_len`, which is the correct predicate, and lets a 0-length element through. Found while auditing the iovec iteration paths for the confusion between a 0-length element and a 0-byte I/O result. Validated: test-syncio, which drives posixaio through do_test_aio, plus test-fs, test-exportfs, test-socket and test-tls all pass. Co-authored-by: Coldwings <coldwings@me.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BufStepV drove doio_loop() by
iovcnt > 0instead of the number ofremaining bytes, and iovector_view::extract_front(0) pops nothing, so a
0-length element could stay at the head of the view and make the next
iocb transfer 0 byte. doio_loop() takes a return of 0 for EOF, hence:
only serves iov[0], so a 0-length head returned 0 right away. Of a
100+0+50 bytes writev() only 100 bytes got transferred, and only 0
when the empty element came first, with no error and errno intact.
m_num_calls, but no completion notification ever arrives for it, so
zerocopy_confirm() waited forever, or until the stream timeout.
as a 0-length segment made write() return 0, and
ret <= 0droppedthe accumulated
count.Make BufStepV skip 0-length elements, so that the iocb is never invoked
with a 0-byte request. This covers all ten BufStepV users, and also
drops the useless 0-byte sendmsg on plain TCP sockets. Before the first
iocb one element is kept, as iocbs serving only iov[0] need a valid one.
Also guard ZeroCopySocketStream::do_sendmsg() against 0-byte messages,
which BufStepV cannot reach when send(iov, iovcnt) is called directly,
and fix SASL writev()/readv() separately, as they don't use BufStepV.
Plain TCP sockets were not affected: sendmsg() returns 0 only once the
whole remaining array sums to 0, at which point the accumulated count
already equals the requested total.
Add writev.empty_iov{,_tls,_zerocopy}, covering a 0-length element at
the head, in the middle and at the tail, plus an all-empty array,
checking both the return value and the bytes seen by the peer.
Validated: the new tests fail without this fix (TLS 150 -> 100 and
150 -> 0, ZeroCopy timing out) and pass with it; the socket, TLS, RPC
and HTTP suites are unaffected. The SASL change is not compile-tested
here, as gsasl is unavailable and ENABLE_SASL is off.
Fixes #1601.
posixaio::preadv() / pwritev() turned a failed pread/pwrite into a
return value of 0, which callers read as EOF or "nothing transferred"
rather than as an error. LOG_ERRNO_RETURN(-1, 0, ...) also overwrote
the errno set by the callee with the meaningless value -1, erasing the
last clue about what actually went wrong.
Return -1 when nothing has been transferred yet, and pass 0 as
new_errno so that the callee's errno survives. When part of the iov[]
array has already been transferred, return the accumulated count, as
preadv(2) and pwritev(2) do; this is also the idiom the preadv() /
pwritev() of windows_compat.cpp and iocp.cpp already follow.
The 0-length elements of iov[] are not affected: the loop advances on
ret < iov_len, which is the correct predicate, and lets a 0-lengthelement through. Found while auditing the iovec iteration paths for
the confusion between a 0-length element and a 0-byte I/O result.
Validated: test-syncio, which drives posixaio through do_test_aio,
plus test-fs, test-exportfs, test-socket and test-tls all pass.
Co-authored-by: Coldwings coldwings@me.com
Generated by Backport Auto PR, by cherry-pick related commits.
Please review and decide whether to merge or close this backport PR.
Conflicts
Cherry-pick produced conflicts. Conflict markers are committed as-is; please resolve them manually before merging.