buglwIP - A Lightweight TCP/IP stack - Bugs: bug #21656, recvmbox problem in netconn API...

 
 

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

bug #21656: recvmbox problem in netconn API (netconn_recv and others)

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Tue 27 Nov 2007 09:27:58 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Fri 04 Jan 2008 04:12:15 PM UTC, comment #12: 

ok, good for me. I will integrate that in the warning.patch file

Frédéric Bernon <fbernon>
Group Member
Fri 04 Jan 2008 03:56:41 PM UTC, comment #11: 

Why don't we simply use netconn_alloc() - that way we don't have to rename the function when adding or removing parameters.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 04 Jan 2008 03:47:55 PM UTC, comment #10: 

But can we replace netconn_alloc_with_proto_and_callback by netconn_alloc_with_callback (since proto is not used) ?

Frédéric Bernon <fbernon>
Group Member
Fri 04 Jan 2008 03:44:37 PM UTC, comment #9: 


> I forgot I waited an answer from Simon


Sorry I forgot to answer... The main reason is I don't really have strong feelings about any name here... We can probably just leave it the way it is...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 02 Dec 2007 06:46:26 PM UTC, comment #8: 


>Maybe api_gethostbyname and api_allocnetconn (plus api_drainconn) so that we have api_ as a prefix for the non-api_msg_msg functions?


api_ prefix for the non-api_msg ? Could be ambigous?

So, perhaps something helping to know which context is running? Since these functions are never called directly, even the do_ prefix could be changed?

Some ideas:

tcpipapi_
tcpiptool_
apitool_
apiinternal_

...

Frédéric Bernon <fbernon>
Group Member
Sun 02 Dec 2007 06:27:33 PM UTC, comment #7: 


> I just have a "problem" with the function name
> "netconn_alloc_with_proto_and_callback"/. Most (all?) functions
> prefixed by netconn_ were in api.h/api_lib.c, and functions
> prefixed by do_ were in api_msg.h/.c. Why this one is a
> exception? do_allocconn could be a better name ?


I though about that name, too. But I was thinking of a rather different solution: All the do_* functions were functions in the function table (up to the last release version...) and should now be functions that take a strut api_msg_msg as argument. I think the other functions (do_gethostbyname and netconn_alloc_with_proto_and_callback) should have a prefix that doesn't make you think they also take a struct api_msg_msg as and argument.

So while I also think netconn_alloc_with_proto_and_callback is not a good name, a do_ prefix isn't the best solution...

Maybe api_gethostbyname and api_allocnetconn (plus api_drainconn) so that we have api_ as a prefix for the non-api_msg_msg functions?

Simon Goldschmidt <goldsimon>
Group administrator
Sun 02 Dec 2007 04:58:19 PM UTC, comment #6: 

I just have a "problem" with the function name "netconn_alloc_with_proto_and_callback"/. Most (all?) functions prefixed by netconn_ were in api.h/api_lib.c, and functions prefixed by do_ were in api_msg.h/.c. Why this one is a exception? do_allocconn could be a better name ?

>/* @todo: should we drain the recvmbox here? */


yes, we should. But like in the previous perhaps it's better to share the code from "/* Drain the recvmbox. / + / Drain the acceptmbox. */" in both api_lib.c and api_msg.c in a "do_drainconn"?

Frédéric Bernon <fbernon>
Group Member
Sun 02 Dec 2007 02:55:43 PM UTC, comment #5: 

The fix is checked in: recvmbox is allocated in netconn_new_with_proto_and_callback (in fact that's now moved to netconn_alloc_with_proto_and_callback) and freed in do_listen for TCP listen netconns.

This is also more secure, I think, since all items needed for a netconn are allocated when creating the netconn.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 Nov 2007 01:52:55 PM UTC, comment #4: 


> So, the question is : is there any reference which say when bind
> have to be call?


There will be none... this is a standard part of the sockets paradigm - you don't have to bind, in which case a local port will be allocated for you.

Jonathan Larmour <jifl>
Group Member
Wed 28 Nov 2007 08:29:10 AM UTC, comment #3: 

So, the question is : is there any reference which say when bind have to be call?

But it's a detail, as long it doesn't change a lot the code size and doesn't change the behavior, you can go...

Frédéric Bernon <fbernon>
Group Member
Wed 28 Nov 2007 02:52:06 AM UTC, comment #2: 

But as Simon says, this could be a response to something sent with udp_sendto(). There is no need for the application to bind in that case. (It's true that behind the scenes udp_sendto() calls udp_send() which calls udp_bind() if the local port was not yet set, but that's an implementation detail, and entirely unknown to api_lib/msg.c).


Jonathan Larmour <jifl>
Group Member
Tue 27 Nov 2007 09:43:42 PM UTC, comment #1: 


>If netconn_recv is called too late, recvmbox is NULL in api_msg.c:recv_udp() and the received data is simply thrown away.


But if you want to receive, it seems logical to "bind". So, to my point of view, this is not really a problem.

Frédéric Bernon <fbernon>
Group Member
Tue 27 Nov 2007 09:27:58 PM UTC, original submission:  

The recvmbox of a netconn is allocated in:

- netconn_bind (only for UDP/RAW)
- netconn_connect
- netconn_recv

So I think there is a problem here: When creating a UDP or RAW netconn that doesn't bind or connect but implicitly sends using sendto and gets an answer for that, the recvmbox is effectively only allocated in netconn_recv. If netconn_recv is called too late, recvmbox is NULL in api_msg.c:recv_udp() and the received data is simply thrown away.

Therefore, I would suggest always allocating a recvmbox (in netconn_new_with....) and freeing this mbox when allocating an acceptmbox for TCP listen netconns in api_msg.c:do_listen().

Simon Goldschmidt <goldsimon>
Group administrator

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by goldsimon (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-12-02 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code