Segfault in MacOS cgame DLL (Apple clang 10.0.1)

[illwieckz]

When running dll cgame on macOS compiled with Apple clang, a crash occurs in FS::Path::StripExtension function.

The bug seems to occur when doing a substring with return std::string(path.begin(), p - 1); but does not occur if a logger is added within the loop containing this return:

std::string StripExtension(Str::StringRef path)
{
	if (path.empty())
		return "";
	if (path.back() == '/')
		return path.substr(path.size() - 1);

	// Find a dot or slash, searching from the end of the string
	for (const char* p = path.end(); p != path.begin(); p--) {
+		Log::Warn(":p %c", p);
		if (p[-1] == '/')
			return path;
		if (p[-1] == '.')
			return std::string(path.begin(), p - 1);
	}
	return path;
}

Extensive lldb log:

]cgame-native-dll.so was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 3479 stopped
* thread #9, stop reason = EXC_BAD_ACCESS (code=1, address=0x700009747000)
    frame #0: 0x000000013bb6f300 cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::char_traits<char>::assign(__c1=0x0000700009736259, __c2=<unavailable>) at __string:208:73 [opt]
   205 	    typedef mbstate_t state_type;
   206 	
   207 	    static inline _LIBCPP_CONSTEXPR_AFTER_CXX14
-> 208 	    void assign(char_type& __c1, const char_type& __c2) _NOEXCEPT {__c1 = __c2;}
    	                                                                        ^
   209 	    static inline _LIBCPP_CONSTEXPR bool eq(char_type __c1, char_type __c2) _NOEXCEPT
   210 	            {return __c1 == __c2;}
   211 	    static inline _LIBCPP_CONSTEXPR bool lt(char_type __c1, char_type __c2) _NOEXCEPT
Target 0: (daemon) stopped.
thread backtrace
* thread #9, stop reason = EXC_BAD_ACCESS (code=1, address=0x700009747000)
  * frame #0: 0x000000013bb6f300 cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::char_traits<char>::assign(__c1=0x0000700009736259, __c2=<unavailable>) at __string:208:73 [opt]
    frame #1: 0x000000013bb6f285 cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::enable_if<__is_forward_iterator<char const*>::value, void>::type std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__init<char const*>(this=<unavailable>, __first=<unavailable>, __last=<unavailable>) at string:2051 [opt]
    frame #2: 0x000000013bb6f214 cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string<char const*>(this=<unavailable>, __first=<unavailable>, __last=<unavailable>) at string:2060 [opt]
    frame #3: 0x000000013bb6f1fd cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string<char const*>(this=<unavailable>, __first=<unavailable>, __last=<unavailable>) at string:2059 [opt]
    frame #4: 0x000000013bb6f1fd cgame-native-dll.so`FS::Path::StripExtension(path=<unavailable>) at FileSystem.cpp:531 [opt]
    frame #5: 0x000000013bb10de2 cgame-native-dll.so`BG_LoadEmoticons() at bg_misc.cpp:2544:22 [opt]
frame select 0
frame #0: 0x000000013bb6f300 cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::char_traits<char>::assign(__c1=0x0000700009736259, __c2=<unavailable>) at __string:208:73 [opt]
   205 	    typedef mbstate_t state_type;
   206 	
   207 	    static inline _LIBCPP_CONSTEXPR_AFTER_CXX14
-> 208 	    void assign(char_type& __c1, const char_type& __c2) _NOEXCEPT {__c1 = __c2;}
    	                                                                        ^
   209 	    static inline _LIBCPP_CONSTEXPR bool eq(char_type __c1, char_type __c2) _NOEXCEPT
   210 	            {return __c1 == __c2;}
   211 	    static inline _LIBCPP_CONSTEXPR bool lt(char_type __c1, char_type __c2) _NOEXCEPT
frame variable
(std::__1::char_traits<char>::char_type &) __c1 = 0x0000700009736259 (&__c1 = 'g')
(const std::__1::char_traits<char>::char_type &) __c2 = <no location, value may have been optimized out>

frame select 1
frame #1: 0x000000013bb6f285 cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::enable_if<__is_forward_iterator<char const*>::value, void>::type std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__init<char const*>(this=<unavailable>, __first=<unavailable>, __last=<unavailable>) at string:2051 [opt]
   2048	        __set_long_size(__sz);
   2049	    }
   2050	    for (; __first != __last; ++__first, (void) ++__p)
-> 2051	        traits_type::assign(*__p, *__first);
   2052	    traits_type::assign(*__p, value_type());
   2053	}
   2054	
frame select 2
frame #2: 0x000000013bb6f214 cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string<char const*>(this=<unavailable>, __first=<unavailable>, __last=<unavailable>) at string:2060 [opt]
   2057	inline _LIBCPP_INLINE_VISIBILITY
   2058	basic_string<_CharT, _Traits, _Allocator>::basic_string(_InputIterator __first, _InputIterator __last)
   2059	{
-> 2060	    __init(__first, __last);
   2061	#if _LIBCPP_DEBUG_LEVEL >= 2
   2062	    __get_db()->__insert_c(this);
   2063	#endif
frame select 3
frame #3: 0x000000013bb6f1fd cgame-native-dll.so`FS::Path::StripExtension(Str::BasicStringRef<char>) [inlined] std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string<char const*>(this=<unavailable>, __first=<unavailable>, __last=<unavailable>) at string:2059 [opt]
   2056	template<class _InputIterator>
   2057	inline _LIBCPP_INLINE_VISIBILITY
   2058	basic_string<_CharT, _Traits, _Allocator>::basic_string(_InputIterator __first, _InputIterator __last)
-> 2059	{
   2060	    __init(__first, __last);
   2061	#if _LIBCPP_DEBUG_LEVEL >= 2
   2062	    __get_db()->__insert_c(this);
frame select 4
frame #4: 0x000000013bb6f1fd cgame-native-dll.so`FS::Path::StripExtension(path=<unavailable>) at FileSystem.cpp:531 [opt]
   528 			if (p[-1] == '/')
   529 				return path;
   530 			if (p[-1] == '.')
-> 531 				return std::string(path.begin(), p - 1);
   532 		}
   533 		return path;
   534 	}
