Optimize, fix memory zeroing and refactor BinaryQueue 70/240670/6
authorMateusz Cegielka <m.cegielka@samsung.com>
Mon, 10 Aug 2020 11:24:35 +0000 (13:24 +0200)
committerKrzysztof Jackiewicz <k.jackiewicz@samsung.com>
Wed, 2 Sep 2020 08:54:48 +0000 (08:54 +0000)
BinaryQueue is a class responsible for buffering data received from
sockets before deserialization, vendored from DPL. It stores the
received data as a list of blocks, which is probably the optimal
approach given the constraints of the services framework here. However,
its implementation is a little inefficient and incorrect:

- Stores data in std::vector<unsigned char> instead of RawBuffer.
  Because of that, any piece of data that passes through a socket may
  live in memory much longer than it should.
- Erases elements from the front of a std::vector. This means all the
  other elements need to be shifted, which could even result in
  quadratic complexity given large enough socket reads and small enough
  messages.
- Always copies incoming data. This means all of incoming traffic has to
  be copied one more time than it needs to.

I have fixed the first issue in the obvious way. To fix the second
issue, I have added a new member that tracks how many bytes have been
read from the first bucket in the queue, which makes physically erasing
elements from the vector unnecessary. Lastly, I changed the push
signature from taking a pointer and a size to taking a RawBuffer&&,
which eliminated some copies and made the remaining ones more explicit.

Change-Id: I36932d5492815e38bf1cdab249327d26c9805ac6

13 files changed:
misc/ckm_db_tool/db-wrapper.cpp
src/manager/client-async/service.cpp
src/manager/client/client-common.cpp
src/manager/common/message-buffer.cpp
src/manager/common/message-buffer.h
src/manager/crypto/sw-backend/store.cpp
src/manager/crypto/tz-backend/store.cpp
src/manager/dpl/core/include/dpl/binary_queue.h
src/manager/dpl/core/src/binary_queue.cpp
src/manager/main/thread-service.cpp
src/manager/service/encryption-service.cpp
unit-tests/test_binary-queue.cpp
unit-tests/test_serialization.cpp

index 4f9498b..427fce1 100644 (file)
@@ -59,9 +59,8 @@ int DbWrapper::unlock()
                return m_logic.unlockSystemDB();
 
        int retCode;
-       RawBuffer ret = m_logic.unlockUserKey(m_uid, m_pw);
        MessageBuffer buff;
-       buff.Push(ret);
+       buff.Push(m_logic.unlockUserKey(m_uid, m_pw));
        buff.Deserialize(retCode);
        return retCode;
 }
index cd7cd19..ceeeeae 100644 (file)
@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2000 - 2015 Samsung Electronics Co., Ltd All Rights Reserved
+ *  Copyright (c) 2000 - 2020 Samsung Electronics Co., Ltd All Rights Reserved
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -183,8 +183,7 @@ void Service::receiveData()
        if (!m_responseBuffer)
                m_responseBuffer.reset(new MessageBuffer());
 
-       RawBuffer raw(buffer, buffer + temp);
-       m_responseBuffer->Push(raw);
+       m_responseBuffer->Push(RawBuffer(buffer, buffer + temp));
 
        // parse while you can
        while (m_responseBuffer->Ready()) {
index bb9ad4d..f440b95 100644 (file)
@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2000 - 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *  Copyright (c) 2000 - 2020 Samsung Electronics Co., Ltd All Rights Reserved
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -308,8 +308,7 @@ int ServiceConnection::receive(CKM::MessageBuffer &recv_buf)
                        break;
                }
 
-               CKM::RawBuffer raw(buffer, buffer + temp);
-               recv_buf.Push(raw);
+               recv_buf.Push(CKM::RawBuffer(buffer, buffer + temp));
        } while (!recv_buf.Ready());
 
        if (ec != CKM_API_SUCCESS)
