Bug 810

Summary: In TCP, Socket::GetSockName() does not return the local socket address
Product: ns-3 Reporter: Bill Roome <wdr>
Component: internetAssignee: Josh Pelkey <jpelkey>
Status: RESOLVED FIXED    
Severity: critical CC: jpelkey, ns-bugs, tomh
Priority: P2    
Version: ns-3.7   
Hardware: All   
OS: All   
Attachments: patch to fix

Description Bill Roome 2010-02-08 16:01:24 EST
Suppose I create a TCP socket, bind it to a wildcard address, and set HandleAccept() as the accept callback:

  Ptr<Socket> s = Socket::CreateSocket (node, TcpSocketFactory::GetTypeId ());
  s.Bind (InetSocketAddress (Ipv4Address::GetAny (), port);
  s.SetAcceptCallback (
     MakeNullCallback<bool, Ptr<Socket>, const Address&> (),
     MakeCallback (HandleAccept));

Then when HandleAccept() calls GetSockName() on the connection socket, GetSockName() returns the wildcard address -- 0.0.0.0. If I bind the server socket to a specific address, GetSockName() does return that address.

In short, in 3.7, GetSockName() returns the address to which the server socket was bound, even if it's a wildcard, rather than the specific address on which the request arrived.

In 3.4, GetSockName() always returned the specific IP address. I don't know about 3.5 & 3.6.
Comment 1 Tom Henderson 2010-02-12 17:35:45 EST
(In reply to comment #0)
> Suppose I create a TCP socket, bind it to a wildcard address, and set
> HandleAccept() as the accept callback:
> 
>   Ptr<Socket> s = Socket::CreateSocket (node, TcpSocketFactory::GetTypeId ());
>   s.Bind (InetSocketAddress (Ipv4Address::GetAny (), port);
>   s.SetAcceptCallback (
>      MakeNullCallback<bool, Ptr<Socket>, const Address&> (),
>      MakeCallback (HandleAccept));
> 
> Then when HandleAccept() calls GetSockName() on the connection socket,
> GetSockName() returns the wildcard address -- 0.0.0.0. If I bind the server
> socket to a specific address, GetSockName() does return that address.
> 
> In short, in 3.7, GetSockName() returns the address to which the server socket
> was bound, even if it's a wildcard, rather than the specific address on which
> the request arrived.
> 
> In 3.4, GetSockName() always returned the specific IP address. I don't know
> about 3.5 & 3.6.

I couldn't reproduce this error-- could you provide a test case?

The examples/tcp/tcp-large-transfer.cc uses a packet sink and binds it to the wildcard.  I edited packet-sink.cc to call GetSockName() after it is provided with the connected socket:

--- a/src/applications/packet-sink/packet-sink.cc       Fri Feb 12 12:12:33 2010 -0800
+++ b/src/applications/packet-sink/packet-sink.cc       Fri Feb 12 14:33:28 2010 -0800
@@ -175,6 +175,10 @@
   NS_LOG_FUNCTION (this << s << from);
   s->SetRecvCallback (MakeCallback(&PacketSink::HandleRead, this));
   m_socketList.push_back (s);
+  Address a;
+  s->GetSockName (a);
+  InetSocketAddress ia = InetSocketAddress::ConvertFrom (a);
+  NS_LOG_UNCOND ("GetSockName " << ia.GetIpv4 () << " " << ia.GetPort ());


and it outputs:

GetSockName 10.1.2.2 50000

not the wildcard.
Comment 2 Bill Roome 2010-02-15 10:15:51 EST
That test fails in 3.7 stable -- PacketSink prints the wildcard address, not the specific address.

The good news: Someone already fixed that bug in the development version. Check TcpSocketImpl::GetSockName(). The dev version returns the address in m_endPoint:

  int
  TcpSocketImpl::GetSockName (Address &address) const
  {
    NS_LOG_FUNCTION_NOARGS ();
    address = InetSocketAddress(m_endPoint->GetLocalAddress (), 
                                m_endPoint->GetLocalPort ());
    return 0;
  }

The stable version, on the other hand, just returned m_localAddress:

  int
  TcpSocketImpl::GetSockName (Address &address) const
  {
    NS_LOG_FUNCTION_NOARGS ();
    address = InetSocketAddress(m_localAddress, m_localPort);
    return 0;
  }

The bad news: The dev version has a bug. m_endPoint can be 0 (I assume that means the socket isn't connected). So GetSockName should use m_localAddress if m_endPoint isn't valid:

  if (m_endPoint != 0)
    {
      address = InetSocketAddress(m_endPoint->.....
    }
  else
    {
      address = InetSocketAddress(m_localAddress, m_localPort);
    }
Comment 3 Tom Henderson 2010-02-22 00:48:14 EST
Created attachment 769 [details]
patch to fix

This patch should resolve the bug.  I made the behavior return zero address and port in this case because 1) that is what ns3 UDP already does, and 2) the posix standard says it is "unspecified" behavior
http://www.opengroup.org/onlinepubs/009695399/functions/getsockname.html

Any other comments?  I looked at raw sockets and decided not to disable GetSockName() even though some systems seem to do so (again, the behavior seems unspecified).
Comment 4 Josh Pelkey 2010-02-22 10:57:52 EST
(In reply to comment #3)
> Created an attachment (id=769) [details]
> patch to fix
> 
> This patch should resolve the bug.  I made the behavior return zero address and
> port in this case because 1) that is what ns3 UDP already does, and 2) the
> posix standard says it is "unspecified" behavior
> http://www.opengroup.org/onlinepubs/009695399/functions/getsockname.html
> 
> Any other comments?  I looked at raw sockets and decided not to disable
> GetSockName() even though some systems seem to do so (again, the behavior seems
> unspecified).

+1 from me
Comment 5 Bill Roome 2010-02-22 11:51:56 EST
(In reply to comment #3)
> Created an attachment (id=769) [details]
> patch to fix
> 
> This patch should resolve the bug.  I made the behavior return zero address and
> port in this case because 1) that is what ns3 UDP already does, and 2) the
> posix standard says it is "unspecified" behavior
> http://www.opengroup.org/onlinepubs/009695399/functions/getsockname.html
> 
> Any other comments?  I looked at raw sockets and decided not to disable
> GetSockName() even though some systems seem to do so (again, the behavior seems
> unspecified).

That depends on how you interpret the standard. The page you cited says the behavior is undefined "If the socket has not been bound to a local name".

Suppose I bind the socket to specific local address & port, but don't connect it. In this case, I'd argue that the socket has been bound to a local name, so getsockname() should return the address passed to bind(). However, with the proposed fix, getsockname() returns the 0 address.

So I'd argue that to be compliant, getsockname() should return the address passed to bind(), which implies getsockname() should return m_localAddress when m_endPoint is 0.

But that's a minor point. I doubt anyone would need to call getsockname() on an unconnected socket. The important thing is to avoid a memory fault.
Comment 6 Tom Henderson 2010-02-22 14:00:21 EST
(In reply to comment #5)
> (In reply to comment #3)
> > Created an attachment (id=769) [details] [details]
> > patch to fix
> > 
> > This patch should resolve the bug.  I made the behavior return zero address and
> > port in this case because 1) that is what ns3 UDP already does, and 2) the
> > posix standard says it is "unspecified" behavior
> > http://www.opengroup.org/onlinepubs/009695399/functions/getsockname.html
> > 
> > Any other comments?  I looked at raw sockets and decided not to disable
> > GetSockName() even though some systems seem to do so (again, the behavior seems
> > unspecified).
> 
> That depends on how you interpret the standard. The page you cited says the
> behavior is undefined "If the socket has not been bound to a local name".
> 
> Suppose I bind the socket to specific local address & port, but don't connect
> it. In this case, I'd argue that the socket has been bound to a local name, so
> getsockname() should return the address passed to bind(). However, with the
> proposed fix, getsockname() returns the 0 address.
> 
> So I'd argue that to be compliant, getsockname() should return the address
> passed to bind(), which implies getsockname() should return m_localAddress when
> m_endPoint is 0.

I agree, but my reading of the code was that m_endPoint could not be zero after Bind(); the endpoint is allocated when Bind() is called.  Are you still seeing that getsockname() returns 0 after Bind()?
Comment 7 Bill Roome 2010-02-22 14:22:30 EST
(In reply to comment #6)
> 
> I agree, but my reading of the code was that m_endPoint could not be zero after
> Bind(); the endpoint is allocated when Bind() is called.  Are you still seeing
> that getsockname() returns 0 after Bind()?

Oops! Sorry, I didn't realize Bind() set m_endPoint. Please ignore my earlier comment.
Comment 8 Josh Pelkey 2010-03-01 15:51:11 EST
changeset bf2ffaeca24f