Skip to content

Fix clang sanitizer invocation#1728

Merged
nlohmann merged 5 commits into
nlohmann:developfrom
t-b:fix-clang-sanitizer-invocation
Sep 10, 2019
Merged

Fix clang sanitizer invocation#1728
nlohmann merged 5 commits into
nlohmann:developfrom
t-b:fix-clang-sanitizer-invocation

Conversation

@t-b
Copy link
Copy Markdown
Contributor

@t-b t-b commented Aug 27, 2019 •
edited
Loading

The clang version used for the sanitizer got upgraded. In addition we now don't recover from sanitizer issues, thus make passing them a requirement, and also print out the stacktrace on sanitizer issues.

Close #1716.

@t-b t-b mentioned this pull request Aug 27, 2019
Closed
@coveralls
Copy link
Copy Markdown

coveralls commented Aug 27, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 6d701b2 on t-b:fix-clang-sanitizer-invocation into eab68e7 on nlohmann:develop.

@t-b

This comment has been minimized.

Sign in to view
@t-b t-b force-pushed the fix-clang-sanitizer-invocation branch from b125e29 to 5cfa1ac Compare August 30, 2019 16:14
@t-b
Copy link
Copy Markdown
Contributor Author

t-b commented Aug 30, 2019

Progress:

  • Fixed some of the violations
  • Left over ones are in the tests (signed char vs uint8_t)
  • I do get an error here with libstd++ from gcc 6.3 with std::vector which AFAIU is not our bug.

I'm attaching the log here
output.txt.

Suggestions on how to proceed are appreciated.

jaredgrubb
jaredgrubb reviewed Aug 30, 2019
View reviewed changes
Comment thread include/nlohmann/detail/conversions/to_json.hpp
@t-b t-b force-pushed the fix-clang-sanitizer-invocation branch 2 times, most recently from 971a8ec to 1dad928 Compare August 31, 2019 12:31
@t-b
Copy link
Copy Markdown
Contributor Author

t-b commented Aug 31, 2019 •
edited
Loading

  • Cleaned up some commits
  • Added UBSAN suppression support and ignore the std::vector<bool> issue,
    see 5da932f for details

@t-b
Copy link
Copy Markdown
Contributor Author

t-b commented Aug 31, 2019

@jaredgrubb @nlohmann The unicode test with sanitizer timed out, but no errors from the sanitizer.

nlohmann
nlohmann requested changes Sep 2, 2019
View reviewed changes
Copy link
Copy Markdown
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just would request some minor changes.

Comment thread include/nlohmann/detail/input/input_adapters.hpp Outdated
@nlohmann
Copy link
Copy Markdown
Owner

nlohmann commented Sep 2, 2019

@jaredgrubb @nlohmann The unicode test with sanitizer timed out, but no errors from the sanitizer.

I think we should increase the timeout to make sure the CI can run successfully.

t-b added 5 commits September 3, 2019 13:22
@t-b
test/CMakeLists.txt: Remove trailing whitespace
f0bff49
@t-b
.travis/cmake: Rework clang sanitizer invocation
9ea3e19 
- Switch to clang-7
- Adapt PATH so that llvm-symbolizer can be found for useful stacktraces
- Adapt compile flags
  "-O0" ensures much faster compile times
  "-fno-sanitize-recover=all
  -fsanitize-recover=unsigned-integer-overflow" this fails the build on
  all issues except unsigned integer overflows. Not failing in this case
  is required in combination with the sanitizer suppression file as only
  recoverable errors can be suppressed.

The UBSAN suppression file ignores errors from stl_bvector.h (which
holds std::vector<bool>).

Clang reports that error as

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/stl_bvector.h:158:20 in

      Start 34: test-deserialization_all
