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

Fix: FreeBSD crash on multiplayer #9728

Closed
wants to merge 1 commit into from
Closed

Fix: FreeBSD crash on multiplayer #9728

wants to merge 1 commit into from

Conversation

RealDeuce
Copy link

On FreeBSD at least, FD_SET(-1, ptr); either causes a segfault or
writes outside of the bounds of ptr. Since -1 is returned when
socket() fails, this is always in valid and can never be read or
written, so always return false in this case.

Motivation / Problem

Fixes crash on (at least) FreeBSD

Description

Thread 7 received signal SIGBUS, Bus error.
Address not present.
[Switching to LWP 100805 of process 32847]
0x0000000000bb44b4 in NetworkTCPSocketHandler::CanSendReceive (this=0x807fcc488) at /usr/ports/games/openttd/work/openttd-12.1/src/network/core/tcp.cpp:226
226		FD_SET(this->sock, &read_fd);
(gdb) print this->sock
$1 = -1
(gdb) bt
#0  0x0000000000bb44b4 in NetworkTCPSocketHandler::CanSendReceive (this=0x807fcc488)
    at /usr/ports/games/openttd/work/openttd-12.1/src/network/core/tcp.cpp:226
#1  0x0000000000c318a6 in QueryNetworkGameSocketHandler::Receive (this=0x807fcc480)
    at /usr/ports/games/openttd/work/openttd-12.1/src/network/network_query.cpp:53
#2  0x0000000000c31ca1 in QueryNetworkGameSocketHandler::SendReceive ()
    at /usr/ports/games/openttd/work/openttd-12.1/src/network/network_query.cpp:147
#3  0x0000000000bd3f61 in NetworkBackgroundLoop ()
    at /usr/ports/games/openttd/work/openttd-12.1/src/network/network.cpp:1027
#4  0x000000000104e9d9 in GameLoop () at /usr/ports/games/openttd/work/openttd-12.1/src/openttd.cpp:1454
#5  0x0000000000da9d5e in VideoDriver::GameLoop (this=0x803694c00)
    at /usr/ports/games/openttd/work/openttd-12.1/src/video/video_driver.cpp:37
#6  0x0000000000da9dcb in VideoDriver::GameThread (this=0x803694c00)
    at /usr/ports/games/openttd/work/openttd-12.1/src/video/video_driver.cpp:44
#7  0x0000000000da9f05 in VideoDriver::GameThreadThunk (drv=0x803694c00)
    at /usr/ports/games/openttd/work/openttd-12.1/src/video/video_driver.cpp:81
#8  0x0000000000daba14 in StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::__1::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}::operator()(char const*, void (*&&)(VideoDriver*), VideoDriver*&&) const (this=0x82ac66fa0, name=0x5e2bcd "ottd:game", 
    F=@0x82ac66fb0: 0xda9ef0 <VideoDriver::GameThreadThunk(VideoDriver*)>, A=@0x82ac66fb8: 0x803694c00)
    at /usr/ports/games/openttd/work/openttd-12.1/src/video/../thread.h:65
#9  0x0000000000dab8cf in std::__1::__invoke<StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::__1::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}, char const*, void (*)(VideoDriver*), VideoDriver*> (__f=..., __args=@0x82ac66fb8: 0x803694c00, 
    __args=@0x82ac66fb8: 0x803694c00, __args=@0x82ac66fb8: 0x803694c00) at /usr/include/c++/v1/type_traits:3539
#10 0x0000000000dab830 in std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::__1::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}, char const*, void (*)(VideoDriver*), VideoDriver*, 2ul, 3ul, 4ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::__1::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}, char const*, void (*)(VideoDriver*), VideoDriver*>&, std::__1::__tuple_indices<2ul, 3ul, 4ul>)
    (__t=...) at /usr/include/c++/v1/thread:273
#11 0x0000000000dab486 in std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, StartNewThread<void (*)(VideoDriver*), VideoDriver*>(std::__1::thread*, char const*, void (*&&)(VideoDriver*), VideoDriver*&&)::{lambda(char const*, void (*&&)(VideoDriver*), VideoDriver*&&)#1}, char const*, void (*)(VideoDriver*), VideoDriver*> >(void*) (__vp=0x82ac66fa0)
    at /usr/include/c++/v1/thread:284
#12 0x0000000801cbcfac in ?? () from /lib/libthr.so.3
#13 0x0000000000000000 in ?? ()
Backtrace stopped: Cannot access memory at address 0x7fffdf5f9000

Limitations

