buglwIP - A Lightweight TCP/IP stack - Bugs: bug #20900, Potential crash error problem with...

 
 

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

bug #20900: Potential crash error problem with netconn_peer & netconn_addr

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Sat 25 Aug 2007 01:39:35 PM UTC
   
 
Category:  None Severity:  4 - Important
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  fbernon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Mon 12 Nov 2007 10:42:46 PM UTC, comment #20: 

Ok, it's check in. If there is no objections, I will close it tomorrow...

Frédéric Bernon <fbernon>
Group Member
Mon 12 Nov 2007 06:20:40 PM UTC, comment #19: 


>I believe you can call netconn_connect() on an existing UDP PCB, which would mean its remote addr can change, so we'd need (2) to be sure this is safe.


I don't think there is any risks here, since this is always the application which change "pcb.udp" (it is not set to NULL like "pcb.tcp" on a err_tcp).

But since Simon and you prefer the second solution, ok, I will check in this evening.


Frédéric Bernon <fbernon>
Group Member
Mon 12 Nov 2007 11:14:15 AM UTC, comment #18: 

I prefer (2) as well.

I believe you can call netconn_connect() on an existing UDP PCB, which would mean its remote addr can change, so we'd need (2) to be sure this is safe.


Jonathan Larmour <jifl>
Group Member
Sun 11 Nov 2007 07:37:59 PM UTC, comment #17: 

I'd prefer the second solution. I think it's more simple if the same function can be used for all types of netconn. Also I don't think it's a real disadvantage that a task switch is needed for UDP and RAW because getpeeraddr is not needed that often (in most cases, it is returned with the receive call).

Simon Goldschmidt <goldsimon>
Group administrator
Sun 11 Nov 2007 06:53:26 PM UTC, comment #16: 

Ok, I propose two solutions (I join two files, but not in diff format, but in a text file with part changes, it was more readable):

