Re: [BUG] Suspected bug in getpeername and getsockname

Re: [BUG] Suspected bug in getpeername and getsockname

[Balbir Singh]

>From: "David S. Miller" <davem@redhat.com>
>
>    Even if we do not pass the value passed by the user
>    to the protocol specific code, I would like to cleanup
>    the code in socket.c to check for invalid values
>    upfront and save time and space in all the calls.
>
>Optimizing error cases never bears any fruit.

In this case, I certainly think it does. Could u give a
case as to why doing this would be harmful? I think the
only issue can be maintainability and doing the change
cleanly. But I think u are a good maintainer and will
accept the changes only if they are properly fixed.
Right :-)

Balbir

_________________________________________________________________
MSN Photos is the easiest way to share and print your photos: 
http://photos.msn.com/support/worldwide.aspx

Re: [BUG] Suspected bug in getpeername and getsockname

[David S. Miller]

   From: "Balbir Singh" <balbir_soni@hotmail.com>
   Date: Thu, 17 Jan 2002 15:35:59 -0800

   
   >From: "David S. Miller" <davem@redhat.com>
   >Optimizing error cases never bears any fruit.
   
   In this case, I certainly think it does. Could u give a
   case as to why doing this would be harmful? I think the
   only issue can be maintainability and doing the change
   cleanly. But I think u are a good maintainer and will
   accept the changes only if they are properly fixed.
   Right :-)

If I give up the maintainability (ie. make the code more error prone
due to duplication) I better be getting something back.

Can the user eat up more than a scheduling quantum because of the
work done by ->getname()?  I certainly don't think you can prove
this.

Since the user can't, there is no real gain from the change, only
negative maintainability aspects.  (and perhaps that it would make
you happy)

It certainly isn't work the long discussion we're having about it,
that is for sure.

You want this to make your broken getname() protocol semantics work
and I'd like you to address that instead.  I get the feeling that
you've designed this weird behavior and that it is not specified in
any standard anyways that your protocol must behave in this way.  I
suggest you change it to work without the user length being
available.

Franks a lot,
David S. Miller
davem@redhat.com

Re: [BUG] Suspected bug in getpeername and getsockname

[Balbir Singh]

>Can the user eat up more than a scheduling quantum because of the
>work done by ->getname()?  I certainly don't think you can prove
>this.
>

That depends on what ->getname() does. Anyway
in my opinion any code which does all the processing
and then catches any error is BROKEN.

>It certainly isn't work the long discussion we're having about it,
>that is for sure.
>

I agree! no point

>You want this to make your broken getname() protocol semantics work
>and I'd like you to address that instead.  I get the feeling that
>you've designed this weird behavior and that it is not specified in
>any standard anyways that your protocol must behave in this way.  I
>suggest you change it to work without the user length being
>available.
>

There is no other choice but to live with it.

Regards,
Balbir

_________________________________________________________________
MSN Photos is the easiest way to share and print your photos: 
http://photos.msn.com/support/worldwide.aspx

Re: [BUG] Suspected bug in getpeername and getsockname

[Balbir Singh]

>    The reasons why I wanted to pass the address is length
>    is
>
>    1. It gives more flexibility for any body implementing
>       the protocol specific code.
>
>And you could do what with this flexibility that can't be taken care
>of at the top level?
>
>    2. We anyway copy the length in move_addr_to_user, we
>       might as well do it in the system call and pass the
>       length to the protocol.
>
>Why?  What are you going to DO, read this: _DO_, with the
>value?
>
>    3. We can finally copy only the length specified back
>       to the  user as we do currently.
>
>We already do this in move_addr_to_user.  If we do it in
>one place, we don't have to duplicate (and thus risk bugs
>in) this logic in the various protocols.
>

I am not asking the top level functionality to change.
I am asking for the correct length be passed to
the protocol specific code. The reason *WHY* I want
to do this is

I am writing some code to use the sockets infrastructure.
My current sockaddr structure is a union of several
sub-protocols that I implement in one common code.
The family is the same in all these cases.

Depending on the length passed to me in getpeername,
I fill in the correct members and return it back.

>    But, consider a case where a user passes a negative value
>    in len.
>
>Now you are totally talking non-sense.  A negative len is
>an error (-EINVAL) and move_addr_to_user handles this case
>just fine.

I know that move_addr_to_user handles it, but after all
the processing has been done. The time and space has
been wasted. The code should have just returned in
the first place. I am sorry if that did not come
through in my last email.


