Opened 11 years ago

Last modified 10 years ago

#5579 new Bugs

1_40_0 serialization break.

Reported by: Phil Hartmann <phil.hartmann82@…> Owned by: Robert Ramey
Milestone: To Be Determined Component: serialization
Version: Boost 1.46.1 Severity: Showstopper
Keywords: serialization object cross dll Cc:

Description

If you compare 1_39 and 1_40 here, you can see one 1_39 compares the addresses of functions to get type information. Which in 1_40 was 'fixed' so that types across DLL's would be matched.

However this of course breaks archive compatibility with 1_39 as the two dll's will have serialized out the CO info twice, and now in 1_40 and beyond it'll only try to read it once.. Leaving the file corrupted.

http://www.boost.org/doc/libs/1_39_0/boost/archive/detail/basic_serializer.hpp http://www.boost.org/doc/libs/1_40_0/boost/archive/detail/basic_serializer.hpp

I'm not even sure how this can be fixed short of storing a flag to determine how cobject's compare thats based on archive version where the comparison starts...

Attachments (2)

boost-serialization-1_46_1-patch-for-5567_and_5579.patch (4.1 KB ) - added by Phil Hartmann <phil.hartmann82@…> 11 years ago.
Patch file for 5579 & 5567
test-runs.zip (39.5 KB ) - added by Phil Hartmann <phil.hartmann82@…> 11 years ago.
.test output of libs/serialization/bjam result.

Download all attachments as: .zip

Change History (11)

comment:1 by Phil Hartmann <phil.hartmann82@…>, 11 years ago

I thought I had a potential fix, but to make matters significantly worse, this is the same problem 1.42 and 1.43 had.. Another major change without having ever bumped the revision number..

http://www.boost.org/doc/libs/1_39_0/libs/serialization/src/basic_archive.cpp http://www.boost.org/doc/libs/1_40_0/libs/serialization/src/basic_archive.cpp

So these two archives will serialization differently and there is no indicator of it.

The only hope would be to put a compile/runtime switch and hope the user never serialized with both 1_37 and 1_40.

comment:2 by Robert Ramey, 11 years ago

"If you compare 1_39 and 1_40 here, you can see one 1_39 compares the addresses of functions to get type information. Which in 1_40 was 'fixed' so that types across DLL's would be matched."

I'm not crazy about the quotes around 'fixed".

In any case, since we know the archive version used to write the file and we know the current archive version, couldn't we condition the recording of the CO record based on writing version? Wouldn't that address the issue? I'm speaking from memory here so bear with me.

Robert Ramey

comment:3 by Phil Hartmann <phil.hartmann82@…>, 11 years ago

Sorry Ramey, didn't mean to imply that the fix itself was incorrect. The change makes sense, the problem arises from archives which are written in 1.37-1.39 and 1.40 because they are all version 5.

DLL1: archive & <type_a>;
DLL2: archive & <type_a>;

In 1.37-1.39 it'll write
<type_a_info><data><type_a_info><data>
in 1.40 and above it'll write
<type_a_info><data><data>
So in 1.40 if I attempt to read in the above to read a 1.39 archive it attempts to read:
<type_a_info><data><data>

Which obviously blows up as its the length of <type_a_info> off on the second read.

What i've added at the moment is a boolean flag to cobject, which if its set it uses the older incorrect comparison, and for newer archives can use the new comparison.

I also added a global constant that I just temporarily set to 'true' called boost_archive_ver_5_is_pre140, i'm not sure what the real solution here is because both of them are version 5. We need some user way to specify via config file to specify if version 5 was 1.39 or 1.40, and if you had archives in both 1.39/1.40 I think we're just kind of hosed in that case, luckily for me I am not, hopefully most others can make the same assumption that they are either 1.40 or 1.39.

const bool boost_archive_ver_5_is_pre_140  = true;

