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...
|
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.
|
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.
|
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).
|
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)
|
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...
|
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.
|
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)...
|
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)
|
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.
|
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?
|
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?
|
Fri 07 Sep 2007 11:51:54 PM UTC, comment #8:
PUSH ! Comment #3 thoughts ?
|
Fri 31 Aug 2007 03:32:57 PM UTC, comment #7:
I'll try and look at this next week. Sorry for the delay.
|
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?
|
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
|
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.
|
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?
|
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...
|
Sun 26 Aug 2007 02:22:17 PM UTC, comment #1:
No objects for this first part of the patch?
(file #13804)
|
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;);
|