ScktComp.TCustomWinSocket Force close fails to use local variable

[JackTrapper]

Tested

• Delphi 10.3: Fails
• Delphi 10.4: Fails

Background

TCustomWinSocket has two Close methods:

• Close: the normal, safe way to close a socket
• Close(True): the dangerous, unsafe, way to forcibly close a socket.

The force close method comes with a huge warning:

If ForceClosed = True, then perform an immediate, possibly dangerous and disorderly shutdown of the communications channel.
If ForceClosed = False, the just call Close above.

NOTE: When ForceClosed = True, NO LOCKING OF THE COMPONENT IS DONE, which may introduce subtle race-conditions in multi-threaded environments. Use this only in the event of possible dead-peers or other catastrophic conditions. Using ForceClosed = True may also leave the component in an indeteminate state. It is recommended that the componenet be destroyed after calling this method.

Inside the force close method, it uses an AtomicExchange to assign two member variables FLookupHandle and FSocket into local stack variables, and replace the member varialbes with nil and INVALID_SOCKET:

System.Win.ScktComp.pas:

procedure TCustomWinSocket.Close(ForceClosed: Boolean);
var
   lookupHandle: THandle;
begin
   //...snip...
   lookupHandle := THandle(AtomicExchange(Pointer(FLookupHandle), nil));
   //...snip...
  end;

So far so good. It's the moral equivalent of:

    LookupHandle := FLookupHandle;
    FLookupHandle := nil;

The bug is that it then tries to operate on the FLookupHandle rather than the local lookupHandle variable:

    //Swap FLookupHandle into local variable.
    LookupHandle := THandle(AtomicExchange(Pointer(FLookupHandle), nil));
    if LookupHandle <> 0 then
      CheckSocketResult(WSACancelASyncRequest(FLookupHandle), 'WSACancelASyncRequest');  <--- mistakenly uses FLookupHandle rather than LookupHandle

The fix is to change:

• Before: WSACancelAsyncRequest(FLookupHandle);
• After: WSACancelAsyncRequest(lookupHandle);

Making the full method:

System.Win.ScktComp.pas:

procedure TCustomWinSocket.Close(ForceClosed: Boolean);
var
  lookupHandle: THandle;
  socketHandle: TSocket;
begin
  if not ForceClosed then
  begin
    Close;
    Exit;
  end;

  lookupHandle := THandle(AtomicExchange(Pointer(FLookupHandle), nil));
  if lookupHandle <> 0 then
    CheckSocketResult(WSACancelASyncRequest(lookupHandle), 'WSACancelASyncRequest'); //FIXFIX: use the local stack variable, and not FLookupHandle

  socketHandle := TSocket(AtomicExchange(Pointer(FSocket), Pointer(INVALID_SOCKET)));
  if (socketHandle <> INVALID_SOCKET) then
    CheckSocketResult(closesocket(socketHandle), 'closesocket');
end;

We also adopt the modern convention of camelCase for local variables; to help us distinguish them from member variables, methods, or properties.