Serializing a std::map or std::unordered_map whose value type is a polymorphic Chrono object
silently writes a truncated record. Reported originally as a crash in #507. A fix is proposed in #774.
Mechanism
ChNameValue is non-owning. Its only value constructor takes const T& and stores a raw pointer,
casting away constness on the way (ChArchive.h#L301-L308):
ChNameValue(const std::string& mname, const T& mvalue, ...)
: _name(mname), _value((T*)(&mvalue)), ...
Because the parameter is const T&, a temporary binds to it happily, and the stored pointer dangles
as soon as that temporary dies.
Dereferencing a std::map<T, Tv> iterator yields std::pair<const T, Tv>&. But the out() wrappers
for both associative containers declared their ChNameValue with a non-const key
(ChArchive.h#L995
and #L1011):
ChNameValue<std::pair<T, Tv>> array_key(std::to_string(i), (*it)); // <-- non-const key
this->out(array_key);
The two types differ, so std::pair's converting constructor materializes a temporary
std::pair<T, Tv>. Two consequences follow:
- The archive writes a copy of the element rather than the element itself.
- That copy is destroyed at the closing semicolon, so the next line reads freed memory.
std::vector and std::list are unaffected: they pass bVal.value()[i] and (*iter), which are
already exactly T&, so no conversion and no temporary occur.
Observed effect
Serializing std::map<std::string, ChLinkTSDA> to JSON produces a 254 byte record containing only
the ChObj fields. There is no _version_ChLinkTSDA, no m_k, m_r, m_f, m_rest_length, and no
bodies. The correct record is about 1100 bytes. Measured container matrix, with each value stamped
rest_length = 12345 and k = 999:
| Container |
Result |
vector<ChLinkTSDA> |
full record, 1006 B, sentinels present |
list<ChLinkTSDA> |
full record, 1006 B, sentinels present |
pair<string, ChLinkTSDA> used directly |
full record, 1028 B, sentinels present |
map<string, ChLinkTSDA> |
truncated to ChObj, 254 B, sentinels lost |
unordered_map<string, ChLinkTSDA> |
truncated to ChObj, 254 B, sentinels lost |
map<string, ChVector3d> |
full and correct |
Why this went unnoticed for so long
Reading a destroyed object is undefined behavior, and undefined does not mean reliably broken. A
trivially destructible type such as ChVector3d normally still reads back its old bytes and looks
perfectly correct, which is why the last row above passes. Only a type with a non-trivial destructor,
and especially a polymorphic one, loses its derived data.
Coverage reinforces this. utest_CH_archive had no STL container test of any kind, and the only map
in demo_CH_archive is an unordered_map<int, double> of PODs, serialized in a function whose
deserialization half contains no assertions at all.
Age
The unordered_map wrapper has had this shape since ef4ef48 (2017-02-09, "Archive machinery for
(de)serializing std::unordered_map"). Notably the in() half of that same commit was written
correctly, with a named local std::pair<T, Tv> mpair;, and it is still correct today. The
out-versus-in asymmetry has been there from the beginning, which suggests an oversight rather than a
deliberate choice. The std::map wrapper was added later and copied the pattern.
On fixing it
Worth recording, because the obvious fix is wrong: giving the temporary a name so that it outlives the
out() call does compile and does stop the crash, but it keeps serializing a copy. That is not
equivalent, because ChLink's copy constructor nulls both body pointers (see #777), so it would
trade a loud bug for a silent one.
#774 instead declares the key const in the two out() wrappers so the ChNameValue binds to the
real element and no temporary is created. The in() path is deliberately untouched:
_wrap_pair::ArchiveIn writes into first, so a const key there would assign through a pointer to a
const object. Since template member functions are instantiated only when used, ArchiveIn is never
instantiated on the const-keyed specialization.
Related
A separate instance of the same non-owning-pointer mistake in this subsystem is filed as #779.
Serializing a
std::maporstd::unordered_mapwhose value type is a polymorphic Chrono objectsilently writes a truncated record. Reported originally as a crash in #507. A fix is proposed in #774.
Mechanism
ChNameValueis non-owning. Its only value constructor takesconst T&and stores a raw pointer,casting away constness on the way (ChArchive.h#L301-L308):
Because the parameter is
const T&, a temporary binds to it happily, and the stored pointer danglesas soon as that temporary dies.
Dereferencing a
std::map<T, Tv>iterator yieldsstd::pair<const T, Tv>&. But theout()wrappersfor both associative containers declared their
ChNameValuewith a non-const key(ChArchive.h#L995
and #L1011):
The two types differ, so
std::pair's converting constructor materializes a temporarystd::pair<T, Tv>. Two consequences follow:std::vectorandstd::listare unaffected: they passbVal.value()[i]and(*iter), which arealready exactly
T&, so no conversion and no temporary occur.Observed effect
Serializing
std::map<std::string, ChLinkTSDA>to JSON produces a 254 byte record containing onlythe
ChObjfields. There is no_version_ChLinkTSDA, nom_k,m_r,m_f,m_rest_length, and nobodies. The correct record is about 1100 bytes. Measured container matrix, with each value stamped
rest_length = 12345andk = 999:vector<ChLinkTSDA>list<ChLinkTSDA>pair<string, ChLinkTSDA>used directlymap<string, ChLinkTSDA>ChObj, 254 B, sentinels lostunordered_map<string, ChLinkTSDA>ChObj, 254 B, sentinels lostmap<string, ChVector3d>Why this went unnoticed for so long
Reading a destroyed object is undefined behavior, and undefined does not mean reliably broken. A
trivially destructible type such as
ChVector3dnormally still reads back its old bytes and looksperfectly correct, which is why the last row above passes. Only a type with a non-trivial destructor,
and especially a polymorphic one, loses its derived data.
Coverage reinforces this.
utest_CH_archivehad no STL container test of any kind, and the only mapin
demo_CH_archiveis anunordered_map<int, double>of PODs, serialized in a function whosedeserialization half contains no assertions at all.
Age
The
unordered_mapwrapper has had this shape since ef4ef48 (2017-02-09, "Archive machinery for(de)serializing std::unordered_map"). Notably the
in()half of that same commit was writtencorrectly, with a named local
std::pair<T, Tv> mpair;, and it is still correct today. Theout-versus-in asymmetry has been there from the beginning, which suggests an oversight rather than a
deliberate choice. The
std::mapwrapper was added later and copied the pattern.On fixing it
Worth recording, because the obvious fix is wrong: giving the temporary a name so that it outlives the
out()call does compile and does stop the crash, but it keeps serializing a copy. That is notequivalent, because
ChLink's copy constructor nulls both body pointers (see #777), so it wouldtrade a loud bug for a silent one.
#774 instead declares the key
constin the twoout()wrappers so theChNameValuebinds to thereal element and no temporary is created. The
in()path is deliberately untouched:_wrap_pair::ArchiveInwrites intofirst, so a const key there would assign through a pointer to aconst object. Since template member functions are instantiated only when used,
ArchiveInis neverinstantiated on the const-keyed specialization.
Related
A separate instance of the same non-owning-pointer mistake in this subsystem is filed as #779.