From f7baf549bcea8793f7882eb7e01cb3affaea97b8 Mon Sep 17 00:00:00 2001 From: David Reiss Date: Mon, 4 Feb 2008 21:56:27 +0000 Subject: [PATCH] Thrift: Revamp TMemoryBuffer constructors. Summary: There were some weird cases where the implicit conversion from const char* to std::string was causing the wrong constructor to be called. There wasn't really a clean workaround, so we're dropping the string constructors. Reviewed By: mcslee Test Plan: Ran the test. Grepped around the /projects tree for uses that had to fixed, and fixed them. Revert Plan: ok Other Notes: This risk was pointed out by Ben Maurer. git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@665461 13f79535-47bb-0310-9956-ffa450edef68 --- lib/cpp/src/transport/TTransportUtils.h | 136 +++++++++++++++++------- test/TMemoryBufferTest.cpp | 32 +++++- 2 files changed, 126 insertions(+), 42 deletions(-) diff --git a/lib/cpp/src/transport/TTransportUtils.h b/lib/cpp/src/transport/TTransportUtils.h index 6aceb824..0aa29c99 100644 --- a/lib/cpp/src/transport/TTransportUtils.h +++ b/lib/cpp/src/transport/TTransportUtils.h @@ -292,6 +292,7 @@ class TFramedTransportFactory : public TTransportFactory { * doubles as necessary. * * @author Mark Slee + * @author David Reiss */ class TMemoryBuffer : public TTransport { private: @@ -316,30 +317,78 @@ class TMemoryBuffer : public TTransport { public: static const uint32_t defaultSize = 1024; + /** + * This enum specifies how a TMemoryBuffer should treat + * memory passed to it via constructors or resetBuffer. + * + * OBSERVE: + * TMemoryBuffer will simply store a pointer to the memory. + * It is the callers responsibility to ensure that the pointer + * remains valid for the lifetime of the TMemoryBuffer, + * and that it is properly cleaned up. + * Note that no data can be written to observed buffers. + * + * COPY: + * TMemoryBuffer will make an internal copy of the buffer. + * The caller has no responsibilities. + * + * TAKE_OWNERSHIP: + * TMemoryBuffer will become the "owner" of the buffer, + * and will be responsible for freeing it. + * The membory must have been allocated with malloc. + */ + enum MemoryPolicy { + OBSERVE = 1, + COPY = 2, + TAKE_OWNERSHIP = 3, + }; + + /** + * Construct a TMemoryBuffer with a default-sized buffer, + * owned by the TMemoryBuffer object. + */ TMemoryBuffer() { initCommon(NULL, defaultSize, true, 0); } + /** + * Construct a TMemoryBuffer with a buffer of a specified size, + * owned by the TMemoryBuffer object. + * + * @param sz The initial size of the buffer. + */ TMemoryBuffer(uint32_t sz) { initCommon(NULL, sz, true, 0); } - // transferOwnership should be true if you want TMemoryBuffer to call free(buf). - TMemoryBuffer(uint8_t* buf, int sz, bool transferOwnership = false) { - initCommon(buf, sz, transferOwnership, sz); - } + /** + * Construct a TMemoryBuffer with buf as its initial contents. + * + * @param buf The initial contents of the buffer. + * Note that, while buf is a non-const pointer, + * TMemoryBuffer will not write to it if policy == OBSERVE, + * so it is safe to const_cast(whatever). + * @param sz The size of @c buf. + * @param policy See @link MemoryPolicy @endlink . + */ + TMemoryBuffer(uint8_t* buf, uint32_t sz, MemoryPolicy policy = OBSERVE) { + if (buf == NULL && sz != 0) { + throw TTransportException(TTransportException::BAD_ARGS, + "TMemoryBuffer given null buffer with non-zero size."); + } - // copy should be true if you want TMemoryBuffer to make a copy of the string. - // If you set copy to false, the string must not be destroyed before you are - // done with the TMemoryBuffer. - TMemoryBuffer(const std::string& str, bool copy = false) { - if (copy) { - initCommon(NULL, str.length(), true, 0); - this->write(reinterpret_cast(str.data()), str.length()); - } else { - // This first argument should be equivalent to the following: - // const_cast(reinterpret_cast(str.data())) - initCommon((uint8_t*)str.data(), str.length(), false, str.length()); + switch (policy) { + case OBSERVE: + case TAKE_OWNERSHIP: + initCommon(buf, sz, policy == TAKE_OWNERSHIP, sz); + break; + case COPY: + initCommon(NULL, sz, true, 0); + this->write(buf, sz); + break; + default: + throw TTransportException(TTransportException::BAD_ARGS, + "Invalid MemoryPolicy for TMemoryBuffer"); } } @@ -362,6 +411,7 @@ class TMemoryBuffer : public TTransport { void close() {} + // TODO(dreiss): Make bufPtr const. void getBuffer(uint8_t** bufPtr, uint32_t* sz) { *bufPtr = buffer_; *sz = wPos_; @@ -384,33 +434,31 @@ class TMemoryBuffer : public TTransport { void resetBuffer() { wPos_ = 0; rPos_ = 0; - } - - // transferOwnership should be true if you want TMemoryBuffer to call free(buf). - void resetBuffer(uint8_t* buf, uint32_t sz, bool transferOwnership = false) { - if (owner_) { - free(buffer_); + // It isn't safe to write into a buffer we don't own. + if (!owner_) { + bufferSize_ = 0; } - owner_ = transferOwnership; - buffer_ = buf; - bufferSize_ = sz; - wPos_ = sz; - rPos_ = 0; } - // See the constructor that takes a string. - void resetFromString(const std::string& str, bool copy = false) { - if (copy) { - uint8_t* buf = (uint8_t*)malloc(str.length()); - if (buf == NULL) { - throw TTransportException("Out of memory"); - } - std::copy(str.begin(), str.end(), buf); - resetBuffer(buf, str.length(), true); - } else { - // See the above comment about const_cast. - resetBuffer((uint8_t*)str.data(), str.length(), false); - } + /// See constructor documentation. + void resetBuffer(uint8_t* buf, uint32_t sz, MemoryPolicy policy = OBSERVE) { + // Use a variant of the copy-and-swap trick for assignment operators. + // This is sub-optimal in terms of performance for two reasons: + // 1/ The constructing and swapping of the (small) values + // in the temporary object takes some time, and is not necessary. + // 2/ If policy == COPY, we allocate the new buffer before + // freeing the old one, precluding the possibility of + // reusing that memory. + // I doubt that either of these problems could be optimized away, + // but the second is probably no a common case, and the first is minor. + // I don't expect resetBuffer to be a common operation, so I'm willing to + // bite the performance bullet to make the method this simple. + + // Construct the new buffer. + TMemoryBuffer new_buffer(buf, sz, policy); + // Move it into ourself. + this->swap(new_buffer); + // Our old self gets destroyed. } uint32_t read(uint8_t* buf, uint32_t len); @@ -439,6 +487,14 @@ class TMemoryBuffer : public TTransport { void consume(uint32_t len); + void swap(TMemoryBuffer& that) { + using std::swap; + swap(buffer_, that.buffer_); + swap(bufferSize_, that.bufferSize_); + swap(wPos_, that.wPos_); + swap(owner_, that.owner_); + } + private: // Data buffer uint8_t* buffer_; @@ -455,6 +511,8 @@ class TMemoryBuffer : public TTransport { // Is this object the owner of the buffer? bool owner_; + // Don't forget to update constrctors, initCommon, and swap if + // you add new members. }; /** diff --git a/test/TMemoryBufferTest.cpp b/test/TMemoryBufferTest.cpp index a8975e1e..2b2f90c0 100644 --- a/test/TMemoryBufferTest.cpp +++ b/test/TMemoryBufferTest.cpp @@ -34,7 +34,7 @@ int main(int argc, char** argv) { shared_ptr strBuffer2(new TMemoryBuffer()); shared_ptr binaryProtcol2(new TBinaryProtocol(strBuffer2)); - strBuffer2->resetFromString(serialized); + strBuffer2->resetBuffer((uint8_t*)serialized.data(), serialized.length()); thrift::test::Xtruct a2; a2.read(binaryProtcol2.get()); @@ -46,10 +46,10 @@ int main(int argc, char** argv) { using std::string; using std::cout; using std::endl; - + string* str1 = new string("abcd1234"); const char* data1 = str1->data(); - TMemoryBuffer buf(*str1, true); + TMemoryBuffer buf((uint8_t*)str1->data(), str1->length(), TMemoryBuffer::COPY); delete str1; string* str2 = new string("plsreuse"); bool obj_reuse = (str1 == str2); @@ -65,4 +65,30 @@ int main(int argc, char** argv) { assert(str3 == "wxyzabcd"); assert(str4 == "67891234"); } + + { + using facebook::thrift::transport::TTransportException; + using facebook::thrift::transport::TMemoryBuffer; + using std::string; + + char data[] = "foo\0bar"; + + TMemoryBuffer buf1((uint8_t*)data, 7, TMemoryBuffer::OBSERVE); + string str = buf1.getBufferAsString(); + assert(str.length() == 7); + buf1.resetBuffer(); + try { + buf1.write((const uint8_t*)"foo", 3); + assert(false); + } catch (TTransportException& ex) {} + + TMemoryBuffer buf2((uint8_t*)data, 7, TMemoryBuffer::COPY); + try { + buf2.write((const uint8_t*)"bar", 3); + } catch (TTransportException& ex) { + assert(false); + } + } + + } -- 2.17.1