>
>    I feel the error should be caught first hand, we should not have
>    spent the time and space calling the protocol specific code at all,
>    we should catch the error and return immediately.
>  ...
>    Don't u feel they should be fixed.
>
>If you want to move the "if (len < 0) return -EINVAL;" right before
>the ->getname() invocation, feel free.  However, this is code
>duplication and is error prone.
>

This duplication can be removed if the code is cleaned
up. For example in getpeername the code would look
something like

sys_getpeername(int fd, struct sockaddr *s, int *len)

1. copy length from user, check for invalid values,
   if the values are invalid return
2. Look up the socket
3. Store the value of len passed by user.
4. Pass the socket, address and len to protocol specific
   getpeername
5. copyout the sockaddr upto a maximum value of len passed
   by user (compare using the value stored in step 3)

I think this is a cleaner implementation. Again, I say
I think, there might be reasons why the existing code
is better, which I am not aware of. The protocol specific
code need not even look at len if it does not care,
it can put in its own value in the address we pass to
it. We will compare it to the original value.

This in addition to giving me what I want to see
(passing the value to protocol specific code), saves
time and space when the value of length is invalid.

The existing code handles things fine, but the code can be
improved.

Even if we do not pass the value passed by the user
to the protocol specific code, I would like to cleanup
the code in socket.c to check for invalid values
upfront and save time and space in all the calls.

I hope this is clear.



>But either way, this is not an argument at all to move the user len
>into the protocols.  YOU DONT NEED TO, and you never will, to
>accomplish any legitimate task.
>
>Again the question remains, why would you ever need the user len in
>the protocol handlers?  All I am hearing is a bunch of hot air so far
>with no real substance.
>
>Franks a lot,
>David S. Miller
>davem@redhat.com


Thanks,
Balbir



_________________________________________________________________
Get your FREE download of MSN Explorer at http://explorer.msn.com/intl.asp.

Re: [BUG] Suspected bug in getpeername and getsockname

[David S. Miller]

   From: "Balbir Singh" <balbir_soni@hotmail.com>
   Date: Thu, 17 Jan 2002 15:20:14 -0800
   
   Depending on the length passed to me in getpeername,
   I fill in the correct members and return it back.

What broken protocol works like this?
   
   Even if we do not pass the value passed by the user
   to the protocol specific code, I would like to cleanup
   the code in socket.c to check for invalid values
   upfront and save time and space in all the calls.

Optimizing error cases never bears any fruit.

Re: [BUG] Suspected bug in getpeername and getsockname

[Balbir Singh]

Actually, you are correct about that.

The reasons why I wanted to pass the address is length
is

1. It gives more flexibility for any body implementing
   the protocol specific code.
2. We anyway copy the length in move_addr_to_user, we
   might as well do it in the system call and pass the
   length to the protocol.
3. We can finally copy only the length specified back
   to the  user as we do currently.

I correct my self, it is not a BUG.

But, consider a case where a user passes a negative value
in len. The system call calls the protocol specific
code and then later at the end in move_addr_to_user()
catches the error. I feel the error should be
caught first hand, we should not have spent the
time and space calling the protocol specific code at all,
we should catch the error and return immediately.

I feel there are several instances of this in the
socket system calls.

Don't u feel they should be fixed.

Balbir



>If move_addr_to_user() takes care of all of the issues, there is no
>reason for the protocol specific code to know anything about the
>user's len at all.
>
>You have to show me a purpose for it to get passed down.  What would
>it get used for?  All the protocol specific could should (and does)
>do is provide the data back to the top level routine and
>move_addr_to_user() takes care of the remaining details.




_________________________________________________________________
Send and receive Hotmail on your mobile device: http://mobile.msn.com

Re: [BUG] Suspected bug in getpeername and getsockname

[David S. Miller]

   From: "Balbir Singh" <balbir_soni@hotmail.com>
   Date: Thu, 17 Jan 2002 14:11:06 -0800

   The reasons why I wanted to pass the address is length
   is
   
   1. It gives more flexibility for any body implementing
      the protocol specific code.

And you could do what with this flexibility that can't be taken care
of at the top level?

   2. We anyway copy the length in move_addr_to_user, we
      might as well do it in the system call and pass the
      length to the protocol.

Why?  What are you going to DO, read this: _DO_, with the
value?

   3. We can finally copy only the length specified back
      to the  user as we do currently.
   
We already do this in move_addr_to_user.  If we do it in
one place, we don't have to duplicate (and thus risk bugs
in) this logic in the various protocols.

   But, consider a case where a user passes a negative value
   in len.