inline class_id_type basic_iarchive_impl::register_type(const basic_iserializer & bis)
{
    // if we are serializing in an old archive (before version 5, or version 5 and the user specified that 
    // archive revision 5 is actually 1_39 not 1_40 [ as this is ambigous ]
    bool old_cid_lookup = (m_archive_library_version < 5 || (5 == m_archive_library_version &&  boost_archive_ver_5_is_pre_140));

    class_id_type cid(cobject_info_set.size());
    cobject_type  co(cid, bis, old_cid_lookup);

    std::pair<cobject_info_set_type::const_iterator, bool> result = cobject_info_set.insert(co);

comment:4 by Phil Hartmann <phil.hartmann82@…>, 11 years ago

Ramey,

Is there anything I can help do to aid this moving forward? I'm very invested in the serialization library at this point, and need to be able to move forward. So helping support and prevent these kinds of issues is of critical importance to me and my team.

If you would like to talk further you can hit me up on Skype at my email (with the . and without the 82). Or I tend to hang around on IRC on the boost channel. Skype is the best bet for grabbing my attention.

comment:5 by Robert Ramey, 11 years ago

Just to make sure I understand the situation -

a) I broke compatibility with 1.40 b) I failed to bump the the archive version # c) we could enhance basic_iarchive.cpp to accomodate fix this. This would work on all transitions except to verion 1.40. d) Implementing this would be a strict improvement in the library. That is, it would hurt no one and help.

So I would suggest the following: a) Sync your code to the latest boost release branch b) make changes to fix this - recognizing that it can't be addressed for one version c) run all the tests using library_test.bat (or .sh) d) post a patch here

and I'll look at it.

Robert Ramey

comment:6 by Phil Hartmann <phil.hartmann82@…>, 11 years ago

Sorry,

For some reason I don't get any notification on reply's.

I'm gathering various output files from our product to test against these changes. I'll also look into the library_test.bat (is there any documentation or is it pretty self explanatory?)

I also depend on https://svn.boost.org/trac/boost/ticket/5567 being fixed, I will likely submit the patch for both as they both are required to correctly load in. I had one question on that one if you could respond to that.

I should be able to have a patch by the end of this week.

by Phil Hartmann <phil.hartmann82@…>, 11 years ago

Patch file for 5579 & 5567

by Phil Hartmann <phil.hartmann82@…>, 11 years ago

Attachment: test-runs.zip added

.test output of libs/serialization/bjam result.

comment:7 by Phil Hartmann <phil.hartmann82@…>, 11 years ago

Sorry for the lengthy delay in getting this, i've just been swmaped.

The patch has both the fixes for 5579 and 5567 as all of the various files I went through throughout the years we've been using serialization these seem to have fixed at least those issues, testing out newer archives also seemed to be ok.

There is a global constant thats just set to 'true' for denoting the version you are coming from, maybe this is something we should expose through some call or just always break 1_40 archives if they serialized across DLL's?

I've attached the patch and ran bjam in libs/serialization/test, then I zipped the resulting .test files up and stuck them here.

I spent 2 days trying to figure out how to use the regression tool and i'm just going in circles with no progression. Probably missing something simple, so I don't know if this is what you need or not, but all of the 286 tests have a status of 'passed' in them, so i'm assuming its good...

  • Phil

comment:8 by Robert Ramey, 11 years ago

I've looked at your patches and I have a few questions.

a) boost/archive/basic_binary_iarchive.hpp lines 88-113. Your changes look correct to me. BUT if these are correct (as the look), it doesn't seem to me that the current regression tests should be passing at all. I'm still investigating this.

b) I'm extremely skeptical of changes to basic_iarchive.cpp to address issue related to the just the binary file. it's not clear to me that this is related to my changing the sizes of binary types or something solely related to serializing across DLLS in dependent of the archive type.

Robert Ramey

comment:9 by blkohn@…, 10 years ago

This has affected our teams code as well, has there been any progress on this?

Note: See TracTickets for help on using tickets.