From f1967cefa9c0468b14fef417ff1e50671fd72ed0 Mon Sep 17 00:00:00 2001 From: Jeremy Zoss Date: Wed, 1 Jul 2015 11:48:20 -0400 Subject: [PATCH] Switch ByteArray to for internal buffer - enables dynamic sizing, for larger messages * up to INT_MAX, which is unadvised - allows efficient data access at front/rear of msgs - maintains same external API * getRawDataPtr() is deprecated (not contiguous memory) - bugFix: SimpleSocket::receiveBytes() buffer-size check - update unit tests to handle ByteArray.MaxSize==INT_MAX --- .../include/simple_message/byte_array.h | 108 +++------ simple_message/src/byte_array.cpp | 207 +++++------------- simple_message/src/simple_message.cpp | 2 +- simple_message/src/socket/simple_socket.cpp | 13 +- simple_message/src/socket/udp_client.cpp | 8 +- simple_message/src/socket/udp_server.cpp | 8 +- simple_message/test/utest.cpp | 39 ++-- 7 files changed, 132 insertions(+), 253 deletions(-) diff --git a/simple_message/include/simple_message/byte_array.h b/simple_message/include/simple_message/byte_array.h index fd53cd7c..622fd3a8 100755 --- a/simple_message/include/simple_message/byte_array.h +++ b/simple_message/include/simple_message/byte_array.h @@ -1,7 +1,7 @@ /* * Software License Agreement (BSD License) * - * Copyright (c) 2011, Southwest Research Institute + * Copyright (c) 2015, Southwest Research Institute * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -38,6 +38,8 @@ #include "shared_types.h" #endif +#include +#include #include "string.h" namespace industrial @@ -55,7 +57,7 @@ namespace byte_array { /** - * \brief The byte array wraps a traditional, fixed size, array of bytes (i.e. char*). + * \brief The byte array wraps a dynamic array of bytes (i.e. char). * * It provides convenient methods for loading and unloading * data types to and from the byte array. The class acts as an @@ -63,12 +65,13 @@ namespace byte_array * raw data changes). It's intended use is for socket communications. * * By default data using the load/unload methods is appended/removed from the end - * of the array. As long as the standard load/unload methods are uses, this is + * of the array. Methods are also provided to load/unload from the front of the + * array. As long as the matching load/unload methods are used, this is * transparent to the user. * - * A fixed size array is used for simplicity (i.e. avoiding re-implementing - * the STL vector class for those systems that don't have access to the STL, - * i.e. Motoman robotics Motoplus compiler. + * The internals of ByteArray have been updated to use a dynamically-allocated + * STL class, for safety and performance reasons. This may limit cross-platform + * usage of this class (e.g. in the MotoPlus compiler). * * THIS CLASS IS NOT THREAD-SAFE * @@ -98,7 +101,7 @@ class ByteArray /** * \brief Initializes or Reinitializes an empty buffer. * - * This method sets all values to 0 and resets the buffer size. + * This method resets the buffer size to zero (empty). * */ void init(); @@ -110,9 +113,7 @@ class ByteArray * in buffer (up to byteSize) * * \param buffer pointer to byte buffer - * \param byte_size size of buffer to copy. If the byte size is greater - * than the max buffer size then only the max buffer amount of bytes is - * copied + * \param byte_size size of buffer to copy. * * \return true on success, false otherwise (max array size exceeded). */ @@ -129,6 +130,16 @@ class ByteArray */ void copyFrom(ByteArray & buffer); + /** + * \brief Copy to std::vector, for raw-ptr access + * + * This method copies the ByteArray data to a std::vector. + * + * \param out vector to copy into + * + */ + void copyTo(std::vector & out); + /** * \brief loads a boolean into the byte array * @@ -257,8 +268,6 @@ class ByteArray /** * \brief unloads a double value from the beginning of the byte array. * If byte swapping is enabled, then the bytes are swapped. - * WARNING: This method performs a memmove every time it is called (this is - * expensive). * * \param value value to unload * @@ -269,8 +278,6 @@ class ByteArray /** * \brief unloads an integer value from the beginning of the byte array. * If byte swapping is enabled, then the bytes are swapped - * WARNING: This method performs a memmove every time it is called (this is - * expensive). * * \param value value to unload * @@ -280,8 +287,6 @@ class ByteArray /** * \brief unloads a void* (treated as char*) from the beginning of the array. - * WARNING: This method performs a memmove every time it is called (this is - * expensive). * WARNING: Byte swapping is not performed in this function. * * \param value to unload @@ -295,14 +300,12 @@ class ByteArray * \brief returns a char* pointer to the raw data. * WARNING: This method is meant for read-only operations * - * This function returns a pointer to the actual raw data stored within - * the class. Care should be taken not to modified the data oustide of - * the class. This method of providing a reference to private class data - * is used in order to avoid dynamic memory allocation that would be required - * to return a copy. + * \deprecated This is unsafe with dynamic buffer sizing. + * Use copyTo(vector) instead. * * \return char* pointer to the raw data */ + __attribute__((deprecated("This ptr will be invalid once buffer is changed. Please use: copyTo(vector) instead."))) char* getRawDataPtr(); /** @@ -329,25 +332,16 @@ class ByteArray static bool isByteSwapEnabled(); private: - /** - * \brief maximum array size (WARNING: THIS VALUE SHOULD NOT EXCEED THE MAX - * 32-BIT INTEGER SIZE) - * - * The byte array class uses shared 32-bit types, so the buffer size cannot - * be larger than this. Ideally this value would be relatively small as passing - * large amounts of data is not desired. - */ - static const industrial::shared_types::shared_int MAX_SIZE = 1024; /** * \brief internal data buffer */ - char buffer_[MAX_SIZE]; - + std::deque buffer_; + /** - * \brief current buffer size + * \brief temporary continuous buffer for getRawDataPtr() use */ - industrial::shared_types::shared_int buffer_size_; + std::vector getRawDataPtr_buffer_; #ifdef BYTE_SWAPPING /** @@ -360,52 +354,6 @@ class ByteArray void swap(void *value, industrial::shared_types::shared_int byteSize); #endif - /** - * \brief sets current buffer size - * - * \param size new size - * - * \return true on success, false otherwise (new size is too large) - */ - bool setBufferSize(const industrial::shared_types::shared_int size); - - /** - * \brief extends current buffer size - * - * \param number of bytes to extend - * - * \return true on success, false otherwise (new size is too large) - */ - bool extendBufferSize(const industrial::shared_types::shared_int size); - - /** - * \brief shortens current buffer size - * - * \param number of bytes to shorten - * - * \return true on success, false otherwise (new size is less than 0) - */ - bool shortenBufferSize(industrial::shared_types::shared_int size); - - /** - * \brief gets pointer to unload location (buffer_size + 1) - * - * \param size new size - * - * \return pointer to load location (NULL if array is at max size) - */ - char* getLoadPtr(); - - /** - * \brief gets pointer to unload location (buffer_size - num_bytes) - * - * The unload location is the beginning of the data item that is to - * be unloaded. - * - * \return pointer to load location (NULL is num_bytes > buffer_size_) - */ - char* getUnloadPtr(const industrial::shared_types::shared_int num_bytes); - }; } // namespace industrial diff --git a/simple_message/src/byte_array.cpp b/simple_message/src/byte_array.cpp index a79ada6f..e9a1bd72 100644 --- a/simple_message/src/byte_array.cpp +++ b/simple_message/src/byte_array.cpp @@ -1,7 +1,7 @@ /* * Software License Agreement (BSD License) * - * Copyright (c) 2011, Southwest Research Institute + * Copyright (c) 2015, Southwest Research Institute * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -38,12 +38,6 @@ #include "log_wrapper.h" #endif -#ifdef MOTOPLUS -#include "motoPlus.h" -#endif - -#include "string.h" - namespace industrial { namespace byte_array @@ -67,15 +61,14 @@ ByteArray::~ByteArray(void) void ByteArray::init() { - memset(&(buffer_[0]), 0, this->MAX_SIZE); - this->setBufferSize(0); + this->buffer_.clear(); } bool ByteArray::init(const char* buffer, const shared_int byte_size) { bool rtn; - if (this->MAX_SIZE >= byte_size) + if (this->getMaxBufferSize() >= byte_size) { LOG_COMM("Initializing buffer to size: %d", byte_size); this->load((void*)buffer, byte_size); @@ -94,8 +87,7 @@ void ByteArray::copyFrom(ByteArray & buffer) { if (buffer.getBufferSize() != 0) { - this->setBufferSize(buffer.getBufferSize()); - memcpy(this->getRawDataPtr(), buffer.getRawDataPtr(), this->buffer_size_); + this->buffer_ = buffer.buffer_; } else { @@ -103,6 +95,11 @@ void ByteArray::copyFrom(ByteArray & buffer) } } +void ByteArray::copyTo(std::vector &out) +{ + out.assign(buffer_.begin(), buffer_.end()); +} + #ifdef BYTE_SWAPPING void ByteArray::swap(void *value, shared_int byteSize) @@ -132,7 +129,8 @@ void ByteArray::swap(void *value, shared_int byteSize) char* ByteArray::getRawDataPtr() { - return &this->buffer_[0]; + this->copyTo( this->getRawDataPtr_buffer_ ); + return &getRawDataPtr_buffer_[0]; } /**************************************************************** @@ -177,15 +175,23 @@ bool ByteArray::load(simple_serialize::SimpleSerialize &value) bool ByteArray::load(ByteArray &value) { LOG_COMM("Executing byte array load through byte array"); - return this->load(value.getRawDataPtr(), value.getBufferSize()); + std::deque& src = value.buffer_; + std::deque& dest = this->buffer_; + + if (this->getBufferSize()+value.getBufferSize() > this->getMaxBufferSize()) + { + LOG_ERROR("Additional data would exceed buffer size"); + return false; + } + + dest.insert(dest.end(), src.begin(), src.end()); + return true; } bool ByteArray::load(void* value, const shared_int byte_size) { bool rtn; - // Get the load pointer before extending the buffer. - char* loadPtr; LOG_COMM("Executing byte array load through void*, size: %d", byte_size); // Check inputs @@ -194,15 +200,20 @@ bool ByteArray::load(void* value, const shared_int byte_size) LOG_ERROR("NULL point passed into load method"); return false; } + if (this->getBufferSize()+byte_size > this->getMaxBufferSize()) + { + LOG_ERROR("Additional data would exceed buffer size"); + return false; + } - loadPtr = this->getLoadPtr(); - - if (this->extendBufferSize(byte_size)) + try { - memcpy(loadPtr, value, byte_size); + char* bytePtr = (char*)value; + this->buffer_.insert(this->buffer_.end(), bytePtr, bytePtr + byte_size); + rtn = true; } - else + catch (std::exception) { LOG_ERROR("Failed to load byte array"); rtn = false; @@ -257,29 +268,24 @@ bool ByteArray::unload(simple_serialize::SimpleSerialize &value) return value.unload(this); } + bool ByteArray::unload(ByteArray &value, const shared_int byte_size) { LOG_COMM("Executing byte array unload through byte array"); - char* unloadPtr = this->getUnloadPtr(byte_size); bool rtn; - if (NULL != unloadPtr) + if (byte_size <= this->getBufferSize()) { - if (this->shortenBufferSize(byte_size)) - { + std::deque& src = this->buffer_; + std::deque& dest = value.buffer_; - rtn = value.load(unloadPtr, byte_size); - rtn = true; - } - else - { - LOG_ERROR("Failed to shorten array"); - rtn = false; - } + dest.insert(dest.end(), src.end()-byte_size, src.end()); + src.erase(src.end()-byte_size, src.end()); + rtn = true; } else { - LOG_ERROR("Unload pointer returned NULL"); + LOG_ERROR("Buffer smaller than requested size."); rtn = false; } @@ -289,7 +295,6 @@ bool ByteArray::unload(ByteArray &value, const shared_int byte_size) bool ByteArray::unload(void* value, shared_int byteSize) { bool rtn; - char* unloadPtr; LOG_COMM("Executing byte array unload through void*, size: %d", byteSize); // Check inputs @@ -299,25 +304,17 @@ bool ByteArray::unload(void* value, shared_int byteSize) return false; } - unloadPtr = this->getUnloadPtr(byteSize); - - if (NULL != unloadPtr) + if (byteSize <= this->getBufferSize()) { + std::deque& src = this->buffer_; - if (this->shortenBufferSize(byteSize)) - { - memcpy(value, unloadPtr, byteSize); + std::copy(src.end()-byteSize, src.end(), (char*)value); + src.erase(src.end()-byteSize, src.end()); rtn = true; - } - else - { - LOG_ERROR("Failed to shorten array"); - rtn = false; - } } else { - LOG_ERROR("Unload pointer returned NULL"); + LOG_ERROR("Buffer is smaller than requested byteSize."); rtn = false; } @@ -357,46 +354,30 @@ bool ByteArray::unloadFront(industrial::shared_types::shared_int &value) #endif return rtn; } + bool ByteArray::unloadFront(void* value, const industrial::shared_types::shared_int byteSize) { bool rtn; - char* unloadPtr = NULL; - char* nextPtr = NULL; - shared_int sizeRemain; + LOG_COMM("Executing byte array unloadFront through void*, size: %d", byteSize); // Check inputs if (NULL == value) { - LOG_ERROR("NULL point passed into unload method"); + LOG_ERROR("NULL point passed into unloadFront method"); return false; } - unloadPtr = &this->buffer_[0]; - - if (NULL != unloadPtr) + if (byteSize <= this->getBufferSize()) { - nextPtr = unloadPtr + byteSize; - sizeRemain = this->getBufferSize() - byteSize; - - LOG_DEBUG("Unloading: %d bytes, %d bytes remain", byteSize, sizeRemain); - if (this->shortenBufferSize(byteSize)) - { - LOG_COMM("Preparing to copy value"); - memcpy(value, unloadPtr, byteSize); - LOG_COMM("Value is unloaded, performing move"); - memmove(unloadPtr, nextPtr, sizeRemain); - LOG_COMM("Move operation completed"); + std::deque& src = this->buffer_; + + std::copy(src.begin(), src.begin()+byteSize, (char*)value); + src.erase(src.begin(), src.begin()+byteSize); rtn = true; - } - else - { - LOG_ERROR("Failed to shorten array"); - rtn = false; - } } else { - LOG_ERROR("Unload pointer returned NULL"); + LOG_ERROR("Buffer is smaller than requested byteSize."); rtn = false; } @@ -405,12 +386,12 @@ bool ByteArray::unloadFront(void* value, const industrial::shared_types::shared_ unsigned int ByteArray::getBufferSize() { - return this->buffer_size_; + return this->buffer_.size(); } unsigned int ByteArray::getMaxBufferSize() { - return this->MAX_SIZE; + return this->buffer_.max_size(); } @@ -422,81 +403,5 @@ bool ByteArray::isByteSwapEnabled() return false; } -bool ByteArray::setBufferSize(shared_int size) -{ - bool rtn; - - if (this->MAX_SIZE >= size) - { - this->buffer_size_ = size; - rtn = true; - } - else - { - LOG_ERROR("Set buffer size: %u, larger than MAX:, %u", size, this->MAX_SIZE); - rtn = false; - } - - return rtn; - -} - -bool ByteArray::extendBufferSize(shared_int size) -{ - unsigned int newSize; - - newSize = this->getBufferSize() + size; - return this->setBufferSize(newSize); - -} - -bool ByteArray::shortenBufferSize(shared_int size) -{ - unsigned int newSize; - bool rtn; - - // If the buffer is not larger than the size it is shortened by - // we fail. This is checked here (as opposed to setBufferSize) - // because setBufferSize assumes a unsigned argument and therefore - // wouldn't catch a negative size. - if (size <= (shared_int)this->getBufferSize()) - { - newSize = this->getBufferSize() - size; - rtn = this->setBufferSize(newSize); - } - else - { - LOG_ERROR("Failed to shorten buffer by %u bytes, buffer too small, %u bytes", size, this->getBufferSize()); - rtn = false; - } - - return rtn; - -} - -char* ByteArray::getLoadPtr() -{ - - return &this->buffer_[this->buffer_size_]; -} - -char* ByteArray::getUnloadPtr(shared_int byteSize) -{ - char* rtn; - - if (byteSize <= (shared_int)this->getBufferSize()) - { - rtn = this->getLoadPtr() - byteSize; - } - else - { - LOG_ERROR("Get unload pointer failed, buffer size: %d, smaller than byte size: %d", - this->getBufferSize(), byteSize); - rtn = NULL; - } - - return rtn; -} - } // namespace byte_array } // namespace industrial diff --git a/simple_message/src/simple_message.cpp b/simple_message/src/simple_message.cpp index 083ff089..c80ee7c2 100755 --- a/simple_message/src/simple_message.cpp +++ b/simple_message/src/simple_message.cpp @@ -119,7 +119,7 @@ void SimpleMessage::toByteArray(ByteArray & msg) msg.load(this->getReplyCode()); if (this->data_.getBufferSize() > 0 ) { - msg.load(this->getData().getRawDataPtr(), this->data_.getBufferSize()); + msg.load(this->data_); } } diff --git a/simple_message/src/socket/simple_socket.cpp b/simple_message/src/socket/simple_socket.cpp index 83028aad..5c5dfd56 100644 --- a/simple_message/src/socket/simple_socket.cpp +++ b/simple_message/src/socket/simple_socket.cpp @@ -57,7 +57,11 @@ namespace industrial if (this->MAX_BUFFER_SIZE > (int)buffer.getBufferSize()) { - rc = rawSendBytes(buffer.getRawDataPtr(), buffer.getBufferSize()); + // copy to local array, since ByteArray no longer supports + // direct pointer-access to data values + std::vector localBuffer; + buffer.copyTo(localBuffer); + rc = rawSendBytes(&localBuffer[0], localBuffer.size()); if (this->SOCKET_FAIL != rc) { rtn = true; @@ -104,10 +108,9 @@ namespace industrial memset(&this->buffer_, 0, sizeof(this->buffer_)); - // Doing a sanity check to determine if the byte array buffer is larger than - // what can be sent in the socket. This should not happen and might be indicative - // of some code synchronization issues between the client and server base. - if (this->MAX_BUFFER_SIZE < (int)buffer.getMaxBufferSize()) + // Doing a sanity check to determine if the byte array buffer is smaller than + // what can be received by the socket. + if (this->MAX_BUFFER_SIZE > buffer.getMaxBufferSize()) { LOG_WARN("Socket buffer max size: %u, is larger than byte array buffer: %u", this->MAX_BUFFER_SIZE, buffer.getMaxBufferSize()); diff --git a/simple_message/src/socket/udp_client.cpp b/simple_message/src/socket/udp_client.cpp index e2806d4f..0d93d377 100644 --- a/simple_message/src/socket/udp_client.cpp +++ b/simple_message/src/socket/udp_client.cpp @@ -101,12 +101,18 @@ bool UdpClient::makeConnect() this->setConnected(false); send.load((void*)&sendHS, sizeof(sendHS)); + // copy to local array, since ByteArray no longer supports + // direct pointer-access to data values + const int sendLen = send.getBufferSize(); + char localBuffer[sendLen]; + send.unload(localBuffer, sendLen); + do { ByteArray recv; recvHS = 0; LOG_DEBUG("UDP client sending handshake"); - this->rawSendBytes(send.getRawDataPtr(), send.getBufferSize()); + this->rawSendBytes(localBuffer, sendLen); if (this->isReadyReceive(timeout)) { bytesRcvd = this->rawReceiveBytes(this->buffer_, 0); diff --git a/simple_message/src/socket/udp_server.cpp b/simple_message/src/socket/udp_server.cpp index 2b4d3258..f52eea87 100644 --- a/simple_message/src/socket/udp_server.cpp +++ b/simple_message/src/socket/udp_server.cpp @@ -142,8 +142,14 @@ bool UdpServer::makeConnect() } while(recvHS != sendHS); + // copy to local array, since ByteArray no longer supports + // direct pointer-access to data values + const int sendLen = send.getBufferSize(); + char localBuffer[sendLen]; + send.unload(localBuffer, sendLen); + // Send a reply handshake - this->rawSendBytes(send.getRawDataPtr(), send.getBufferSize()); + this->rawSendBytes(localBuffer, sendLen); this->setConnected(true); rtn = true; diff --git a/simple_message/test/utest.cpp b/simple_message/test/utest.cpp index dc15cf00..7c7f8a88 100644 --- a/simple_message/test/utest.cpp +++ b/simple_message/test/utest.cpp @@ -53,6 +53,7 @@ // threads //#include #include +#include using namespace industrial::simple_message; using namespace industrial::byte_array; @@ -85,9 +86,6 @@ TEST(ByteArraySuite, init) const shared_int SIZE = 100; ByteArray bytes; - shared_int TOO_BIG = bytes.getMaxBufferSize()+1; - - char bigBuffer[TOO_BIG]; char buffer[SIZE]; // Valid byte arrays @@ -95,8 +93,16 @@ TEST(ByteArraySuite, init) EXPECT_EQ((shared_int)bytes.getBufferSize(), SIZE); // Invalid init (too big) - // Invalid buffers - EXPECT_FALSE(bytes.init(&bigBuffer[0], TOO_BIG)); + if (bytes.getMaxBufferSize() < std::numeric_limits::max()) + { + shared_int TOO_BIG = bytes.getMaxBufferSize()+1; + char bigBuffer[TOO_BIG]; + EXPECT_FALSE(bytes.init(&bigBuffer[0], TOO_BIG)); + } + else + std::cout << std::string(15, ' ') + << "ByteArray.MaxSize==INT_MAX. Skipping TOO_BIG tests" << std::endl; + } TEST(ByteArraySuite, loading) @@ -219,11 +225,6 @@ TEST(ByteArraySuite, copy) // Copy ByteArray copyFrom; ByteArray copyTo; - ByteArray tooBig; - - - shared_int TOO_BIG = tooBig.getMaxBufferSize()-1; - char bigBuffer[TOO_BIG]; EXPECT_TRUE(copyFrom.init(&buffer[0], SIZE)); EXPECT_TRUE(copyTo.load(copyFrom)); @@ -232,10 +233,20 @@ TEST(ByteArraySuite, copy) EXPECT_EQ((shared_int)copyTo.getBufferSize(), 2*SIZE); // Copy too large - EXPECT_TRUE(tooBig.init(&bigBuffer[0], TOO_BIG)); - EXPECT_FALSE(copyTo.load(tooBig)); - // A failed load should not change the buffer. - EXPECT_EQ((shared_int)copyTo.getBufferSize(), 2*SIZE); + ByteArray tooBig; + if (tooBig.getMaxBufferSize()-1 <= std::numeric_limits::max()) + { + shared_int TOO_BIG = tooBig.getMaxBufferSize()-1; + char bigBuffer[TOO_BIG]; + + EXPECT_TRUE(tooBig.init(&bigBuffer[0], TOO_BIG)); + EXPECT_FALSE(copyTo.load(tooBig)); + // A failed load should not change the buffer. + EXPECT_EQ((shared_int)copyTo.getBufferSize(), 2*SIZE); + } + else + std::cout << std::string(15, ' ') + << "ByteArray.MaxSize==INT_MAX. Skipping TOO_BIG tests" << std::endl; } // Need access to protected members for testing