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.

Sign up for GitHub

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

Jump to bottom

change bjdata ndarray flag to detect negative size, as part of #3475 #3479

Merged

nlohmann merged 9 commits into nlohmann:develop from NeuroJSON:issue3475

May 10, 2022

Contributor

fangq commented May 8, 2022

here is the fix to #3475. I took @falbrechtskirchinger's suggestion, and use size_and_type.second's bit 8 (all BJData nd UBJSON markers are ASCII letters) as an ND array indicator, this allows negative sizes to be thrown as json.exception.out_of_range error.

This should fix the detected fuzzer errors. Let me know if you still see anything.


          change bjdata ndarray flag to detect negative size, fix nlohmann#3475

774d737

fangq requested a review from nlohmann as a code owner

May 8, 2022 22:20


          fix CI error

coveralls commented May 8, 2022 •

edited

Loading

Coverage remained the same at 100.0% when pulling 55350c9 on NeuroJSON:issue3475 into b205361 on nlohmann:develop.

fangq added 2 commits

May 8, 2022 20:02


          fix CI on 32bit windows

e74a859


          remove platform specific out_of_range error messages

ffcf148

Contributor

falbrechtskirchinger commented May 9, 2022 •

edited

Loading

My apologies for derailing the conversation in #3475.

You have fixed an issue raised in #3475 but not the issue that started #3475. @nlohmann and I started talking about fuzzing in general, I discovered more failures, and then I shared one of those, which is the one you have now fixed.

~~Unless I'm mistaken, this doesn't fix the assertion error from the issue description? That was somehow related to unexpect_eof(), right?~~ Edit: Apparently, it does.

I suggest you change the title of this PR to something more generic and we address all assertion failures discovered by fuzzing in one go. ~~I'll share a list in #3475.~~ Edit: Everything in that list has been fixed.

nlohmann requested changes

View reviewed changes

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

tests/src/unit-bjdata.cpp Outdated Show resolved Hide resolved

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

falbrechtskirchinger suggested changes

View reviewed changes

Contributor

falbrechtskirchinger left a comment

I'd use 0x0100 instead of 256. I find it to be slightly clearer but maybe that's just my embedded background. 🤷‍♂️

Also please use bit operations to test and clear bit 8.

// unfortunately we need a cast here which makes it more verbose :-(
is_ndarray = static_cast<bool>(size_and_type.second & 0x0100)

size_and_type.second &= ~0x0100;

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

falbrechtskirchinger mentioned this pull request

json:parse_bjdata_fuzzer reaches assertion #3475

Closed

2 tasks

falbrechtskirchinger reviewed

View reviewed changes

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

falbrechtskirchinger reviewed

View reviewed changes

include/nlohmann/detail/input/binary_reader.hpp Show resolved Hide resolved

Contributor

falbrechtskirchinger commented May 9, 2022

After these changes, the only crashes that remain seem to be stack overflows.

Summary stats
=============

       Fuzzers alive : 10
      Total run time : 5 hours, 40 minutes
         Total execs : 253 millions
    Cumulative speed : 124093 execs/sec
       Average speed : 12409 execs/sec
       Pending items : 0 faves, 129 total
  Pending per fuzzer : 0 faves, 12 total (on average)
       Crashes saved : 10
Cycles without finds : 53/17/6/16/13/8/19/8/15/17
  Time without finds : 2 minutes, 27 seconds

I've built the fuzzer with ASan to confirm.

$ find fuzzer*/crashes/ -iname 'id*' -exec bash -c 'echo {}; ../fuzzer <{} |& grep -E "^SUMMARY"; echo' \;

fuzzer0/crashes/id:000000,sig:11,src:000034,time:600227,execs:7245472,op:havoc,rep:16
SUMMARY: AddressSanitizer: stack-overflow /var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.3/work/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3 in __asan_memset

fuzzer1/crashes/id:000000,sig:11,src:000007,time:602569,execs:7906177,op:havoc,rep:16
SUMMARY: AddressSanitizer: stack-overflow /var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.3/work/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3 in __asan_memset

fuzzer2/crashes/id:000000,sig:11,src:000023,time:600389,execs:7884746,op:havoc,rep:4
SUMMARY: AddressSanitizer: stack-overflow /var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.3/work/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3 in __asan_memset

fuzzer3/crashes/id:000000,sig:11,src:000006,time:602138,execs:8012670,op:havoc,rep:4
SUMMARY: AddressSanitizer: stack-overflow /var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.3/work/compiler-rt/lib/asan/../sanitizer_common/sanitizer_stacktrace.h:53:45 in StackTrace

fuzzer4/crashes/id:000000,sig:11,src:000011+000001,time:602442,execs:7641057,op:splice,rep:2
SUMMARY: AddressSanitizer: stack-overflow /var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.3/work/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3 in __asan_memset

fuzzer5/crashes/id:000000,sig:11,src:000006,time:601260,execs:10537713,op:havoc,rep:16
SUMMARY: AddressSanitizer: stack-overflow /var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.3/work/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3 in __asan_memset

fuzzer6/crashes/id:000000,sig:11,src:000034+000053,time:601491,execs:7847427,op:splice,rep:16
SUMMARY: AddressSanitizer: stack-overflow /var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.3/work/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3 in __asan_memset

