[ECOS] Re: bsp_connect function in packages/net/bsd_tcpip/current/src/sys/kern/sockio.c improvement proposal

[Andrew Lunn <andrew dot lunn at ascom dot ch>]

Please send emails like this to ecos-discuss, not the maintainers.

On Thu, Jan 08, 2004 at 04:16:40PM -0800, Philippe Vandermersch wrote:
> Hi,
> 
> I found out an annoying problem in this function.
> bsd_connect completely ignores the parameter len in the prototype but 
> instead relies on the field sa->sin_len.
> 
> If sa->sin_len is not initialized which is usally the case when the 
> connection of the socket  failed with error 22
> 
> For example, this will fail on eCos but not on Linux or VxWorks :
> 
> static void transfer( )
> {
>    Int                  sock;
>    struct sockaddr_in   addr;
>    Int                  status;
>    UInt32               peerHostId;
> 
> 
>    if (sendMode) {
>        if (hostName) {
>            peerHostId = getIPAddr(hostName);           
>        }
>        else {
>            __SYSLOG("Hostname have to be provided\n");
>            exit(-1);
>        }
> 
>       
>  //    addr.sin_len         = sizeof (struct sockaddr_in);     <-- This 
> line must be uncommented to work on eCos
>        addr.sin_family      = AF_INET;
>        addr.sin_addr.s_addr = peerHostId;
>        addr.sin_port        = htons(portNr);
> 
>        do {
> #ifdef TCP       
>        sock   = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> #else
>        sock   = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
> #endif       
>        status = connect(sock, (Pointer)&addr, sizeof (struct 
> sockaddr_in));  ===> FAILED with errno = 22
>       
> 
> For the sake of the software compatibility, I think it would be good to 
> add this in this function :
>        sa->sin_len = len;
> 
> or at least return a better code than EINVAL. It took me sometime to 
> find out from where this error was coming.
> 
> 
> ======================== IMPLEMENTATION 
> PROPOSAL=================================
> static int
> bsd_connect(cyg_file *fp, const sockaddr *sa, socklen_t len)
> {
>    struct socket *so;
>    int error, s;
> 
>    sa->sin_len = len ;           // <----- Improvement proposal
> 
>    so = (struct socket *)fp->f_data;


This is not quite correct. Take a look at the FreeBSD code. It does
fill in the length, but only the kernel copy of the sockaddr. It
leaves the userspace copy alone. See

http://fxr.watson.org/fxr/source/kern/uipc_syscalls.c#L1595

Please could you write a proper patch with ChangeLog.

It will also need testing properly. This might show up bugs in the
rest of eCos's network code and applications. eg a properly filled in
IPv6 sockaddr is being passed to connect, but the len parameter is
that of an IPv4 address etc.

     Andrew

-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss