ERROR: ThreadSanitizer: SEGV on unknown address

[abaelhe]

Description

cd ~/Downloads/json-develop/build/tests/Debug
for x in ./test-* ; do ./$x 1> /dev/null || echo $x ; done

./test-cbor_cpp11
==36891==ERROR: ThreadSanitizer: requested allocation size 0xffffffffffffffff exceeds maximum supported size of 0x10000000000
#0 operator new(unsigned long) (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x7596b)
#1 void* std::__1::__libcpp_operator_new(unsigned long) new:235 (test-cbor_cpp11:x86_64+0x10004abc4)

==36891==HINT: if you don't care about these errors you may set allocator_may_return_null=1
SUMMARY: ThreadSanitizer: allocation-size-too-big (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x7596b) in operator new(unsigned long)
Abort trap: 6
./test-cbor_cpp11

./test-testsuites_cpp11

ThreadSanitizer:DEADLYSIGNAL
==36941==ERROR: ThreadSanitizer: SEGV on unknown address 0x60000005d000 (pc 0x7ff80f562adb bp 0x7ff7b5762f10 sp 0x7ff7b5762f00 T6880876)
==36941==The signal is caused by a READ memory access.
#0 flockfile :197228464 (libsystem_c.dylib:x86_64+0x12ada)
#1 fgetc :197228464 (libsystem_c.dylib:x86_64+0x3382e)
#2 nlohmann::detail::file_input_adapter::get_character() input_adapters.hpp:56 (test-testsuites_cpp11:x86_64+0x100044ca7)
#3 nlohmann::detail::lexer<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >, nlohmann::detail::file_input_adapter>::get() lexer.hpp:1341 (test-testsuites_cpp11:x86_64+0x100044b0b)
#4 nlohmann::detail::lexer<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >, nlohmann::detail::file_input_adapter>::skip_bom() lexer.hpp:1480 (test-testsuites_cpp11:x86_64+0x100042db8)
#5 nlohmann::detail::lexer<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >, nlohmann::detail::file_input_adapter>::scan() lexer.hpp:1504 (test-testsuites_cpp11:x86_64+0x100042997)
#6 nlohmann::detail::parser<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >, nlohmann::detail::file_input_adapter>::get_token() parser.hpp:456 (test-testsuites_cpp11:x86_64+0x1000427cc)
#7 nlohmann::detail::parser<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >, nlohmann::detail::file_input_adapter>::parser(nlohmann::detail::file_input_adapter&&, std::__1::function<bool (int, nlohmann::detail::parse_event_t, nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >&)>, bool, bool) parser.hpp:73 (test-testsuites_cpp11:x86_64+0x100042738)
#8 nlohmann::detail::parser<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >, nlohmann::detail::file_input_adapter>::parser(nlohmann::detail::file_input_adapter&&, std::__1::function<bool (int, nlohmann::detail::parse_event_t, nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >&)>, bool, bool) parser.hpp:71 (test-testsuites_cpp11:x86_64+0x1000426ac)
#9 nlohmann::detail::parser<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >, nlohmann::detail::file_input_adapter> nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >::parsernlohmann::detail::file_input_adapter(nlohmann::detail::file_input_adapter, std::__1::function<bool (int, nlohmann::detail::parse_event_t, nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >&)>, bool, bool) json.hpp:169 (test-testsuites_cpp11:x86_64+0x100041e9d)
#10 nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > > nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >::parse<__sFILE*>(__sFILE*&&, std::__1::function<bool (int, nlohmann::detail::parse_event_t, nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator > >&)>, bool, bool) json.hpp:3992 (test-testsuites_cpp11:x86_64+0x100041b38)
#11 _DOCTEST_ANON_FUNC_42() unit-testsuites.cpp:389 (test-testsuites_cpp11:x86_64+0x100006be3)
#12 doctest::Context::run() doctest.h:6486 (test-testsuites_cpp11:x86_64+0x100064469)
#13 main doctest.h:6571 (test-testsuites_cpp11:x86_64+0x1000663b2)
#14 start :197228464 (dyld:x86_64+0x551d)