fuzzer7/crashes/id:000000,sig:11,src:000003,time:601378,execs:7127960,op:havoc,rep:8
SUMMARY: AddressSanitizer: stack-overflow /var/tmp/portage/sys-libs/compiler-rt-sanitizers-14.0.3/work/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3 in __asan_memset

fuzzer8/crashes/id:000000,sig:11,src:000007,time:600565,execs:7814233,op:havoc,rep:8
SUMMARY: AddressSanitizer: stack-overflow /home/flo/projects/json/issue3475/include/nlohmann/detail/input/binary_reader.hpp:2417 in nlohmann::detail::binary_reader<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::vector<unsigned char, std::allocator<unsigned char> > >, nlohmann::detail::iterator_input_adapter<__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > > >, 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, std::vector<unsigned char, std::allocator<unsigned char> > > > >::get_ubjson_array()

fuzzer9/crashes/id:000000,sig:11,src:000034+000031,time:601075,execs:7913481,op:splice,rep:16
SUMMARY: AddressSanitizer: stack-overflow /home/flo/projects/json/issue3475/include/nlohmann/detail/input/binary_reader.hpp:2417 in nlohmann::detail::binary_reader<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::vector<unsigned char, std::allocator<unsigned char> > >, nlohmann::detail::iterator_input_adapter<__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > > >, 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, std::vector<unsigned char, std::allocator<unsigned char> > > > >::get_ubjson_array()

fangq changed the title ~~change bjdata ndarray flag to detect negative size, fix #3475~~ change bjdata ndarray flag to detect negative size, as part of #3475


          Incorporate suggestions from @nlohmann and @falbrechtskirchinger

cfb5da6

falbrechtskirchinger suggested changes

View reviewed changes

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved

include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved


          fix CI errors

2798bed

falbrechtskirchinger reviewed

View reviewed changes

include/nlohmann/detail/input/binary_reader.hpp

+                                                      exception_message(input_format, "invalid byte: 0x" + last_token, "type"), nullptr));
+                          }
+                          if (JSON_HEDLEY_UNLIKELY(!sax->key(key) || !sax->string(bjdtype[size_and_type.second]) ))

Contributor

falbrechtskirchinger May 9, 2022

Test coverage has decreased because this SAX abort is untested. Can you please construct a test case for this scenario?


          add coverage

8ede0cd

nlohmann added the aspect: binary formats label

falbrechtskirchinger reviewed

View reviewed changes

tests/src/unit-bjdata.cpp

+                          SaxCountdown scp(10);
+                          CHECK(!json::sax_parse(v, &scp, json::input_format_t::bjdata));
+                      }

Contributor

falbrechtskirchinger May 10, 2022

From my testing, the correct counts seem to be 6 and 7.

Please also replace all occurrences of CHECK(! with CHECK_FALSE(.

Contributor Author

fangq May 10, 2022 •

edited

Loading

@falbrechtskirchinger, thanks. is there a way to print the event count? I previously try to manually count but sometime my count does not match the expected.

Contributor

falbrechtskirchinger May 10, 2022

Not that I know off, I just step through it with the debugger.

Contributor

falbrechtskirchinger May 10, 2022

Did I give you the wrong line, or what's going on here?

https://coveralls.io/builds/48991323/source?filename=include%2Fnlohmann%2Fdetail%2Finput%2Fbinary_reader.hpp#L2456

Line 2456 is being missed now. I'm going to compare it against the previous coverage run.

Contributor

falbrechtskirchinger May 10, 2022

No, it was definitely line 2445 before.

https://coveralls.io/builds/48971838/source?filename=include%2Fnlohmann%2Fdetail%2Finput%2Fbinary_reader.hpp#L2445

🤷‍♂️

Owner

nlohmann May 10, 2022

@falbrechtskirchinger, thanks. is there a way to print the event count? I previously try to manually count but sometime my count does not match the expected.

Do you mean something like https://github.com/doctest/doctest/blob/master/doc/markdown/logging.md#info?

Contributor

falbrechtskirchinger May 10, 2022

I guess we could add CHECK(m_event_count == -1) to ~SaxEventCountdown(). But that wouldn't solve things completely. A more advanced solution could add an enum to indicate in which event we expect the counter to reach -1 and check if that happened in the destructor. I'll add that to my to-do list. There was another PR that moved SaxEventCountdown into a separate header. Probably a good idea as well.

fangq added 2 commits

May 10, 2022 10:20


          fix sax event order

7a4914d


          fix coverage

55350c9

falbrechtskirchinger approved these changes

View reviewed changes

Contributor

falbrechtskirchinger left a comment

Looks good to me.

nlohmann approved these changes

View reviewed changes

Owner

nlohmann left a comment

Looks good to me.

nlohmann self-assigned this

nlohmann added the release item: 🐛 bug fix label

nlohmann added this to the Release 3.11.0 milestone

nlohmann linked an issue

that may be closed by this pull request

json:parse_bjdata_fuzzer reaches assertion #3475

Closed

2 tasks

nlohmann merged commit a8a547d into nlohmann:develop

Owner

nlohmann commented May 10, 2022

Thanks for fixing!

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

Labels

aspect: binary formats release item: 🐛 bug fix