From: Christos Tsantilas Date: Fri, 6 Oct 2017 04:20:31 +0000 (+0300) Subject: Bug 4718: Support filling raw buffer space of shared SBufs (#64) X-Git-Tag: SQUID_4_0_22~6 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=676fad138d2e168037360170a35ae4b0bc4ceb5b;p=thirdparty%2Fsquid.git Bug 4718: Support filling raw buffer space of shared SBufs (#64) SBuf::forceSize() requires exclusive SBuf ownership but its precursor SBuf::rawSpace() method does not guarantee exclusivity. The pair of calls may result in SBuf::forceSize() throwing for no good reason. New SBuf API provides a new pair of raw buffer appending calls that reduces the number of false negatives. This change may alleviate bug 4718 symptoms but does not address its core problem (which is still unconfirmed). This is a Measurement Factory project. --- diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index 9d2d99c849..9877255e48 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -467,7 +467,7 @@ void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io) Must(readBuf.length() < SQUID_TCP_SO_RCVBUF); // now we can ensure that there is space to read new data, // even if readBuf.spaceSize() currently returns zero. - readBuf.rawSpace(1); + readBuf.rawAppendStart(1); CommIoCbParams rd(this); // will be expanded with ReadNow results rd.conn = io.conn; diff --git a/src/base/File.cc b/src/base/File.cc index 0ccbe38706..d181d8064b 100644 --- a/src/base/File.cc +++ b/src/base/File.cc @@ -244,14 +244,15 @@ File::readSmall(const SBuf::size_type minBytes, const SBuf::size_type maxBytes) { SBuf buf; const auto readLimit = maxBytes + 1; // to detect excessively large files that we do not handle + char *rawBuf = buf.rawAppendStart(readLimit); #if _SQUID_WINDOWS_ DWORD result = 0; - if (!ReadFile(fd_, buf.rawSpace(readLimit), readLimit, &result, nullptr)) { + if (!ReadFile(fd_, rawBuf, readLimit, &result, nullptr)) { const auto savedError = GetLastError(); throw TexcHere(sysCallFailure("ReadFile", WindowsErrorMessage(savedError).c_str())); } #else - const auto result = ::read(fd_, buf.rawSpace(readLimit), readLimit); + const auto result = ::read(fd_, rawBuf, readLimit); if (result < 0) { const auto savedErrno = errno; throw TexcHere(sysCallError("read", savedErrno)); @@ -260,7 +261,7 @@ File::readSmall(const SBuf::size_type minBytes, const SBuf::size_type maxBytes) const auto bytesRead = static_cast(result); assert(bytesRead <= readLimit); Must(!buf.length()); - buf.forceSize(bytesRead); + buf.rawAppendFinish(rawBuf, bytesRead); if (buf.length() < minBytes) { const auto failure = buf.length() ? "premature eof" : "empty file"; diff --git a/src/comm/Read.cc b/src/comm/Read.cc index 9946efb267..6fa09baa04 100644 --- a/src/comm/Read.cc +++ b/src/comm/Read.cc @@ -84,7 +84,7 @@ Comm::ReadNow(CommIoCbParams ¶ms, SBuf &buf) SBuf::size_type sz = buf.spaceSize(); if (params.size > 0 && params.size < sz) sz = params.size; - char *inbuf = buf.rawSpace(sz); + char *inbuf = buf.rawAppendStart(sz); errno = 0; const int retval = FD_READ_METHOD(params.conn->fd, inbuf, sz); params.xerrno = errno; @@ -92,7 +92,7 @@ Comm::ReadNow(CommIoCbParams ¶ms, SBuf &buf) debugs(5, 3, params.conn << ", size " << sz << ", retval " << retval << ", errno " << params.xerrno); if (retval > 0) { // data read most common case - buf.append(inbuf, retval); + buf.rawAppendFinish(inbuf, retval); fd_bytes(params.conn->fd, retval, FD_READ); params.flag = Comm::OK; params.size = retval; diff --git a/src/fs/rock/RockHeaderUpdater.cc b/src/fs/rock/RockHeaderUpdater.cc index 981ae8281c..22487324a4 100644 --- a/src/fs/rock/RockHeaderUpdater.cc +++ b/src/fs/rock/RockHeaderUpdater.cc @@ -98,24 +98,25 @@ Rock::HeaderUpdater::stopReading(const char *why) void Rock::HeaderUpdater::NoteRead(void *data, const char *buf, ssize_t result, StoreIOState::Pointer) { + IoCbParams io(buf, result); // TODO: Avoid Rock::StoreIOStateCb for jobs to protect jobs for "free". CallJobHere1(47, 7, CbcPointer(static_cast(data)), Rock::HeaderUpdater, noteRead, - result); + io); } void -Rock::HeaderUpdater::noteRead(ssize_t result) +Rock::HeaderUpdater::noteRead(const Rock::HeaderUpdater::IoCbParams result) { - debugs(47, 7, result); - if (!result) { // EOF + debugs(47, 7, result.size); + if (!result.size) { // EOF stopReading("eof"); } else { - Must(result > 0); - bytesRead += result; - readerBuffer.forceSize(readerBuffer.length() + result); + Must(result.size > 0); + bytesRead += result.size; + readerBuffer.rawAppendFinish(result.buf, result.size); exchangeBuffer.append(readerBuffer); debugs(47, 7, "accumulated " << exchangeBuffer.length()); } @@ -130,7 +131,7 @@ Rock::HeaderUpdater::readMore(const char *why) Must(reader); readerBuffer.clear(); storeRead(reader, - readerBuffer.rawSpace(store->slotSize), + readerBuffer.rawAppendStart(store->slotSize), store->slotSize, bytesRead, &NoteRead, diff --git a/src/fs/rock/RockHeaderUpdater.h b/src/fs/rock/RockHeaderUpdater.h index 33571ef4e9..cb4ebc9e99 100644 --- a/src/fs/rock/RockHeaderUpdater.h +++ b/src/fs/rock/RockHeaderUpdater.h @@ -28,6 +28,12 @@ class HeaderUpdater: public AsyncJob CBDATA_CHILD(HeaderUpdater); public: + class IoCbParams { + public: + IoCbParams(const char *aBuf, ssize_t aSize) : buf(aBuf), size(aSize) {} + const char *buf; + ssize_t size; + }; HeaderUpdater(const Rock::SwapDir::Pointer &aStore, const Ipc::StoreMapUpdate &update); virtual ~HeaderUpdater() override = default; @@ -45,7 +51,7 @@ private: void startReading(); void stopReading(const char *why); void readMore(const char *why); - void noteRead(ssize_t result); + void noteRead(const IoCbParams result); void noteDoneReading(int errflag); void parseReadBytes(); @@ -67,6 +73,13 @@ private: SlotId staleSplicingPointNext; ///< non-updatable old HTTP body suffix start }; +inline +std::ostream &operator <<(std::ostream &os, const HeaderUpdater::IoCbParams ¶ms) +{ + os << static_cast(params.buf) << "," << params.size; + return os; +} + } // namespace Rock #endif /* SQUID_FS_ROCK_HEADER_UPDATER_H */ diff --git a/src/sbuf/SBuf.cc b/src/sbuf/SBuf.cc index 2cc50f1cfb..42d21e81bb 100644 --- a/src/sbuf/SBuf.cc +++ b/src/sbuf/SBuf.cc @@ -143,6 +143,28 @@ SBuf::reserve(const SBufReservationRequirements &req) return spaceSize(); // reallocated and probably reserved enough space } +char * +SBuf::rawAppendStart(size_type anticipatedSize) +{ + char *space = rawSpace(anticipatedSize); + debugs(24, 8, id << " start appending up to " << anticipatedSize << " bytes"); + return space; +} + +void +SBuf::rawAppendFinish(const char *start, size_type actualSize) +{ + Must(bufEnd() == start); + Must(store_->canAppend(off_ + len_, actualSize)); + debugs(24, 8, id << " finish appending " << actualSize << " bytes"); + + size_type newSize = length() + actualSize; + if (newSize > min(maxSize,store_->capacity-off_)) + throw SBufTooBigException(__FILE__,__LINE__); + len_ = newSize; + store_->size = off_ + newSize; +} + char * SBuf::rawSpace(size_type minSpace) { @@ -520,18 +542,6 @@ SBuf::rawContent() const return buf(); } -void -SBuf::forceSize(size_type newSize) -{ - debugs(24, 8, id << " force " << (newSize > length() ? "grow" : "shrink") << " to length=" << newSize); - - Must(store_->LockCount() == 1); - if (newSize > min(maxSize,store_->capacity-off_)) - throw SBufTooBigException(__FILE__,__LINE__); - len_ = newSize; - store_->size = newSize; -} - const char* SBuf::c_str() { diff --git a/src/sbuf/SBuf.h b/src/sbuf/SBuf.h index 1cc4658e7c..d41dc8faff 100644 --- a/src/sbuf/SBuf.h +++ b/src/sbuf/SBuf.h @@ -364,24 +364,18 @@ public: */ const char* rawContent() const; - /** Exports a writable pointer to the SBuf internal storage. - * \warning Use with EXTREME caution, this is a dangerous operation. - * - * Returns a pointer to the first unused byte in the SBuf's storage, - * which can be be used for appending. At least minSize bytes will - * be available for writing. - * The returned pointer must not be stored by the caller, as it will - * be invalidated by the first call to a non-const method call - * on the SBuf. - * This call guarantees to never return NULL. - * \see reserveSpace - * \note Unlike reserveSpace(), this method does not guarantee exclusive - * buffer ownership. It is instead optimized for a one writer - * (appender), many readers scenario by avoiding unnecessary - * copying and allocations. - * \throw SBufTooBigException if the user tries to allocate too big a SBuf - */ - char *rawSpace(size_type minSize); + /// \returns a buffer suitable for appending at most `anticipatedSize` bytes + /// The buffer must be used "immediately" because it is invalidated by most + /// non-constant SBuf method calls, including such calls against other SBuf + /// objects that just happen to share the same underlying MemBlob storage! + char *rawAppendStart(size_type anticipatedSize); + + /// Updates SBuf metadata to reflect appending `actualSize` bytes to the + /// buffer returned by the corresponding rawAppendStart() call. Throws if + /// rawAppendStart(actualSize) would have returned a different value now. + /// \param start raw buffer previously returned by rawAppendStart() + /// \param actualSize the number of appended bytes + void rawAppendFinish(const char *start, size_type actualSize); /** Obtain how much free space is available in the backing store. * @@ -390,16 +384,6 @@ public: */ size_type spaceSize() const { return store_->spaceSize(); } - /** Force a SBuf's size - * \warning use with EXTREME caution, this is a dangerous operation - * - * Adapt the SBuf internal state after external interference - * such as writing into it via rawSpace. - * \throw TextException if SBuf doesn't have exclusive ownership of store - * \throw SBufTooBigException if new size is bigger than available store space - */ - void forceSize(size_type newSize); - /** exports a null-terminated reference to the SBuf internal storage. * \warning ACCESSING RAW STORAGE IS DANGEROUS! DO NOT EVER USE * THE RETURNED POINTER FOR WRITING @@ -672,6 +656,25 @@ private: void checkAccessBounds(size_type pos) const; + /** Exports a writable pointer to the SBuf internal storage. + * \warning Use with EXTREME caution, this is a dangerous operation. + * + * Returns a pointer to the first unused byte in the SBuf's storage, + * which can be be used for appending. At least minSize bytes will + * be available for writing. + * The returned pointer must not be stored by the caller, as it will + * be invalidated by the first call to a non-const method call + * on the SBuf. + * This call guarantees to never return nullptr. + * \see reserveSpace + * \note Unlike reserveSpace(), this method does not guarantee exclusive + * buffer ownership. It is instead optimized for a one writer + * (appender), many readers scenario by avoiding unnecessary + * copying and allocations. + * \throw SBufTooBigException if the user tries to allocate too big a SBuf + */ + char *rawSpace(size_type minSize); + /** Low-level append operation * * Takes as input a contiguous area of memory and appends its contents diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc index de03520c3c..50a3a2e28e 100644 --- a/src/ssl/bio.cc +++ b/src/ssl/bio.cc @@ -336,12 +336,12 @@ Ssl::ServerBio::readAndParse(char *buf, const int size, BIO *table) int Ssl::ServerBio::readAndBuffer(BIO *table) { - char *space = rbuf.rawSpace(SQUID_TCP_SO_RCVBUF); - const int result = Ssl::Bio::read(space, rbuf.spaceSize(), table); + char *space = rbuf.rawAppendStart(SQUID_TCP_SO_RCVBUF); + const int result = Ssl::Bio::read(space, SQUID_TCP_SO_RCVBUF, table); if (result <= 0) return result; - rbuf.forceSize(rbuf.length() + result); + rbuf.rawAppendFinish(space, result); return result; } diff --git a/src/tests/stub_SBuf.cc b/src/tests/stub_SBuf.cc index 9884ccb551..5030dc39f7 100644 --- a/src/tests/stub_SBuf.cc +++ b/src/tests/stub_SBuf.cc @@ -49,8 +49,8 @@ SBuf SBuf::consume(size_type n) STUB_RETVAL(*this) const SBufStats& SBuf::GetStats() STUB_RETVAL(SBuf::stats) SBuf::size_type SBuf::copy(char *dest, size_type n) const STUB_RETVAL(0) const char* SBuf::rawContent() const STUB_RETVAL(NULL) -char *SBuf::rawSpace(size_type minSize) STUB_RETVAL(NULL) -void SBuf::forceSize(size_type newSize) STUB +char *SBuf::rawAppendStart(size_type) STUB_RETVAL(NULL) +void SBuf::rawAppendFinish(const char *, size_type) STUB const char* SBuf::c_str() STUB_RETVAL("") void SBuf::reserveCapacity(size_type minCapacity) STUB SBuf::size_type SBuf::reserve(const SBufReservationRequirements &) STUB_RETVAL(0) diff --git a/src/tests/testSBuf.cc b/src/tests/testSBuf.cc index a91467039d..c402152732 100644 --- a/src/tests/testSBuf.cc +++ b/src/tests/testSBuf.cc @@ -418,10 +418,9 @@ testSBuf::testRawSpace() { SBuf s1(literal); SBuf s2(fox1); - SBuf::size_type sz=s2.length(); - char *rb=s2.rawSpace(strlen(fox2)+1); + char *rb=s2.rawAppendStart(strlen(fox2)+1); strcpy(rb,fox2); - s2.forceSize(sz+strlen(fox2)); + s2.rawAppendFinish(rb, strlen(fox2)); CPPUNIT_ASSERT_EQUAL(s1,s2); }