buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19225, bind, connect, and sendto should...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #19225: bind, connect, and sendto should verify input paramers

Submitter:  Dmitry Potapov <dpotapov>
Submitted:  Wed 07 Mar 2007 07:36:04 AM UTC
   
 
Category:  sockets/netconn Severity:  2 - Minor
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  fbernon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Wed 07 Mar 2007 01:18:49 PM UTC, comment #9: 

I have prepared another patch, which adds LWIP_ASSERT in those places. Also, I have removed excessive set_errno() calls after get_socket(), because errno is set inside of get_socket().

And, yes, netconn API is mainly useful because it allows to avoid an extra copy (and extra memory too).

(file #12143)

Dmitry Potapov <dpotapov>
Wed 07 Mar 2007 11:09:17 AM UTC, comment #8: 

We're all agreed it would be nice, but I'm sure there are some people out there using the netconn API, and the risk of breaking the sockets API would be reasonably high, so this is not something I'd encourage people to spend time on.

As to the original issue, if these checks are really wanted I guess that ASSERTs would be OK.  However, there are so many other places where we should really be doing similar things that the end result is barely improved, so don't worry too much about relatively minor problems like this.

Kieran Mansley <kieranm>
Group Member
Wed 07 Mar 2007 10:49:48 AM UTC, comment #7: 

I definitively would like to, though not today ;-)

No, to be honest, as much as I would like a clean socket API directly interfacing the stack, I think it would certainly need some time until it is as stable as the current solution.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 07 Mar 2007 10:38:00 AM UTC, comment #6: 

Agree with you Simon. You do the job ? :)

Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 10:34:45 AM UTC, comment #5: 


> In fact, I think that netconn_ is not so useful, except for the zero-copy.


In fact, that's what I think, also. I don't really know what the benefit is using netconn api (OK, except for the zero-copy). If you really want to design a fast application, you'd use the callback API. And if not, you can use the socket API.

The only benefit seems to be the zero-copy on receive. TCP sending with copy=0 (zero copy send) is rare since you must have real static (or ROM) data.

So, when receiving, socket API is of course slower, but does that really outweight the fact that we have to support 3 APIs instead of 2?

Also, I think a tightly integrated socket API could be more performant than the current "3-layer-solution".

Simon Goldschmidt <goldsimon>
Group administrator
Wed 07 Mar 2007 10:13:11 AM UTC, comment #4: 


>So, if you worry about footprint and performance, it is better to use netconn_ interface.


In fact, I think that netconn_ is not so useful, except for the zero-copy. To my point of view, it's will be better to only one "sequential" interface, the BSD sockets.

>BTW, there is no need to use set_errno(EBADF) if get_socket(s) returns NULL, because get_socket has already set errno.


Agree with you.

Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 09:52:31 AM UTC, comment #3: 

I believe that the whole socket.c is overhead that you are ready to pay for better compatibility with standard sockets. So, if you worry about footprint and performance, it is better to use netconn_ interface. And, we check the first parameter - socket descriptor, why don't have a reasonable check for others? If you think it adds big overhead, then let's make it ASSERT.

BTW, there is no need to use set_errno(EBADF) if get_socket(s) returns NULL, because get_socket has already set errno.

   sock = get_socket(s);
   if (!sock) {
-    set_errno(EBADF);
     return -1;
   }

As to api_lib.c, I prefer to see ASSERT instead of that not NULL checking.

Dmitry Potapov <dpotapov>
Wed 07 Mar 2007 09:21:22 AM UTC, comment #2: 

Don't you think this checking would have to be disable to reduce footprint and increase performance in "production time", but enable to allow a higher checking during "debug time", or if you don't manage applications using lwip (in my case, I can trust the application developer, it's me :) )?

Same thing about api_lib.c:

Lot of code to enable parameters checking inside netconn functions, like :

if (conn == NULL) {
    return ERR_VAL;
  }

Perhaps an ASSERT about that ?



Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 08:57:03 AM UTC, comment #1: 

Is this designed to catch misbehaving applications, or to prevent applications using a feature that we don't support?

If the former, I'm inclined to leave as is: to make the sockets layer completely robust to misbehaving applications would be a lot of work and a lot of code.

If it's to protect against stuff we don't support, then perhaps it would be OK.

Kieran Mansley <kieranm>
Group Member
Wed 07 Mar 2007 07:36:04 AM UTC, original submission:  

I have attached the patch, which check namelen and sin_family in the aformentioned functions

  if ((namelen != sizeof(struct sockaddr_in)) ||
      ((((struct sockaddr_in *)name)->sin_family) != AF_INET)) {
    sock_set_errno(sock, EINVAL);
    return -1;
  }

Dmitry Potapov <dpotapov>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #12143:  socket.patch added by dpotapov (4KiB - text/x-patch)
file #12137:  socket-inaddr-check.patch added by dpotapov (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by dpotapov (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-03-11 fbernon Open/ClosedOpen Closed
    2007-03-11 fbernon StatusNone Fixed
    2007-03-11 fbernon Assigned toNone fbernon
    2007-03-07 dpotapov Attached File- Added socket.patch, #12143
    2007-03-07 kieranm Severity3 - Normal 2 - Minor
        Item GroupFaulty Behaviour Change Request
    2007-03-07 dpotapov Attached File- Added socket-inaddr-check.patch, #12137

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code