1/ (gettcpaddr.txt) this solution is based on the fact that only tcp part need a protection (since raw & udp pcb can't be deleted by tcpip_thread). This idea is to just add a do_gettcpaddr in api_msg.c, and a netconn_gettcpaddr in api_lib.c (used by netconn_addr and netconn_peer to reduce code).

2/ (getaddr.txt) this solution is based on the idea to do all current processing in tcpip_thread. Both netconn_addr and netconn_peer are merged in a same code (netconn_getaddr in api_lib.c, and do_getaddr in api_msg.c): I wasn't sure that adding a "do_getpeeraddr" a "do_getlocaladdr" should be better.

My preference is for the first solution, but give me your points of view...



(file #14358, file #14359)

Frédéric Bernon <fbernon>
Group Member
Fri 09 Nov 2007 01:49:44 PM UTC, comment #15: 

Ok, I will try time this weekend to propose a patch to fix it...

Frédéric Bernon <fbernon>
Group Member
Fri 09 Nov 2007 01:00:18 PM UTC, comment #14: 

Any update on the status of this?  This is one I might be prepared to drop from 1.3.0, but if you're nearly done on it already we can go ahead.  I'll leave it in 1.3.0 for now.

Kieran Mansley <kieranm>
Group Member
Sun 07 Oct 2007 05:34:11 PM UTC, comment #13: 

Change of netconn_addr parameters list is done. In the CHANGELOG, you can read :

"VERY IMPORTANT: this change cause an API breakage for netconn_addr, since a parameter type change. Any compiler should cause an error without any changes in yours netconn_peer calls (so, it can't be a "silent change")."

I will also note that in the wiki.

Now, I going to do "internal" changes part (code in tcpip_thread context)...

Frédéric Bernon <fbernon>
Group Member
Fri 05 Oct 2007 03:32:34 PM UTC, comment #12: 

I'm setting the severety to '4 - important' to reflect the fact that it's planned for 1.3.0 (that way, all bugs planned for 1.3.0 are >= 4)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 Oct 2007 03:30:18 PM UTC, comment #11: 

No, just the time! I was thinking to do that this weekend, and spend a little time to comment and document since this is a API breakage.

Frédéric Bernon <fbernon>
Group Member
Fri 05 Oct 2007 03:28:28 PM UTC, comment #10: 

Frédéric, as this is planned for 1.3.0, is there anything holding us back from making the change?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 18 Sep 2007 12:57:39 PM UTC, comment #9: 

As I understand it the problem is that we return a pointer to an address that can change under the feet of the caller.  You're suggesting to improve this by copying the address and returning as a value rather than a pointer into the pcb structure.

This will mean that we don't get a crash when people try and looking at the address they've been returned, but that they might be looking at an out-of-date copy.

As this is a fix for a bug, I don't see much alternative but to take it, even though it changes the API.  I don't think either of these are frequently called data-path operations, so the drop in performance shouldn't be a factor.  Can we make sure that this change is well documented however?

Kieran Mansley <kieranm>
Group Member
Fri 07 Sep 2007 11:51:54 PM UTC, comment #8: 

PUSH ! Comment #3 thoughts ?

Frédéric Bernon <fbernon>
Group Member
Fri 31 Aug 2007 03:32:57 PM UTC, comment #7: 

I'll try and look at this next week.  Sorry for the delay.

Kieran Mansley <kieranm>
Group Member
Fri 31 Aug 2007 03:24:31 PM UTC, comment #6: 

Since the design of netconn_addr can cause stack crash, I'm in flavor the change it, even if it break one function of the API. Kieran, thoughts?

Frédéric Bernon <fbernon>
Group Member
Fri 31 Aug 2007 03:21:09 PM UTC, comment #5: 

(from IRC Chat #lwip)

(19:56:37)<fbernon>I have to go, but when you have time, can you take a look to bug #20900? Be careful, some * have disappear in the bugtracker (but the mail should be good)
...
(16:02:58)<jifl>I'll leave API changes for Kieran to decide. I agree, but it's a big change for an API
...
(17:04:42)<fbernon>Can I copy/paste your answer to the bugtracker to "push" this bug ?
(17:11:57)<jifl>sure

Frédéric Bernon <fbernon>
Group Member
Thu 30 Aug 2007 05:58:25 PM UTC, comment #4: 

Sorry, some * have disappear in the bugtracker. So, look api.h to see the prototype. The idea should be to remove one indirection for "addr" parameter.

Frédéric Bernon <fbernon>
Group Member
Thu 30 Aug 2007 05:53:55 PM UTC, comment #3: 

I got a first partial patch, but I see another problem with the netconn_addr prototype:

err_t netconn_addr(struct netconn conn, struct ip_addr *addr, u16_t *port);

In these parameters, we don't get the "addr", but a pointer to the internal pcb's addr. So, even if we move the code in tcpip_thread context, this function is not very safe-thread. The better solution will  be to change the prototype to :

err_t netconn_addr(struct netconn *conn, struct ip_addr *addr, u16_t *port);

But it break the netconn API. Thoughts about that?

Frédéric Bernon <fbernon>
Group Member
Mon 27 Aug 2007 10:10:37 AM UTC, comment #2: 

Ok, it's check in. Now, I prepare a patch fo do_getpeername & do_getaddrname...

Frédéric Bernon <fbernon>
Group Member
Sun 26 Aug 2007 02:22:17 PM UTC, comment #1: 

No objects for this first part of the patch?

(file #13804)

Frédéric Bernon <fbernon>
Group Member
Sat 25 Aug 2007 01:39:35 PM UTC, original submission:  

I think there is a potential crash error problem with netconn_peer & netconn_addr

netconn_peer can return :
- ERR_ARG if (conn != NULL)
- ERR_CONN if (conn->pcb.tcp == NULL)

The first is already checked in lwip_accept (the netconn_accept result), the second could appear if between the time we accept the "newconn", a err_tcp appears and set to NULL conn->pcb.tcp. Ok, it could be possible, but, since the err_tcp is processed in a different thread (tcpip_thread) than the application thread, this can even appear just after the check in netconn_peer:

  case NETCONN_TCP:
    if (conn->pcb.tcp == NULL)
      return ERR_CONN;
    <err_tcp is call here in tcpip_thread, and set pcb.tcp to NULL>
    *addr = (conn->pcb.tcp->remote_ip); <<<CRASH
    *port = conn->pcb.tcp->remote_port;
    break;

So, in a general way, it's not safe to call the current netconn_peer with a NETCONN_TCP. Same for netconn_addr (more, there is not the "if (conn->pcb.tcp == NULL) return ERR_CONN;"). At socket layer, it give "same" problems with lwip_getsockname (which call netconn_addr), and lwip_getpeername (which call netconn_peer). For these both functions, to be safe-thread, I think we should go in tcpip_thread context to get these values by adding do_getpeername & do_getaddrname (but could use the current api_msg_msg's "bc" union for that). At runtime, "lwip_accept" would be slower, and I think that lwip_getsockname/netconn_addr and lwip_getpeername/netconn_peer are now often called (but perhaps I'm wrong).

There is also this check in netconn_recv: "if (conn->pcb.tcp->state == LISTEN) {". For this one, we could set conn->state to NETCONN_ACCEPT (from netconn_state enum) in do_listen, and replace this check by "if (conn->state == NETCONN_ACCEPT). Better, since NETCONN_ACCEPT is not currently used in the source code, I would rename it NETCONN_LISTEN. Last, I think this check is done to process application error (call a recv on a socket in listen mode), we should replace it by something like this :

LWIP_ERROR("netconn_recv: conn is in listen mode", (conn->state != NETCONN_ACCEPT), conn->err = ERR_CONN; return NULL;);

Frédéric Bernon <fbernon>
Group Member

 

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

Attached Files
file #14358:  getaddr.txt added by fbernon (3KiB - text/plain)
file #14359:  gettcpaddr.txt added by fbernon (5KiB - text/plain)
file #13804:  NETCONN_LISTEN.patch added by fbernon (1KiB - application/octet-stream)

 

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 goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by fbernon (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-11-13 fbernon StatusWorks For Me Fixed
        Open/ClosedOpen Closed
    2007-11-12 fbernon StatusIn Progress Works For Me
    2007-11-11 fbernon Attached File- Added getaddr.txt, #14358
        Attached File- Added gettcpaddr.txt, #14359
    2007-10-07 fbernon StatusNone In Progress
    2007-10-05 goldsimon Severity3 - Normal 4 - Important
    2007-08-26 fbernon Attached File- Added NETCONN_LISTEN.patch, #13804

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code