28/88 Test nlohmann#71: test-testsuites_default .............***Failed    0.32 sec
/usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/stl_bvector.h:158:20: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned int'
    #0 0x628f72 in std::_Bit_iterator_base::_M_bump_down() /usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/stl_bvector.h:158:20
    nlohmann#1 0x628d16 in std::_Bit_iterator::operator--() /usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/stl_bvector.h:251:7
    nlohmann#2 0x634aac in std::vector<bool, std::allocator<bool> >::pop_back() /usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/stl_bvector.h:1010:7
    nlohmann#3 0x61eff0 in bool nlohmann::detail::parser<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer> >::sax_parse_internal<nlohmann::detail::json_sax_dom_parser<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer> > >(nlohmann::detail::json_sax_dom_parser<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer> >*) /home/firma/devel/json/include/nlohmann/detail/input/parser.hpp:439:28
    nlohmann#4 0x604864 in nlohmann::detail::parser<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer> >::parse(bool, nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&) /home/firma/devel/json/include/nlohmann/detail/input/parser.hpp:116:13
    nlohmann#5 0x5f8079 in nlohmann::operator>>(std::istream&, nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&) /home/firma/devel/json/include/nlohmann/json.hpp:6356:42
    nlohmann#6 0x5e1d92 in _DOCTEST_ANON_FUNC_21() /home/firma/devel/json/test/src/unit-testsuites.cpp:343:9
    nlohmann#7 0x7207fe in doctest::Context::run() /home/firma/devel/json/test/thirdparty/doctest/doctest.h:5938:21
    nlohmann#8 0x72681a in main /home/firma/devel/json/test/thirdparty/doctest/doctest.h:6016:71
    nlohmann#9 0x7f75d22362e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
    nlohmann#10 0x4c28b9 in _start (/home/firma/devel/json/build/test/test-testsuites+0x4c28b9)

The pop_back() in parser.hpp

      assert(not states.empty());
  ->  states.pop_back();

triggers the UBSAN report. But the assertion above ensure that we only
call pop_back() on an non-empty vector, therefore this is a STL library
bug and thus must be ignored for us.
@t-b
input_buffer_adapter: Fix handling of nullptr input
61fe5f1 
Clang UBSAN currently complains that the char * to input_buffer_adapter
is a nullptr.

Turns out it is actually required to accept nullptr, see for example
line 415 in input_adapters.hpp

  ...
  // the address of first cannot be used: use nullptr
  ia = std::make_shared<input_buffer_adapter>(nullptr, len);
  ....

Therefore we have to handle it gracefully here. We now also ignore the
length parameter l if b is a nullptr.
@t-b
external_constructor<std::valarray>: Handle empty array properly
d5c0d52 
Clang UBSAN complains with the following message when an empty std::valarray is passed in:

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/valarray:571:14 in

2/2 Test nlohmann#68: test-regression_all ..............***Failed    4.68 sec
/usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/valarray:571:14: runtime error: reference binding to null pointer of type 'const do
uble'
    #0 0x6fbe57 in std::valarray<double>::operator[](unsigned long) const /usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/valarray:
571:7
    nlohmann#1 0x6fbe57 in double const* std::begin<double>(std::valarray<double> const&) /usr/lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/v
alarray:1207
    nlohmann#2 0x6fbe57 in void nlohmann::detail::external_constructor<(nlohmann::detail::value_t)2>::construct<nlohmann::basic_json<std::map, std::vector, s
td::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_seri
alizer>, double, 0>(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool
, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, std::valarray<double> const&) /home/firma/devel/json/include/nlohmann/deta
il/conversions/to_json.hpp:157
    nlohmann#3 0x5e3fe3 in void nlohmann::detail::to_json<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>
, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>, double, 0>(nlohmann::basic_json<std::map, std
::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohman
n::adl_serializer>&, std::valarray<double> const&) /home/firma/devel/json/include/nlohmann/detail/conversions/to_json.hpp:270:5
    nlohmann#4 0x5e3fe3 in decltype((to_json(fp, std::forward<std::valarray<double>&>(fp0))) , ((void)())) nlohmann::detail::to_json_fn::operator()<nlohmann:
:basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double
, std::allocator, nlohmann::adl_serializer>, std::valarray<double>&>(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std
::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, std::valarray<double>&) c
onst /home/firma/devel/json/include/nlohmann/detail/conversions/to_json.hpp:334
    nlohmann#5 0x5e3fe3 in decltype((nlohmann::(anonymous namespace)::to_json(fp, std::forward<std::valarray<double>&>(fp0))) , ((void)())) nlohmann::adl_ser
ializer<std::valarray<double>, void>::to_json<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, st
d::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>, std::valarray<double>&>(nlohmann::basic_json<std:
:map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator
, nlohmann::adl_serializer>&, std::valarray<double>&) /home/firma/devel/json/include/nlohmann/adl_serializer.hpp:45
    nlohmann#6 0x5e3fe3 in nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool,
 long, unsigned long, double, std::allocator, nlohmann::adl_serializer>::basic_json<std::valarray<double>&, std::valarray<double>, 0>(std::valarray<d
ouble>&) /home/firma/devel/json/include/nlohmann/json.hpp:1257
    nlohmann#7 0x5e3fe3 in _DOCTEST_ANON_FUNC_2() /home/firma/devel/json/test/src/unit-regression.cpp:1377
    nlohmann#8 0x77313e in doctest::Context::run() /home/firma/devel/json/test/thirdparty/doctest/doctest.h:5938:21
    nlohmann#9 0x777ae0 in main /home/firma/devel/json/test/thirdparty/doctest/doctest.h:6016:71
    nlohmann#10 0x7fae220532e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
    nlohmann#11 0x4a6479 in _start (/home/firma/devel/json/build/test/test-regression+0x4a6479)

The important thing to note here is that a std::valarray is *not* a STL
container, so the usual containter and iterator semantics don't apply.

Therefore we have to check if the container is non-empty before.
@t-b
.travis.yml: Increase the timeout to 45 minutes
6d701b2 
The clang sanitizer tests, and there especially the unicode tests, can
hit the default timeout of 25 minutes (1500 seconds) quite easily, so
let's raise the timeout to 45 minutes (2700 seconds).
@t-b t-b force-pushed the fix-clang-sanitizer-invocation branch from 1dad928 to 6d701b2 Compare September 3, 2019 11:34
@t-b
Copy link
Copy Markdown
Contributor Author

t-b commented Sep 3, 2019

I think we should increase the timeout to make sure the CI can run successfully.

@nlohmann Done.

@t-b
Copy link
Copy Markdown
Contributor Author

t-b commented Sep 3, 2019

Thanks for the review btw.

nlohmann
nlohmann approved these changes Sep 10, 2019
View reviewed changes
Copy link
Copy Markdown
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann self-assigned this Sep 10, 2019
@nlohmann nlohmann added this to the Release 3.7.1 milestone Sep 10, 2019
@nlohmann
Copy link
Copy Markdown
Owner

nlohmann commented Sep 10, 2019

🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description

  • Updated sanitizers and make sanitizer check a mandatory CI step.

@nlohmann nlohmann merged commit a6bd798 into nlohmann:develop Sep 10, 2019
@nlohmann
Copy link
Copy Markdown
Owner

nlohmann commented Sep 10, 2019

Thanks a lot!!

@t-b t-b deleted the fix-clang-sanitizer-invocation branch September 10, 2019 09:39
@bassosimone
Copy link
Copy Markdown

bassosimone commented Sep 10, 2019

Thanks @t-b and @nlohmann!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nlohmann nlohmann nlohmann approved these changes

+1 more reviewer

@jaredgrubb jaredgrubb jaredgrubb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@nlohmann nlohmann

Labels

release item: 🔨 further change

Projects

None yet

Milestone

Release 3.7.1

Development

Successfully merging this pull request may close these issues.

json::parse() ubsan regression with v3.7.0

5 participants

@t-b @coveralls @nlohmann @bassosimone @jaredgrubb