From f196006bd9255c0e8a5efa2085959374b69593cb Mon Sep 17 00:00:00 2001 From: Pierre BAUDEMONT Date: Thu, 8 Sep 2022 16:59:15 +0200 Subject: [PATCH] FIX proposal for a crash that happens when feeding ACE::sendv_n_i and ACE::writev_n with iovecs that have been allocated on the heap. In case of a partial send, libACE will loop until all the data has been sent. The problem is that it updates its arguments when doing so, leaving the caller with one or more updated pointers inside the table of iovecs. Attempting to free such a pointer results in a fault (the value can even not be aligned properly). The fix simply copies the iovec data _if necessary_ (no perf penalty otherwise), and updates those values instead of the arguments. --- ACE/ace/ACE.cpp | 120 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 111 insertions(+), 9 deletions(-) diff --git a/ACE/ace/ACE.cpp b/ACE/ace/ACE.cpp index 565b36876f07d..4838a9103512d 100644 --- a/ACE/ace/ACE.cpp +++ b/ACE/ace/ACE.cpp @@ -1777,7 +1777,13 @@ ACE::sendv_n_i (ACE_HANDLE handle, size_t &bytes_transferred = bt == 0 ? temp : *bt; bytes_transferred = 0; - iovec *iov = const_cast (i); + const iovec *iov = i; + // as per its prototype, this function shall not modify i, even in case of + // partial send, when it is required to update iov->base + // in that case, this local copy will be used instead + // NOTE: the performance penalty of memory allocation only occurs if + // required + iovec *shadow_iovec = nullptr; for (int s = 0; s < iovcnt; @@ -1819,13 +1825,41 @@ ACE::sendv_n_i (ACE_HANDLE handle, if (n != 0) { + if (shadow_iovec == nullptr) + { +# ifdef ACE_HAS_ALLOC_HOOKS + ACE_ALLOCATOR_RETURN (shadow_iovec, (iovec *) + ACE_Allocator::instance ()->malloc (iovcnt * + sizeof (iovec)), + -1); +# else + ACE_NEW_RETURN (shadow_iovec, + iovec[iovcnt], + -1); +# endif /* ACE_HAS_ALLOC_HOOKS */ + ACE_OS::memcpy(shadow_iovec, iov, iovcnt * sizeof(iovec)); + // from now on, shadow_iovec is the backend data used by iov + iov = shadow_iovec; + } + char *base = reinterpret_cast (iov[s].iov_base); - iov[s].iov_base = base + n; + // updating the value requires a pointer to non-const, so directly + // rely on shadow_iovec + shadow_iovec[s].iov_base = base + n; // This blind cast is safe because n < iov_len, after above loop. - iov[s].iov_len = iov[s].iov_len - static_cast (n); + shadow_iovec[s].iov_len = shadow_iovec[s].iov_len - static_cast (n); } } + if (shadow_iovec != nullptr) + { +# ifdef ACE_HAS_ALLOC_HOOKS + ACE_Allocator::instance ()->free (shadow_iovec); +# else + delete [] shadow_iovec; +# endif /* ACE_HAS_ALLOC_HOOKS */ + } + return ACE_Utils::truncate_cast (bytes_transferred); } @@ -1845,7 +1879,13 @@ ACE::sendv_n_i (ACE_HANDLE handle, int val = 0; ACE::record_and_set_non_blocking_mode (handle, val); - iovec *iov = const_cast (i); + const iovec *iov = i; + // as per its prototype, this function shall not modify i, even in case of + // partial send, when it is required to update iov->base + // in that case, this local copy will be used instead + // NOTE: the performance penalty of memory allocation only occurs if + // required + iovec *shadow_iovec = nullptr; for (int s = 0; s < iovcnt; @@ -1891,13 +1931,41 @@ ACE::sendv_n_i (ACE_HANDLE handle, if (n != 0) { + if (shadow_iovec == nullptr) + { +# ifdef ACE_HAS_ALLOC_HOOKS + ACE_ALLOCATOR_RETURN (shadow_iovec, (iovec *) + ACE_Allocator::instance ()->malloc (iovcnt * + sizeof (iovec)), + -1); +# else + ACE_NEW_RETURN (shadow_iovec, + iovec[iovcnt], + -1); +# endif /* ACE_HAS_ALLOC_HOOKS */ + ACE_OS::memcpy(shadow_iovec, iov, iovcnt * sizeof(iovec)); + // from now on, shadow_iovec is the backend data used by iov + iov = shadow_iovec; + } + char *base = reinterpret_cast (iov[s].iov_base); - iov[s].iov_base = base + n; + // updating the value requires a pointer to non-const, so directly + // rely on shadow_iovec + shadow_iovec[s].iov_base = base + n; // This blind cast is safe because n < iov_len, after above loop. - iov[s].iov_len = iov[s].iov_len - static_cast (n); + shadow_iovec[s].iov_len = shadow_iovec[s].iov_len - static_cast (n); } } + if (shadow_iovec != nullptr) + { +# ifdef ACE_HAS_ALLOC_HOOKS + ACE_Allocator::instance ()->free (shadow_iovec); +# else + delete [] shadow_iovec; +# endif /* ACE_HAS_ALLOC_HOOKS */ + } + ACE::restore_non_blocking_mode (handle, val); if (error) @@ -2154,7 +2222,13 @@ ACE::writev_n (ACE_HANDLE handle, size_t &bytes_transferred = bt == 0 ? temp : *bt; bytes_transferred = 0; - iovec *iov = const_cast (i); + const iovec *iov = i; + // as per its prototype, this function shall not modify i, even in case of + // partial send, when it is required to update iov->base + // in that case, this local copy will be used instead + // NOTE: the performance penalty of memory allocation only occurs if + // required + iovec *shadow_iovec = nullptr; for (int s = 0; s < iovcnt; @@ -2177,13 +2251,41 @@ ACE::writev_n (ACE_HANDLE handle, if (n != 0) { + if (shadow_iovec == nullptr) + { +# ifdef ACE_HAS_ALLOC_HOOKS + ACE_ALLOCATOR_RETURN (shadow_iovec, (iovec *) + ACE_Allocator::instance ()->malloc (iovcnt * + sizeof (iovec)), + -1); +# else + ACE_NEW_RETURN (shadow_iovec, + iovec[iovcnt], + -1); +# endif /* ACE_HAS_ALLOC_HOOKS */ + ACE_OS::memcpy(shadow_iovec, iov, iovcnt * sizeof(iovec)); + // from now on, shadow_iovec is the backend data used by iov + iov = shadow_iovec; + } + char *base = reinterpret_cast (iov[s].iov_base); - iov[s].iov_base = base + n; + // updating the value requires a pointer to non-const, so directly + // rely on shadow_iovec + shadow_iovec[s].iov_base = base + n; // This blind cast is safe because n < iov_len, after above loop. - iov[s].iov_len = iov[s].iov_len - static_cast (n); + shadow_iovec[s].iov_len = shadow_iovec[s].iov_len - static_cast (n); } } + if (shadow_iovec != nullptr) + { +# ifdef ACE_HAS_ALLOC_HOOKS + ACE_Allocator::instance ()->free (shadow_iovec); +# else + delete [] shadow_iovec; +# endif /* ACE_HAS_ALLOC_HOOKS */ + } + return ACE_Utils::truncate_cast (bytes_transferred); }