Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
DanRStevens opened this issue Mar 7, 2019 · 2 comments · Fixed by #280
Closed

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

DanRStevens opened this issue Mar 7, 2019 · 2 comments · Fixed by #280

Comments

@DanRStevens
Copy link
Member

DanRStevens commented Mar 7, 2019

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
Copy link
Member Author

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
Copy link
Member

Brett208 commented Mar 8, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants