OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Stream code should not direct serialize vectors of non-trivial types #276

Closed DanRStevens closed 5 years ago

DanRStevens commented 5 years ago

For reference, the following enable_if check is used to only directly serialize trivial objects:

        // Trivially copyable data types
        template<typename T>
        inline std::enable_if_t<std::is_trivially_copyable<T>::value> Write(const T& object) {
            WriteImplementation(&object, sizeof(object));
        }

However, when serializing a vector, currently no check is done for the underlying type of the vector:

        // Vector data types
        template<typename T, typename A>
        inline void Write(const std::vector<T, A>& vector) {
            // Size calculation can't possibly overflow since the vector size necessarily fits in memory
            WriteImplementation(vector.data(), vector.size() * sizeof(T));
        }

This could potentially fail in bad ways for vectors of non-trivial types. We should add an appropriate enable_if guard on the vector's underlying type T, to ensure it is trivially-copyable.

In particular, this unguarded method should fail for std::vector<std::string> (unless the strings happen to all use the short-string optimization).


See the proposals in Issue #277 and Issue #278 for how we can serialize vectors of non-trivial types.

DanRStevens commented 5 years ago

This short snippet of code to demonstrates the problem:

#include "src/Stream/FileWriter.h"
#include <vector>
#include <string>

int main() {
  Stream::FileWriter writer("Test.out");
  std::vector<std::string> data;

  data.push_back("Short string");
  data.push_back("A considerably longer string");

  writer.Write(data);  // **

  return 0;
}

Compiled with:

clang++ -std=c++14 Test.cpp -L./ -lOP2Utility -lstdc++fs

The code compiled and ran without error. It produced the following output:

hexdump -C Test.out 
00000000  20 51 20 01 00 00 00 00  0c 00 00 00 00 00 00 00  | Q .............|
00000010  53 68 6f 72 74 20 73 74  72 69 6e 67 00 7f 00 00  |Short string....|
00000020  e0 50 20 01 00 00 00 00  1c 00 00 00 00 00 00 00  |.P .............|
00000030  1c 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000040

Ideally, this should either produce a compile error, or more useful binary output.

Brett208 commented 5 years ago

String only failing based on its length seems really unintuitive. This is a good check to add.