bug in OverbyteIcsWSocketS for 9.5 stable?

[merijnb 4]

{ user may check and close connection in event handler, log stuff, etc  }
    TriggerClientConnect(Client, Error);
    { The event handler may have destroyed the client ! }
    if FClientList.IndexOf(Client) < 0 then begin
        Client.Free;      { V9.5 }
        Exit;
    end;

This is a snippet from OverbyteIcsWSocketS from the method TCustomWSocketServer.TriggerSessionAvailable.

 

The line Client.Free is added in 9.5 (as seen in the comments for that line).

 

Was this intended? If the client actually is freed in TriggerClientConnect(), it is removed from FClientList (in TCustomWSocketServer.Notification), which means this call op Client.Free will be on an object which is already freed.

[Angus Robertson 710]

I agree the comment and code don't seem to make sense, but have you actually reproduced a problem?  

 

I did make major changes to the event of which that code is an extract, which has a several client.free lines for errors, some protected by if assigned, probably need more.  

 

One of my web servers has been running for 270 hours serving 740,000 client sessions, without any errors, seems stable to me.  But maybe I don't destroy the client in the event.  

 

Angus

 

[Daniel van der Lei 0]

Yes, this causes issues, in our case we sometimes deliberately free the client in the event as part of a IP blocking logic. 

 

[Angus Robertson 710]

The changes I made in that unit were explicitly for blocking IPs, using the new GEO databases included with ICS.  

 

You should use the new OnClientAcceptFilter event to check IPs you want to reject, and the connection is never accepted, so no client to close, very efficient firewall

 

My main web server has rejected 63,000 connections in the last 24 hours, all different IP addresses but from my Chinese hacker. 

 

I've improved the error handling already but won't be in SVN until next week. 

 

Angus

 

[Daniel van der Lei 0]

Thank you for pointing that out, we'll definitely have a look at that new event.