Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#10747 closed defect (fixed)

IPC: server crashes if the client is killed

Reported by: alarsen Owned by: vadz
Priority: high Milestone:
Component: network Version: stable-latest
Keywords: wxTCPConnection OnDisconnect Cc:
Blocked By: Blocking:
Patch: yes

Description

Hi,
if a client holding a wxTCPConnection is abnormally terminated, the connected server crashes.
The bug can be easily reproduced with the samples; connect the client to the server, then kill the client.

The problem seems to be that the server will attempt to send an IPC_DISCONNECT via the already destroyed socket (see attached backtrace).

Cheers
Anders

Attachments (3)

baseipcserver.patch download (957 bytes) - added by alarsen 9 years ago.
samples update (call wxConnection::OnDisconnect)
backtrace.txt download (2.9 KB) - added by alarsen 9 years ago.
backtrace of baseipcserver crashing
ipc-crash.patch download (475 bytes) - added by alarsen 9 years ago.
trivial fix

Download all attachments as: .zip

Change History (11)

comment:1 follow-up: Changed 9 years ago by vadz

  • Status changed from new to accepted
  • Summary changed from wxTCPConnection crash to IPC: server asserts if the client is killed

This doesn't seem to be really a crash, ignoring the assert allows the server to continue to work but it definitely shouldn't do this nevertheless and I'll look at it soon, thanks for your testing!

Changed 9 years ago by alarsen

samples update (call wxConnection::OnDisconnect)

comment:2 in reply to: ↑ 1 Changed 9 years ago by alarsen

  • Summary changed from IPC: server asserts if the client is killed to IPC: server crashes if the client is killed

Replying to vadz:

This doesn't seem to be really a crash, ignoring the assert allows the server to continue to work but it definitely shouldn't do this nevertheless and I'll look at it soon, thanks for your testing!

Well, our application did crash, that's why I looked into it.
If you apply the attached patch to the IPC samples (making the baseipcserver call wxConnection::OnDisconnect() instead of leaving the connection object dangling) you can reproduce the crash in the baseipcserver.
I didn't manage to get neither an assert nor a backtrace from the baseipcserver, and gdb loses track of where it came from when I try to catch the crash in the debugger :(

Cheers
Anders

Changed 9 years ago by alarsen

backtrace of baseipcserver crashing

comment:3 follow-up: Changed 9 years ago by alarsen

The server application crashes when SomeDerivedConnection::OnDisconnect() calls wxTCPConnection::OnDisconnect() - as it must do according to docs - but only if the client crashed as opposed to disconnecting properly.

It seems the GUI sample does not call wxTCPConnection::OnDisconnect(), thus the confusion, sorry.

Cheers
Anders

comment:4 in reply to: ↑ 3 Changed 9 years ago by alarsen

  • Patch set

Replying to alarsen:

The server application crashes when SomeDerivedConnection::OnDisconnect() calls wxTCPConnection::OnDisconnect() - as it must do according to docs - but only if the client crashed as opposed to disconnecting properly.

A solution seems to be to not call wxTCPConnection::Disconnect() in the wxTCPConnection destructor (see attached patch).
That call looks unneccesary anyway, since the connection object is normally destroyed as a result of Disconnect() having been called before.

Cheers
Anders

Changed 9 years ago by alarsen

trivial fix

comment:5 Changed 9 years ago by alarsen

Having thought a bit more about it I believe the correct solution is to call SetConnection(false) in the event handler when wxSOCKET_LOST is received, as in the (updated) trivial patch.

Cheers
Anders

comment:6 Changed 9 years ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [60568]) don't call Disconnect() if the connection had been already lost (closes #10747)

comment:7 Changed 9 years ago by vadz

Thanks again, Anders, I do agree with your conclusion, it seems logical to call SetConnected(false) to prevent the code from trying to disconnect again. And it also makes sense to handle connection loss in exactly the same way as a normal disconnection (which is why I applied a slightly modified form of the patch enforcing this).

There is still some confusion about who should delete the connections and when but I think it's unavoidable with the current API which allows you to either call base class OnDisconnect() or not. So we probably can't do anything about it.

comment:8 Changed 9 years ago by VZ

(In [60569]) don't call Disconnect() if the connection had been already lost (closes #10747) [backport of r60568 from trunk]

Note: See TracTickets for help on using tickets.