index ef1ceb2..237f716 100644 (file)
@@ -30,9 +30,9 @@
 
 namespace CKM {
 
-void MessageBuffer::Push(const RawBuffer &data)
+void MessageBuffer::Push(RawBuffer &&data)
 {
-       m_buffer.AppendCopy(&data[0], data.size());
+       m_buffer.Push(std::move(data));
 }
 
 bool MessageBuffer::Ready()
@@ -58,7 +58,7 @@ void MessageBuffer::Read(size_t num, void *bytes)
                Throw(Exception::OutOfData);
        }
 
-       m_buffer.FlattenConsume(bytes, num);
+       m_buffer.Pop(bytes, num);
        m_bytesLeft -= num;
 }
 
index 6f0572a..14e4c3f 100644 (file)
@@ -47,7 +47,7 @@ public:
        MessageBuffer(MessageBuffer &&) = default;
        MessageBuffer &operator=(MessageBuffer &&) = default;
 
-       void Push(const RawBuffer &data);
+       void Push(RawBuffer &&data);
 
        bool Ready();
 
@@ -71,7 +71,7 @@ protected:
                if (m_buffer.Size() < sizeof(size_t))
                        return;  // we cannot count m_bytesLeft because buffer is too small
 
-               m_buffer.FlattenConsume(&m_bytesLeft, sizeof(size_t));
+               m_buffer.Pop(&m_bytesLeft, sizeof(size_t));
        }
 
        size_t m_bytesLeft;