frame variable
(Str::StringRef) path = <no location, value may have been optimized out>

(const char *) p = <no location, value may have been optimized out>

Note: for some reasons the game does not compile as Debug build on my macOS dev environnement (cmake even fails to configure the compiler to begin with), so I only test with RelWithDebInfo builds.

---

Moved from https://github.com/Unvanquished/Unvanquished/issues/1659

[illwieckz]

On https://www.cplusplus.com/reference/string/string/string/ I read that constructor:

string (const char* s, size_t n);

But we do:

std::string(path.begin(), p - 1);

With p being an iterator iterated from path.end():

for (const char* p = path.end(); p != path.begin(); p--)

So p does not look like to be a size but a pointer, right?

[illwieckz]

On the other hand that page also provides this example, that looks similar to what we do:

std::string s7 (s0.begin(), s0.begin()+7);

Edit: I missed that constructor in the page, which looks to be what we use:

template <class InputIterator>
  string  (InputIterator first, InputIterator last);

[illwieckz]

Maybe macOS compiler interprets const char* p as a number (then a size) instead of an iterator and then selects the wrong constructor?

[illwieckz]

This non-ambiguous code seems to work on macOS:

return std::string(path.begin(), size_t(p - 1 - path.begin()));

And it strips extensions properly:

Warn: filename: dragoon.crn 
Warn: name: dragoon 
Warn: filename: medstat.crn 
Warn: name: medstat 

It also works on Linux.

[slipher]

size_t is NOT implicitly convertible from a pointer. The (const char*, size_t) constructor cannot be called with a pointer as the second argument.

Seeing that the results are affected by adding a printf, it looks like a typical memory corruption bug, where the stack or heap gets corrupted at some point. And then at some random later point depending on exact memory layout it may crash (or not). The bug is unlikely to be inside the StripExtension function.

[illwieckz]

Well, I'm lost with that issue, and my experience of macOS is that when it crashes the debug data is often meaningless. And to support what you says, on macOS it's more likely to crash where the issue is not than on Linux. It's also very common to get the crash far deep in some macOS subsystem like in #563 and in a context unrelated to the root bug, and I have seen many similar issues when porting NetRadiant to macOS where crashes happened far deep in Apple internals and stacktraces were unrelated to the code I commented out to workaround the bugs…

One interesting thing is that if I do:

return std::string(path.begin(), size_t(p - 1));

Instead of:

return std::string(path.begin(), p - 1);

The code compiles without warning but then the game crashes with out of memory error (which is not the same error as the original issue).

Edit: the error when doing size_t(p - 1):

Loaded VM module in 253 msec
daemon(7769,0x700006b76000) malloc: can't allocate region
*** mach_vm_map(size=123145414934528) failed (error code=3)
daemon(7769,0x700006b76000) malloc: *** set a breakpoint in malloc_error_break to debug
Warn: filename: granger.crn
Warn: Error during initialization: CGame VM: Out of memory

[illwieckz]

Note that adding log printfs before the for loop has no effect (same crash), adding a log printf in the for loop has an effect (no crash).

[necessarily-equal]

Maybe we should change the title to hint at a memory corruption instead?

[slipher]

I can reproduce misbehavior around the same spot. On my machine it manifests as

Warn: Conflicting emoticon images found: emoticons/hive.crn and emoticons/hive.crn 
Warn: Conflicting emoticon images found: emoticons/hovel.crn and emoticons/hovel.crn 
Warn: Conflicting emoticon images found: emoticons/bot.crn and emoticons/bot.crn 
Warn: Conflicting emoticon images found: emoticons/overmind.crn and emoticons/overmind.crn 
Warn: Conflicting emoticon images found: emoticons/grenade.crn and emoticons/grenade.crn 
Warn: Conflicting emoticon images found: emoticons/prifle.crn and emoticons/prifle.crn 
Warn: Conflicting emoticon images found: emoticons/tesla.crn and emoticons/tesla.crn 
Warn: Conflicting emoticon images found: emoticons/dev.crn and emoticons/dev.crn 
Warn: Conflicting emoticon images found: emoticons/acidtube.crn and emoticons/acidtube.crn 
Warn: Conflicting emoticon images found: emoticons/cross.crn and emoticons/cross.crn [further messages like this will be suppressed] 

Debugging now.

[slipher]

Never mind, the above log is arising from a different problem: the cgame DLL doesn't get unloaded upon cgame restart. Although that is something with a lot of potential to cause undefined behaviors...

[slipher]

So I guess I can't reproduce anything on my machine (Clang 13.0.0), but it may be worth checking whether #577 makes it go away.