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

XML ubsan fixes #288

Merged
merged 2 commits into from
Aug 23, 2023
Merged

XML ubsan fixes #288

merged 2 commits into from
Aug 23, 2023

Conversation

cmazakas
Copy link
Member

@cmazakas cmazakas commented Aug 22, 2023

The XML oarchive class generates a few ubsan failures, this PR aims to fix them.

To replicate the failures on develop:

b2 -q test//test_mult_archive_types cxxstd=20 undefined-sanitizer=norecover address-sanitizer=norecover  link=static toolset=clang-16

The first failure is that we're incrementing a nullptr in the increment() of the xml_escape iterator because we never assigned m_bnext to non-null.

The second is a static_cast to a Base that we just replace with a reinterpret_cast, trying to utilize the common initial sequence to silence the sanitizer.

We found this in Unordered when adding Serialization support to some of our containers.

…string

This avoids an extraneous warning when running Serialization code through ubsan which flags increments to a nullptr

The relevant condtional check (`++m_bnext < m_bend`) still passes with this change
There's no inheritance so static_cast'ing to a base is incorrect, instead we use reinterpret_cast with the common initial sequence to avoid ubsan failures
@robertramey robertramey merged commit 60a2837 into boostorg:develop Aug 23, 2023
0 of 14 checks passed
@pdimov
Copy link
Member

pdimov commented Aug 24, 2023

This change is broken and causes crashes.

@pdimov
Copy link
Member

pdimov commented Aug 24, 2023

.\boost/archive/detail/interface_oarchive.hpp(55,16): warning: 'reinterpret_cast' to class 'boost::archive::text_oarchive *' from its base at non-zero offset 'boost::archive::detail::interface_oarchive<boost::archive::text_oarchive> *' behaves differently from 'static_cast' [-Wreinterpret-base-class]
        return reinterpret_cast<Archive*>(this);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.\boost/archive/detail/interface_oarchive.hpp(78,15): note: in instantiation of member function 'boost::archive::detail::interface_oarchive<boost::archive::text_oarchive>::This' requested here
        this->This()->save_override(t);
              ^

@pdimov
Copy link
Member

pdimov commented Aug 24, 2023

testing.capture-output bin.v2\libs\msm\test\SerializeSimpleEuml.test\msvc-14.2\release\cxxstd-latest-iso\threading-multi\SerializeSimpleEuml.run
====== BEGIN OUTPUT ======
Running 1 test case...
unknown location(0): fatal error: in "my_test": memory access violation occurred at address 0x4, while attempting to  read inaccessible data
libs\msm\test\SerializeSimpleEuml.cpp(134): last checkpoint

*** 1 failure is detected in the test module "MyTest"
 
EXIT STATUS: 201 
====== END OUTPUT ======

@robertramey
Copy link
Member

I was sort of skeptical of this change. But I didn't have the time/inclination to really investigate it so I merged it. On my local machine All tests still passed. I'll change this back to the way it was.

@robertramey
Copy link
Member

Done!

@cmazakas
Copy link
Member Author

Yeah, I'd like to apologize for not vetting my changes more carefully.

Initially, I was overwhelmed by the size of Serialization and how it was laid out.

Going forward in the future, I'll test changes locally a bit more carefully before opening another PR.

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 this pull request may close these issues.

3 participants