==36941==Register values:
rax = 0x000000010bd096ac rbx = 0x0000000000000000 rcx = 0x000000010bd096ac rdx = 0x0000000000000103
rdi = 0x0000000000000000 rsi = 0x00007ff80f556154 rbp = 0x00007ff7b5762f10 rsp = 0x00007ff7b5762f00
r8 = 0x0000000000000001 r9 = 0x000000010b925160 r10 = 0x0000000000000000 r11 = 0x0000000000000000
r12 = 0x000000010bd093a0 r13 = 0x00007ff7b5765278 r14 = 0x0000000000000002 r15 = 0x000000010bcf5010
ThreadSanitizer can not provide additional info.
SUMMARY: ThreadSanitizer: SEGV (libsystem_c.dylib:x86_64+0x12ada) in flockfile
==36941==ABORTING
Abort trap: 6
./test-testsuites_cpp11

Reproduction steps

mkdir build
cd build
cmake -G 'Xcode' ..
xcodebuild -verbose

cd tests/Debug
for x in ./test-* ; do ./$x 1> /dev/null || echo $x ; done

Expected vs. actual results

SUCCESS! for all tests

Minimal code example

No response

Error messages

No response

Compiler and operating system

Xcode 13.4.1 Build version 13F100, Clang 13.1.6 (clang-1316.0.21.2.5), macOS-12.4-x86_64-i386-64bit

Library version

json-develop-master

Validation

• The bug also occurs if the latest version from the develop branch is used.
• I can successfully compile and run the unit tests.

[falbrechtskirchinger]

Looks like you're missing the test data.

Please run the tests through ctest at least once, as that will download the test data automatically. Alternatively, download the test data from here and point JSON_TestDataDirectory to that folder.

[abaelhe]

even though the program should not expose a SEGV;-)

[falbrechtskirchinger]

Maybe. Care (enough) to submit a PR? ;-)

[nlohmann]

Trying to reproduce - where did you enable TSAN?

[falbrechtskirchinger]

@nlohmann This isn't really a TSan issue. Look at the trace. json_test_data is missing and I run into this myself quite frequently.
The unit tests need something like std::ifstream ifs(...); REQUIRE_MESSAGE(ifs.good(), "Could not open file. Did you forget to download json_test_data?"); if anyone else wants to do the work.

[nlohmann]

Thanks, I can do so - I just missed why TSAN was enabled in the first place.

[abaelhe]

@nlohmann
TSAN is a default setting for latest MacOS Development.
For Linux or other OS, you can download the latest release of LLVM Project(llvm, clang, lldb,......)

Download latest release of binary distribution or source package:
https://releases.llvm.org

[abaelhe]

@nlohmann
here the SEGV caused by missing "test data",
for final customers, This SEGV may triggered by missing configuration file, data file or any other possible file types, since our Json library is broadly used in anywhere that is possible, and the SEGV sort of severity will lead to the services/products fully unavailabe!

And we don't assume final customers can get it done at first time like you professional experts, when they facing a SIGSEGV and tons error text.

further more, we actually have encountered and resolving such SEGV issue.

[nlohmann]

The issue is accessing a FILE* after fopen failed. I think it's the job of client code to check that this does not happen. We could add an assertion though.

[abaelhe]

double check, both your Json library and the client side;
we can achieve the famous "out of the box" experience of Apple, for your Json library clients.

Why you nlohmann innovated this Json library besides bson, cjson, fastjson, simplejson, ujson, cppjson, and other famous existing Json libraries?
Clients and contributors believe we can.
make it better.

[falbrechtskirchinger]

@nlohmann here the SEGV caused by missing "test data", for final customers, This SEGV may triggered by missing configuration file, data file or any other possible file types, since our Json library is broadly used in anywhere that is possible, and the SEGV sort of severity will lead to the services/products fully unavailable!

