Bug: HTTPTunnel and Socks servers can't have non-IP address

[Fr0sT.Brutal 900]

HTTP Tunnel and Socks server addresses can not have literal address (including 'localhost') because IPv6 check in `procedure TCustom*WSocket.Connect` throws 'Unsupported address format'. I suppose this check should only test for unavailable IPv6 case (i.e. address is IPv6 but OS doesn't support it).

[Angus Robertson 574]

Not looked closely, but in ICS setting SocketFamily to sfAny is the common way to avoid IP address checks.  

 

But there were some changes by a contributor in V8.56 to support IPv6 with proxies, so maybe something got broken.

 

Angus

 

[Fr0sT.Brutal 900]

 

Angus, seems it got.

    { V8.56 IPv6 support from Max Terentiev }
    if WSocketIsIP(FSocksServer, LSocketFamily) then begin // is socks proxy IP is IPv6 ?
        if (LSocketFamily = sfIPv4) or (IsIPv6APIAvailable) then
            FSocketFamily := LSocketFamily
        else
            FSocketFamily := DefaultSocketFamily;
      end
      else
         raise ESocketException.Create('Unsupported socks address format');

Current value of FSocketFamily is not considered

[Angus Robertson 574]

I suspect those two checks are unnecessary, should I just remove them?   Or will it break use of IP addresses?

 

Angus

 

[Fr0sT.Brutal 900]

I suspect the logic here should be
 

If address is IP then
  if IP is v4 or v6 available then
    Ffamily := Lfamily
  else
    Error('IPv6 not supported')

  so that it 1) assigns proper FFamily and 2) checks for v6 availability.

[Angus Robertson 574]

Unfortunately the developer that added SocketFamily and IPv6 never documented it's intended external use.  I've been adding SocketFamily into more components and samples recently now I have IPv6 on all my local and hosted servers, firewall support for IPv6 is in it's infancy and stopped IPv6 working for a long time. 

 

I've been finding DNS lookups may offer an IPv6 address which then fails to connect for various reasons, so you need the ensure SocketFamily is set to sfIPv4 to the alternate IPv4 is used instead.   But that is all done automatically, so not sure that family checking is necessary for proxies, if it's done later anyway.

 

Angus

 

[Fr0sT.Brutal 900]

I've never dealt with IPv6 but for me the purpose of this piece а code is to 1) assign v6 family from v6 IP address so that a user doesn't have to specify the family manually and 2) if IP is v6 check that OS supports it. And I'm not sure whether 2) is necessary, maybe it would fail later anyway on address resolution.

[Angus Robertson 574]

I've asked the author to comment.  The Connect methods are inherited, often something happens where you don't expect it.

 

Angus

 

[Fr0sT.Brutal 900]

 Source/OverbyteIcsWSocket.pas | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/Source/OverbyteIcsWSocket.pas b/Source/OverbyteIcsWSocket.pas
index dce509a0..8dd824a4 100644
--- a/Source/OverbyteIcsWSocket.pas
+++ b/Source/OverbyteIcsWSocket.pas
@@ -22299,13 +22299,10 @@ begin
     else begin
      { V8.56 IPv6 support from Max Terentiev }
         if WSocketIsIP(String(FHttpTunnelServer), LSocketFamily) then begin
-            if (LSocketFamily = sfIPv4) or (IsIPv6APIAvailable) then
-                FSocketFamily := LSocketFamily
-            else
-                FSocketFamily := DefaultSocketFamily;
-        end
-        else
-          raise ESocketException.Create('Unsupported http proxy address format');
+            if (LSocketFamily = sfIPv6) and not IsIPv6APIAvailable then
+              raise ESocketException.Create('IPv6 is not available');
+            FSocketFamily := LSocketFamily;
+        end;
         if WSocketIsIP(FAddrStr, LSocketFamily) then begin
             if LSocketFamily=sfIPv6 then
                 FAddrStr:='['+FAddrStr+']';  // IPv6 must be in [ ]

Fixed subj

Edited September 19, 2019 by Fr0sT.Brutal
fixed code

[Angus Robertson 574]

Perhaps you can explain what this patch is intended to achieve, in the code as a new patch, so anyone looking at it understands why you  are proposing a change and marking the changes as V8.63.  Looking at the patch, I've no idea myself, so won't use it.

 

Angus

 

[Fr0sT.Brutal 900]

Ehm, it is intended to achieve the fix of bug that is described above.

Bug is that HTTP proxy could not have non-IP address.

As an alternative, only

else
          raise ESocketException.Create('Unsupported http proxy address format');

lines could be removed. The case of v6 IP when v6 is unavailable on the system will then fall to WSocket_Synchronized_ResolveHost(FHttpTunnelServer) and fail with 'Winsock Resolve Host: Cannot convert host address' exception. Not too informative as for me but this would be minimal change.

[Angus Robertson 574]

That may be so, but this thread is two months old and not available to people looking at the ICS source code.  The more effort I have to put into checking and testing bug fixes, the longer they take.

 

Angus