Now you are totally talking non-sense.  A negative len is
an error (-EINVAL) and move_addr_to_user handles this case
just fine.

   I feel the error should be caught first hand, we should not have
   spent the time and space calling the protocol specific code at all,
   we should catch the error and return immediately.
 ...
   Don't u feel they should be fixed.
   
If you want to move the "if (len < 0) return -EINVAL;" right before
the ->getname() invocation, feel free.  However, this is code
duplication and is error prone.

But either way, this is not an argument at all to move the user len
into the protocols.  YOU DONT NEED TO, and you never will, to
accomplish any legitimate task.

Again the question remains, why would you ever need the user len in
the protocol handlers?  All I am hearing is a bunch of hot air so far
with no real substance.

Franks a lot,
David S. Miller
davem@redhat.com

Re: [BUG] Suspected bug in getpeername and getsockname

[Balbir Singh]

What I was trying to state is that the protocol specific
code does not get to see the length passed from the user.
The protocol specific code would like to look at what
the user passed.

Balbir

>You totally missed what move_addr_to_user() does, which is in fact
>truncate the copied len to what the user supplied.  Also, the comments
>in move_addr_to_user even quote the bits of 1003.1g you a referring
>to.
>
>In short, the bug you suggest is not there.




_________________________________________________________________
Send and receive Hotmail on your mobile device: http://mobile.msn.com

Re: [BUG] Suspected bug in getpeername and getsockname

[kuznet]

Hello!

> The protocol specific code would like to look at what
> the user passed.

For what purpose?

Protocol has no rights to use this length, because user has right
to pass buffer of any length and address will be simply truncated.
And the truncation is made by net/socket.c, so that protocols need
not worry about this.

Alexey

Re: [BUG] Suspected bug in getpeername and getsockname

[David S. Miller]

   From: "Balbir Singh" <balbir_soni@hotmail.com>
   Date: Thu, 17 Jan 2002 08:27:17 -0800

   What I was trying to state is that the protocol specific
   code does not get to see the length passed from the user.
   The protocol specific code would like to look at what
   the user passed.
   
If move_addr_to_user() takes care of all of the issues, there is no
reason for the protocol specific code to know anything about the
user's len at all.

You have to show me a purpose for it to get passed down.  What would
it get used for?  All the protocol specific could should (and does)
do is provide the data back to the top level routine and
move_addr_to_user() takes care of the remaining details.

[BUG] Suspected bug in getpeername and getsockname

[Balbir Singh]

The current code for sys_getpeername is shown below

asmlinkage long sys_getsockname(int fd, struct sockaddr *usockaddr, int 
*usockaddr_len)
{
        struct socket *sock;
        char address[MAX_SOCK_ADDR];
        int len, err;

        sock = sockfd_lookup(fd, &err);
        if (!sock)
                goto out;
        err = sock->ops->getname(sock, (struct sockaddr *)address, &len, 0);
        if (err)
                goto out_put;
        err = move_addr_to_user(address, len, usockaddr, usockaddr_len);

out_put:
        sockfd_put(sock);
out:
        return err;
}

The man page getpeername(2) says
========================================================
The namelen parameter should be initialized to
indicate the amount of  space  pointed  to  by name.
On return it  contains  the actual size of the name
returned (in bytes).  The name is truncated if the buffer
provided is too small.
=========================================================

The code does not copy_from_user the passed value of
length (by the user). It instead passes to the protocol
specific code a pointer in the stack (len). The copyout to
user space is correct. But still the value passed
from the user should also be considered. If this value
is less than what we want to copyout, the smaller value
should be used.

The same bug exists even in getsockname. The fix is
trivial.

1. Copy in the value the user passed.
2. Pass this value to the protocol (sock ops) getpeername
   or getsockname. Let it decide what to do if the user
   passed value is smaller than the size it wants to
   return.
3. Copyout the values
Am I missing something or is this a known bug.

If this fix is acceptable I can quickly send a patch
after testing it. Please cc me, I am no longer subscribed
to lkml.


Thanks,
Balbir

_________________________________________________________________
Join the world’s largest e-mail service with MSN Hotmail. 
http://www.hotmail.com

Re: [BUG] Suspected bug in getpeername and getsockname

[David S. Miller]

You totally missed what move_addr_to_user() does, which is in fact
truncate the copied len to what the user supplied.  Also, the comments
in move_addr_to_user even quote the bits of 1003.1g you a referring
to.

In short, the bug you suggest is not there.