index 08891ed..5c846ed 100644 (file)
@@ -75,7 +75,7 @@ RawBuffer passwordToKey(const Password &password, const RawBuffer &salt,
 RawBuffer unpack(const RawBuffer &packed, const Password &pass)
 {
        MessageBuffer buffer;
-       buffer.Push(packed);
+       buffer.Push(RawBuffer(packed));
        int encryptionScheme = 0;
        RawBuffer data;
        buffer.Deserialize(encryptionScheme, data);
@@ -84,7 +84,7 @@ RawBuffer unpack(const RawBuffer &packed, const Password &pass)
                return data;
 
        MessageBuffer internalBuffer;
-       internalBuffer.Push(data);
+       internalBuffer.Push(std::move(data));
        RawBuffer encrypted;
        RawBuffer iv;
        RawBuffer tag;
index 3183937..ba7de98 100644 (file)
@@ -47,7 +47,7 @@ void unpack(const RawBuffer &packed,
                        RawBuffer &tag)
 {
        MessageBuffer buffer;
-       buffer.Push(packed);
+       buffer.Push(RawBuffer(packed));
 
        buffer.Deserialize(scheme);
 
@@ -61,7 +61,7 @@ void unpack(const RawBuffer &packed,
 RawBuffer unpackData(const RawBuffer &packed)
 {
        MessageBuffer buffer;
-       buffer.Push(packed);
+       buffer.Push(RawBuffer(packed));
 
        int scheme;
        buffer.Deserialize(scheme);
index 404a80c..99a03ef 100644 (file)
  * @version     1.0
  * @brief       This file is the header file of binary queue
  */
-#ifndef CENT_KEY_BINARY_QUEUE_H
-#define CENT_KEY_BINARY_QUEUE_H
 
+#pragma once
+
+#include <ckm/ckm-raw-buffer.h>
 #include <dpl/exception.h>
-#include <list>
-#include <vector>
 #include <symbol-visibility.h>
 
+#include <list>
+#include <queue>
+
 namespace CKM {
 
-/**
- * Binary stream implemented as constant size bucket list
- */
 class COMMON_API BinaryQueue final {
 public:
        class Exception {
@@ -40,59 +39,22 @@ public:
                DECLARE_EXCEPTION_TYPE(Base, OutOfData)
        };
 
-private:
-       typedef std::vector<unsigned char> Bucket;
-
-       typedef std::list<Bucket> BucketList;
-       BucketList m_buckets;
-       size_t m_size;
-
-public:
-       /**
-        * Construct empty binary queue
-        */
        BinaryQueue();
-
        BinaryQueue(const BinaryQueue &other) = delete;
-
-       /**
-        * Construct binary queue by moving data from other binary queue
-        *
-        * @param[in] other Other binary queue to move from
-        */
        BinaryQueue(BinaryQueue &&) = default;
 
-       /**
-        * Append copy of @a bufferSize bytes from memory pointed by @a buffer
-        * to the end of binary queue. Uses default deleter based on free.
-        *
-        * @return none
-        * @param[in] buffer Pointer to buffer to copy data from
-        * @param[in] bufferSize Number of bytes to copy
-        * @exception std::bad_alloc Cannot allocate memory to hold additional data
-        */
-       void AppendCopy(const void *buffer, size_t bufferSize);
+       void Push(RawBuffer&& buffer);
+
+       void Pop(void* buffer, size_t bufferSize);
 
-       /**
-        * Retrieve total size of all data contained in binary queue
-        *
-        * @return Number of bytes in binary queue
-        */
        size_t Size() const;
 
-       /**
-        * Retrieve @a bufferSize bytes from beginning of binary queue, copy them
-        * to user supplied buffer, and remove from binary queue
-        *
-        * @return none
-        * @param[in] buffer Pointer to user buffer to receive bytes
-        * @param[in] bufferSize Size of user buffer pointed by @a buffer
-        * @exception BinaryQueue::Exception::OutOfData Number of bytes to flatten
-        *            is larger than available bytes in binary queue
-        */
-       void FlattenConsume(void *buffer, size_t bufferSize);
+private:
+       // TODO: The default choice is std::deque which is generally faster, but in this use case it's
+       // very likely that this queue will only ever contain 1 element, and std::deque may be slower,
+       // so I have kept this like before. The optimal approach would be to use deque+SSO, probably.
+       std::queue<RawBuffer, std::list<RawBuffer>> m_buckets;
+       size_t m_size, m_offset;
 };
 
 } // namespace CKM
-
-#endif // CENT_KEY_BINARY_QUEUE_H
index ac8db27..ee6606d 100644 (file)
  * @version     1.0
  * @brief       This file is the implementation file of binary queue
  */
+
 #include <dpl/binary_queue.h>
-#include <cassert>
+
 #include <algorithm>
+#include <cassert>
 #include <cstring>
 
 namespace CKM {
-BinaryQueue::BinaryQueue() :
-       m_size(0)
-{}
 
-void BinaryQueue::AppendCopy(const void *buffer, size_t bufferSize)
+BinaryQueue::BinaryQueue()
+       : m_size(0)
+       , m_offset(0)
 {
-       if (bufferSize == 0)
-               return;
-
-       assert(buffer != NULL);
-
-       m_buckets.emplace_back(bufferSize);
-       memcpy(m_buckets.back().data(), buffer, bufferSize);
-       m_size += bufferSize;
 }
 
-size_t BinaryQueue::Size() const
+void BinaryQueue::Push(RawBuffer&& buffer)
 {
-       return m_size;
+       if (buffer.empty())
+               return;
+
+       m_size += buffer.size();
+       m_buckets.push(std::move(buffer));
 }
 
-void BinaryQueue::FlattenConsume(void *buffer, size_t bufferSize)
+void BinaryQueue::Pop(void* buffer, size_t bufferSize)
 {
-       // Check parameters
        if (bufferSize == 0)
                return;
-
-       assert(buffer != NULL);
-
        if (bufferSize > m_size)
                Throw(Exception::OutOfData);
 
-       size_t bytesLeft = bufferSize;
-       void *ptr = buffer;
-       assert(!m_buckets.empty());
-
-       // Flatten data
-       do {
-               auto& bucket = m_buckets.front();
+       assert(buffer != NULL);
 
-               // Get consume size
-               size_t bucketSize = bucket.size();
-               size_t count = std::min(bytesLeft, bucketSize);
+       while (bufferSize > 0) {
+               RawBuffer& bucket = m_buckets.front();
+               size_t toPop = std::min(bucket.size() - m_offset, bufferSize);
 
-               // Copy data to user pointer
-               memcpy(ptr, bucket.data(), count);
+               memcpy(buffer, bucket.data() + m_offset, toPop);
+               buffer = static_cast<unsigned char*>(buffer) + toPop;
+               bufferSize -= toPop;
+               m_size -= toPop;
 
-               // consume
-               if (count == bucketSize) {
-                       m_buckets.pop_front();
-               } else {
-                       bucket.erase(bucket.begin(), bucket.begin() + count);
-                       assert(count == bytesLeft);
-                       break;
-               }
+               if (toPop == bucket.size() - m_offset) {
+                       m_buckets.pop();
+                       m_offset = 0;
+               } else
+                       m_offset += toPop;
+       }
+}
 
-               bytesLeft -= count;
-               ptr = static_cast<char *>(ptr) + count;
-       } while (bytesLeft);
-       m_size -= bufferSize;
+size_t BinaryQueue::Size() const
+{
+       return m_size;
 }
 
 } // namespace CKM
index ef51289..e89efd3 100644 (file)
@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2000 - 2015 Samsung Electronics Co., Ltd All Rights Reserved
+ *  Copyright (c) 2000 - 2020 Samsung Electronics Co., Ltd All Rights Reserved
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -46,7 +46,7 @@ void ThreadService::Handle(const WriteEvent &)
 void ThreadService::Handle(const ReadEvent &event)
 {
        auto &info = m_connectionInfoMap[event.connectionID.counter];
-       info.buffer.Push(event.rawBuffer);
+       info.buffer.Push(RawBuffer(event.rawBuffer));
 
        if (!info.buffer.Ready())
                return;
index 843c3f3..96e7f14 100644 (file)
@@ -142,7 +142,7 @@ void EncryptionService::ProcessEncryption(const ConnectionID &conn,
 void EncryptionService::CustomHandle(const ReadEvent &event)
 {
        auto &info = m_connectionInfoMap[event.connectionID.counter];
-       info.buffer.Push(event.rawBuffer);
+       info.buffer.Push(RawBuffer(event.rawBuffer));
 
        while (ProcessOne(event.connectionID, info, true));
 }
index 779eab4..7a4b23f 100644 (file)
@@ -22,79 +22,69 @@ using namespace CKM;
 
 namespace {
 
-constexpr unsigned char buf[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07};
-constexpr size_t bufSize = sizeof(buf);
+const RawBuffer BUF = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07};
 
 } // namespace anonymous
 
 BOOST_AUTO_TEST_SUITE(BINARY_QUEUE_TEST)
 
-POSITIVE_TEST_CASE(append_copy)
+POSITIVE_TEST_CASE(push)
 {
        BinaryQueue bq;
        BOOST_REQUIRE(bq.Size() == 0);
 
-       bq.AppendCopy(buf, bufSize);
+       bq.Push(RawBuffer(BUF));
+       BOOST_REQUIRE(bq.Size() == BUF.size());
 
-       BOOST_REQUIRE(bq.Size() == bufSize);
-
-       bq.AppendCopy(buf, bufSize);
-
-       BOOST_REQUIRE(bq.Size() == 2 * bufSize);
+       bq.Push(RawBuffer(BUF));
+       BOOST_REQUIRE(bq.Size() == 2 * BUF.size());
 }
 
-NEGATIVE_TEST_CASE(append_copy)
+NEGATIVE_TEST_CASE(push)
 {
        BinaryQueue bq;
 
-       bq.AppendCopy(buf, 0);
-
+       bq.Push(RawBuffer());
        BOOST_REQUIRE(bq.Size() == 0);
 }
 
-POSITIVE_TEST_CASE(flatten_consume)
+POSITIVE_TEST_CASE(pop)
 {
-       BinaryQueue bq;
+       const size_t PART1_SIZE = BUF.size() / 2;
 
-       constexpr size_t part1Size = bufSize / 2;
-
-       bq.AppendCopy(buf, part1Size);
-       bq.AppendCopy(buf + part1Size, bufSize - part1Size);
-
-       char out[3 * bufSize / 4];
-       bq.FlattenConsume(out, sizeof(out));
-       BOOST_REQUIRE(memcmp(buf, out, sizeof(out)) == 0);
+       BinaryQueue bq;
 
-       constexpr size_t remainingSize = bufSize - sizeof(out);
-       BOOST_REQUIRE(bq.Size() == remainingSize);
+       bq.Push(RawBuffer(BUF.begin(), BUF.begin() + PART1_SIZE));
+       bq.Push(RawBuffer(BUF.begin() + PART1_SIZE, BUF.end()));
 
-       bq.FlattenConsume(out, remainingSize);
+       RawBuffer out(3 * BUF.size() / 4);
+       bq.Pop(out.data(), out.size());
+       BOOST_REQUIRE(std::equal(out.begin(), out.end(), BUF.begin()));
 
-       BOOST_REQUIRE(memcmp(buf + sizeof(out), out, remainingSize) == 0);
-       BOOST_REQUIRE(memcmp(buf + remainingSize,
-                            out + remainingSize,
-                            sizeof(out) - remainingSize) == 0);
+       const size_t REST_SIZE = BUF.size() - out.size();
+       BOOST_REQUIRE(bq.Size() == REST_SIZE);
 
+       bq.Pop(out.data(), REST_SIZE);
+       BOOST_REQUIRE(std::equal(out.begin(), out.begin() + REST_SIZE, BUF.begin() + out.size()));
+       BOOST_REQUIRE(std::equal(out.begin() + REST_SIZE, out.end(), BUF.begin() + REST_SIZE));
        BOOST_REQUIRE(bq.Size() == 0);
 }
 
-NEGATIVE_TEST_CASE(flatten_consume)
+NEGATIVE_TEST_CASE(pop)
 {
        BinaryQueue bq;
 
        char out;
-       BOOST_REQUIRE_THROW(bq.FlattenConsume(&out, sizeof(out)),
-                           BinaryQueue::Exception::OutOfData);
+       BOOST_REQUIRE_THROW(bq.Pop(&out, sizeof(out)), BinaryQueue::Exception::OutOfData);
 
-       bq.AppendCopy(buf, bufSize);
+       bq.Push(RawBuffer(BUF));
 
-       bq.FlattenConsume(&out, 0);
-       BOOST_REQUIRE(bq.Size() == bufSize);
+       bq.Pop(&out, 0);
+       BOOST_REQUIRE(bq.Size() == BUF.size());
 
-       char out2[bufSize + 1];
-       BOOST_REQUIRE_THROW(bq.FlattenConsume(out2, sizeof(out2)),
-                           BinaryQueue::Exception::OutOfData);
-       BOOST_REQUIRE(bq.Size() == bufSize);
+       RawBuffer out2(BUF.size() + 1);
+       BOOST_REQUIRE_THROW(bq.Pop(out2.data(), out2.size()), BinaryQueue::Exception::OutOfData);
+       BOOST_REQUIRE(bq.Size() == BUF.size());
 }
 
 BOOST_AUTO_TEST_SUITE_END()
index b9b9eed..c825acd 100644 (file)
@@ -98,9 +98,8 @@ POSITIVE_TEST_CASE(Serialization_CryptoAlgorithm)
 
        CryptoAlgorithmSerializable input(ca);
        CryptoAlgorithmSerializable output;
-       RawBuffer buffer = SerializeMessage(input);
        MessageBuffer resp;
-       resp.Push(buffer);
+       resp.Push(SerializeMessage(input));
        resp.Deserialize(output);
 
        checkIntParam(output, ParamName::ALGO_TYPE,
@@ -121,9 +120,8 @@ NEGATIVE_TEST_CASE(Serialization_CryptoAlgorithm)
 
        CryptoAlgorithmSerializable input(ca);
        CryptoAlgorithmSerializable output;
-       RawBuffer buffer = SerializeMessage(input);
        MessageBuffer resp;
-       resp.Push(buffer);
+       resp.Push(SerializeMessage(input));
        resp.Deserialize(output);
 
        // wrong type
@@ -151,9 +149,8 @@ NEGATIVE_TEST_CASE(Serialization_CryptoAlgorithm_wrong_name)
 
        CryptoAlgorithmSerializable input(ca);
        CryptoAlgorithmSerializable output;
-       RawBuffer buffer = SerializeMessage(input);
        MessageBuffer resp;
-       resp.Push(buffer);
+       resp.Push(SerializeMessage(input));
        BOOST_REQUIRE_THROW(resp.Deserialize(output),
                                                CryptoAlgorithmSerializable::UnsupportedParam);
 }