Simple. Fix your code! The library is not responsible for mistakes by the user. We can try and make your life easier, but we can't magically solve a file being unavailable. 🤷‍♂️

And we don't assume final customers can get it done at first time like you professional experts, when they facing a SIGSEGV and tons error text.

You are supposed to be the expert. Don't hand your customers a broken product.

further more, we actually have encountered and resolving such SEGV issue.

Then you should know how to prevent it in the future, right? ;-)

[falbrechtskirchinger]

Maybe you're unaware that you can use std::ifstream instead of FILE *?
The former would result in an exception (of type json::parse_error) that you can catch instead of a segfault.

[gregmarr]

https://github.com/nlohmann/json/blob/develop/include/nlohmann/detail/input/input_adapters.hpp#L43

Would it make sense to have the file_input_adapter constructor throw if f is null?

[falbrechtskirchinger]

https://github.com/nlohmann/json/blob/develop/include/nlohmann/detail/input/input_adapters.hpp#L43

Would it make sense to have the file_input_adapter constructor throw if f is null?

Then, while we're at it, stream_input_adapter should throw as well. If possible, an empty input should produce a distinct parse error instead of 101.

[gregmarr]

Then, while we're at it, stream_input_adapter should throw as well.

I'm not as worried about something which already causes a well-defined exception, as about passing a null FILE* which we're then going to pass to fgetc, which requires a valid FILE*.

There isn't really an equivalent to a null in the input_stream_adapter constructor as it takes the stream by reference. About the only thing I can think of to check there is eof(), and that's already cleanly handled.

If possible, an empty input should produce a distinct parse error instead of 101.

That can be done once at the very beginning of the parse, rather than in the adapters, as you'd want that for a valid stream or file or string that just doesn't have any text in it.

[nlohmann]

Then, while we're at it, stream_input_adapter should throw as well. If possible, an empty input should produce a distinct parse error instead of 101.

This would be a breaking change though...

[falbrechtskirchinger]

There isn't really an equivalent to a null in the input_stream_adapter constructor as it takes the stream by reference. About the only thing I can think of to check there is eof(), and that's already cleanly handled.

I was thinking of good(). This is really what the user should be doing to determine if a file was correctly opened. I dislike that there isn't a clear error in case this is missed, though.

If possible, an empty input should produce a distinct parse error instead of 101.

That can be done once at the very beginning of the parse, rather than in the adapters, as you'd want that for a valid stream or file or string that just doesn't have any text in it.

Yes. It's a separate issue, but nonetheless, something that's been bothering me lately. It's also kind of a breaking change.

[falbrechtskirchinger]

Then, while we're at it, stream_input_adapter should throw as well. If possible, an empty input should produce a distinct parse error instead of 101.

This would be a breaking change though...

Yes. 4.0 …

[nlohmann]

Without touching other input adapters, I would like to focus on the case of using file_input_adapter with a null pointer.

We could do the following:

1. Add an assertion assert(f != nullptr) to the constructor.
2. Throw an exception when nullptr is passed to the constructor.
3. Guard the std::fgetc call with an if and return eof() in case the adapter was created with a null pointer.

I am currently leaning to (1), together with more explicit documentation that the FILE* pointer must not be null.

What do you think?

[gregmarr]

Did you mean exception rather than assertion in 2?

I thought about option 3, but really didn't want to add the overhead, so I think a one-time check in the constructor is the way to go. Whether it's an assert or an exception doesn't make much difference in general, though the exception will also prevent issues in release builds with asserts turned off.

[falbrechtskirchinger]

I'd favor option 2 with an ID that can be re-used in other adapters. Otherwise, option 1.

[nlohmann]

Please have a look at #3593 where an exception is thrown if a passed file pointer is null.

[abaelhe]

Please have a look at #3593 where an exception is thrown if a passed file pointer is null.

you made it ;-)