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

Jump to bottom

Memory corruption in 1.11.0-beta2 on Android NDK in multiplayer servers list #8799

Closed
pelya opened this issue Mar 3, 2021 · 9 comments · Fixed by #8801
Closed

Memory corruption in 1.11.0-beta2 on Android NDK in multiplayer servers list #8799

pelya opened this issue Mar 3, 2021 · 9 comments · Fixed by #8801

Comments

@pelya
Copy link

pelya commented Mar 3, 2021

Version of OpenTTD

1.11.0-beta2, built with Android NDK arm64.

Expected result

OpenTTD does not crash

Actual result

OpenTTD crashes

Steps to reproduce

Run OpenTTD, click 'Multiplayer', click 'Search internet'. OpenTTD crashes with 70% probability.
Reproduced when OpenTTD is built with clang from Android NDK for arm64 architecture, not reproduced on GCC x86_64.
Please check it with Valgrind or something, I suspect the problem is because of different C++ standard library.
@LordAro
Copy link
Member

LordAro commented Mar 3, 2021 •
edited

Since this is a (somewhat) unsupported platform that you're compiling yourself, perhaps you can help by bisecting to find which commit introduced the problem?

Perhaps it could also be reproduced on a "standard" system by compiling with Clang's stdlib, libc++ which may be what's being used here. Using CXXFLAGS="-stdlib=libc++" when running cmake should be enough to force that

@pelya
Copy link
Author

pelya commented Mar 3, 2021 via email

@TrueBrain
Copy link
Member

Also if you have any more indication besides "crashing", that would be of great help. A crash.log, a traceback, anything more, could help us understand better what is going on with Android NDK.

That said, I cannot reproduce this problem with clang on x86_64.

@orudge
Copy link
Contributor

orudge commented Mar 3, 2021

I can reproduce this on both ARM64 and Intel on Mac - attached is an arm64 log, but it's happening on my x86_64 MacBook Pro too:

crash-apple.txt
crash.log

@LordAro
Copy link
Member

LordAro commented Mar 3, 2021

That basically confirms libc++ is to blame, macOS will be using it for all platforms

@TrueBrain
Copy link
Member

On Linux, after installing libc++ (and the abi), you can reproduce this with:

CXXFLAGS="-stdlib=libc++ -I/usr/lib/llvm-11/include/c++/v1/" CC=clang CXX=clang++ cmake ..
(in a clean build folder!).

More detailed crashlog:

Thread 1 "openttd" received signal SIGSEGV, Segmentation fault.
0x00000000009587cc in StrEmpty (s=0x211d <error: Cannot access memory at address 0x211d>) at OpenTTD/src/network/core/../../string_func.h:62
62              return s == nullptr || s[0] == '\0';
(gdb) bt
#0  0x00000000009587cc in StrEmpty (s=0x211d <error: Cannot access memory at address 0x211d>) at OpenTTD/src/network/core/../../string_func.h:62
#1  0x0000000000959425 in NetworkGameWindow::NGameAllowedSorter (a=@0x33a89d8: 0x2011, b=@0x33a8938: 0x3355900) at OpenTTD/src/network/network_gui.cpp:326
#2  0x000000000095e59a in GUIList<NetworkGameList*, StringFilter&>::Sort<bool (*)(NetworkGameList* const&, NetworkGameList* const&)>(bool (*)(NetworkGameList* const&, NetworkGameList* const&))::{lambda(NetworkGameList* const&, NetworkGameList* const&)#1}::operator()(NetworkGameList* const&, NetworkGameList* const&) const (this=0x7fffffffcbc0, a=@0x33a89d8: 0x2011, b=@0x33a8938: 0x3355900) at OpenTTD/src/network/../sortlist_type.h:261

@JGRennison
Copy link
Contributor

OpenTTD/src/network/network_gui.cpp

Line 336 in 73fd634

return (r != 0) ? r < 0 : !NGameClientSorter(a, b);
looks dubious as this could return true for an element when compared with itself, which breaks partial ordering. This can cause std::sort to fall over, similar to #7838.
NGameClientSorter(b, a) would probably be better.

@TrueBrain TrueBrain mentioned this issue Mar 3, 2021
Merged
@TrueBrain
Copy link
Member

Spot on @JGRennison .

And to be clear, this was not memory corruption; this is reading a vector out-of-bounds. Slight nuance :)

LordAro pushed a commit that referenced this issue Mar 3, 2021
@TrueBrain
Fix #8799: NGameAllowedSorter() is not imposing strict weak ordering …
0243ae4 
…relation (#8801)

In other words, it should only (!) return true if A comes for B.
This promise was broken for the situation where two values are
identical. It would return true in these cases too. This is of
course not possible: if two values are identical, neither come
before the other. As such, the sorter was not imposing strict
weak ordering relations.

libstdc++ handled this scenario just fine, but libc++ crashes
badly on this, as it allowed comparing of [begin, end] instead
of [begin, end).
libc++ considered this not a bug (and by specs, they are correct;
just this way of crashing is of course a bit harsh):
https://bugs.llvm.org/show_bug.cgi?id=47903
@pelya
Copy link
Author

pelya commented Mar 3, 2021 via email

@LordAro LordAro mentioned this issue Mar 6, 2021
Closed
@TrueBrain TrueBrain mentioned this issue Mar 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix: NGameAllowedSorter() is not imposing strict weak ordering relation TrueBrain/OpenTTD
5 participants
@pelya @TrueBrain @JGRennison @LordAro @orudge