None

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@RealDeuce
Explicitly disallow socket descriptor of -1
55d0e62 
On FreeBSD at least, FD_SET(-1, ptr); either causes a segfault or
writes outside of the bounds of ptr.  Since -1 is returned when
socket() fails, this is always in valid and can never be read or
written, so always return false in this case.
@RealDeuce RealDeuce changed the title Explicitly disallow socket descriptor of -1 Fix: FreeBSD crash on multiplayer Dec 4, 2021
@RealDeuce RealDeuce changed the title Fix: FreeBSD crash on multiplayer Fix: [NET] FreeBSD crash on multiplayer Dec 4, 2021
@RealDeuce RealDeuce changed the title Fix: [NET] FreeBSD crash on multiplayer Fix: FreeBSD crash on multiplayer Dec 4, 2021
@TrueBrain
Copy link
Member

I do not think this is the correct solution for the bug you encounter. We should add an assert() in that function to ensure this->sock isn't INVALID_SOCKET, as when CanSendReceive() is called after CloseSocket() we have another logic issue going on in our code; returning false is just a patch, but doesn't actually solve the real problem, I think.

I have a hard time seeing a code-flow that could currently result in the above backtrace. You also don't mention how to reproduce this problem: could you indicate how you trigger this bug? As it should happen on any OS, just most OSes don't have issues with it, I guess ;)

Basically, this->sock should only become INVALID_SOCKET after the QueryNetworkGameSocketHandler instance is removed from QueryNetworkGameSocketHandler::queries (CloseSocket() is called on dtor). But at that point it should no longer be possible that Receive() is called on the object. So there is something else going on here.

@TrueBrain TrueBrain mentioned this pull request Dec 4, 2021
Merged
@TrueBrain
Copy link
Member

I created a PR to validate some cases where this->sock would be INVALID_SOCKET in an unexpected situation. Could you maybe, next to indicate how you managed to trigger this problem, try #9729 and see if that triggers an assert ? Would be most helpful, tnx :)

@RealDeuce
Copy link
Author

I started OpenTTD, then clicked on Multiplayer, then it crashed.

@TrueBrain
Copy link
Member

Owh, another piece of information that could be really useful to debug this, is if you can run OpenTTD with -dnet=6 and paste the output here.

But I am mostly curious if my PR triggers an assert .. would be nice. As this might be another FreeBSD mismatch :)

@RealDeuce
Copy link
Author

Owh, another piece of information that could be really useful to debug this, is if you can run OpenTTD with -dnet=6 and paste the output here.

This is with my fix (so no crash)

openttd -dnet=6
dbg: [net] Starting network
dbg: [net] Initializing UDP listeners
dbg: [net] Network online, multiplayer available
dbg: [net] Detected broadcast addresses:
dbg: [net]   0) 192.168.0.255
fluidsynth: error: fluid_is_soundfont(): fopen() failed: 'File does not exist.'
fluidsynth: error: fluid_is_soundfont(): fopen() failed: 'File does not exist.'
fluidsynth: error: fluid_is_soundfont(): fopen() failed: 'File does not exist.'
fluidsynth: error: fluid_is_soundfont(): fopen() failed: 'File does not exist.'
fluidsynth: error: fluid_is_soundfont(): fopen() failed: 'File does not exist.'
fluidsynth: error: fluid_is_soundfont(): fopen() failed: 'File does not exist.'
fluidsynth: error: fluid_is_soundfont(): fopen() failed: 'File does not exist.'
dbg: [net] 44.135.218.201:3979 resolved in:
dbg: [net] - 44.135.218.201:3979 (IPv4)
dbg: [net] Attempting to connect to 44.135.218.201:3979 (IPv4)
dbg: [net] dragon.rrx.ca:3979 resolved in:
dbg: [net] - [2602:80b:7007::65]:3979 (IPv6)
dbg: [net] - 44.31.37.65:3979 (IPv4)
dbg: [net] Attempting to connect to [2602:80b:7007::65]:3979 (IPv6)
dbg: [net] Attempting to connect to 44.31.37.65:3979 (IPv4)
dbg: [net] Could not connect to [2602:80b:7007::65]:3979 (IPv6): Connection refused
dbg: [net] Connected to dragon.rrx.ca:3979
dbg: [net] Failed to get address of the peer: Bad file descriptor
dbg: [net] - using [::]:0 (IPv6)
dbg: [net] Timeout while connecting to 44.135.218.201:3979
dbg: [net] Closed UDP listeners
dbg: [net] Initializing UDP listeners
dbg: [net] Closed UDP listeners
dbg: [net] Shutting down network

@TrueBrain TrueBrain mentioned this pull request Dec 4, 2021
Closed
@TrueBrain
Copy link
Member

Right, indeed it is actually another problem that causes this issue in the end. I created #9730 to track this issue, so we can resolve it properly. Sadly, in GitHub you cannot convert a Pull Request to an issue. One of those quirks :)

This PR patches up the crash, but doesn't actually addresses the real problem. Additionally, it causes a memory leak, as every time this happens the attached objects are not destroyed. So I am going to close this PR, and let's figure out what actually goes wrong :)

Tnx for the info, and let's continue in #9730 :)

@TrueBrain TrueBrain closed this Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@RealDeuce @TrueBrain