Bug 4213 - Slimserver can crash due to display being deleted from client object at forget time
: Slimserver can crash due to display being deleted from client object at forge...
Status: CLOSED FIXED
Product: Logitech Media Server
Classification: Unclassified
Component: Misc
: 6.5.0
: PC All
: P2 normal (vote)
: ---
Assigned To: Andy Grundman
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-24 10:17 UTC by Adrian Smith
Modified: 2008-12-18 11:12 UTC (History)
2 users (show)

See Also:
Category: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Smith 2006-09-24 10:17:22 UTC
Issue is believed to occur because Slim::Display::Display::forgetClient is called at forgetClient time and removes the display from the client object.  

Although forgetClient removes the client from the client hash, it does not remove references to it from Slimproto.pm and does not close slimproto sockets.  Hence it is possible for subsequent slimproto traffic to cause client methods to be called.  If these relate to the display the server is likely to crash as $client->display is undef.

Short term fix is to avoid setting $client->display to undef at forgetClient time.  Will add this to 6.5 and trunk.

Long term need to consider actively closing slimproto sockets and deleting state within forgetClient?
Comment 1 Adrian Smith 2006-09-24 10:24:40 UTC
Short term fix added to 6.5 and trunk [10018 & 100019]
Comment 2 Chris Owens 2006-09-26 09:36:51 UTC
Dan, what target would you like me to set this for?
Comment 3 Dan Sully 2006-09-26 10:52:56 UTC
Adrian - is there a better fix for this? Or is this what's going into 6.5.1?

Thanks
Comment 4 Adrian Smith 2006-09-26 11:06:34 UTC
I think its fine as is for 6.5.1 as it should not crash now due to $client->display undef issues.

I believe the root cause is that $client->forgetClient does not completely forget the client.  In particular it does not clear out any slimproto state.  Slimproto relies on timeouts or errors from select to clear up state, prior to this it can cause client methods to be called on clients which have already been forgotten.  So at present if forgetClient removes state, there is a risk that subsequent slimproto packets could cause unexpected effects.  The crash due to $client->display undef being the one we noticed!

So for 7.0 I think we need forgetClient to really forget the client.  However fixing for 6.5 is probably a bit risky as Andy and I spent too long messing with slimproto already and any changes need thinking though to avoid side effects.

ccing Andy as he probably has a view.


Comment 5 Dan Sully 2006-09-26 21:15:08 UTC
Ok - retargetting for 7.0 and assigning to Andy for now.
Comment 6 Andy Grundman 2007-10-23 10:45:34 UTC
This is fixed now right?  forgetClient really forgets everything about the player.
Comment 7 Adrian Smith 2007-10-28 05:43:36 UTC
I think comment #4 still holds, the client object is not destroyed until Slimproto.pm removes it it from %sock2client.  It not broken at present so proabaly leave it, but I'm unconvinced that it removes the client when we think and so there is still the possibility to receive and process slimproto updates after we think the client is forgotten.
Comment 8 Andy Grundman 2007-11-07 11:24:08 UTC
OK we definitely have a memory leak here, client objects are never destroyed until the server shuts down.  Going to be tough to track down...
Comment 9 Andy Grundman 2007-11-07 12:48:31 UTC
Got it!  Change 14472.
Comment 10 Chris Owens 2008-03-07 09:03:09 UTC
This bug is being closed since it was resolved for a version which is now released!  Please download the new version of SqueezeCenter (formerly SlimServer) at http://www.slimdevices.com/su_downloads.html

If you are still seeing this bug, please re-open it and we will consider it for a future release.