RE : [lwip-devel] Last changes by Marc

[Frédéric BERNON]

> - "sockets.c / check netconn_peer": I don't see no case where
> netconn_peer should return an error. It's increase the footprint, so, is 
> it really useful (do you already have the case with cvs head code)?

I think it spot a potential crash error problem.

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 sure 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 
HYMATOM SA 
Chef de projet informatique 
Microsoft Certified Professional 
Tél. : +33 (0)4-67-87-61-10 
Fax. : +33 (0)4-67-70-85-44 
Email : address@hidden 
Web Site : http://www.hymatom.fr 
====================================
P Avant d'imprimer, penser à l'environnement
 


-----Message d'origine-----
De : address@hidden [mailto:address@hidden De la part de Jonathan Larmour
Envoyé : vendredi 17 août 2007 17:28
À : lwip-devel
Objet : Re: [lwip-devel] Last changes by Marc


Frédéric BERNON wrote:
> Some notes about last changes by Marc Boucher. We have talk yesterday
> with him about some of these changes.

I didn't see anything about these changes anywhere? Nor anything in the 
Changelog. I'm also surprised by all these changes this close to 1.3 when 
other less problematic changes are not being done. That being said, most of 
the changes look good.

> - "Add a different memp pool for MEMP_NUM_TCPIP_MSG_API &
> MEMP_NUM_TCPIP_MSG_INPKT". I'm in flavor of this change, since it avoid 
> problems when we receive lot of packets (or when we got a DoS attack). 
> Perhaps "INPKT" is not so clear. "INPACKET"? (it's a detail of course).

INPKT seems ok to me.

> - "pbuf": split the flags fields in flags & type don't cause me any
> problems  (except I would rename "flgs" in "flags").

I agree.

> - "PBUF_FLAG_PUSH": don't cause me any problem, but it's only used by
> socket layer. If we add LWIP_SOCKET option, we can use it to disable the 
> code in tcp_in.c if anyone want.

Yes please.

> - "tcpip.c / ETHARP_TCPIP_INPUT / ETHARP_TCPIP_ETHINPUT": No problem,
> but, to be honest, I'm in flavor to remove ETHARP_TCPIP_INPUT & 
> ETHARP_TCPIP_ETHINPUT option, and to only support "ETHINPUT" case. It 
> will simplify the code, and is the only way to have no concurrent access 
> in ARP module. No objects to change that?

That seems fine to me. Worth doing for 1.3 I think since we're breaking old 
ports as it is, so no reason to hold back here (and cause more breakage for 
1.4).

> - "tcpip.c / TCPIP_MSG_TIMEOUT": this part increase for evryone the
> footprint (even if you don't use pppoe, your linker should remove 
> tcpip_timeout, but not the part in the main loop's switch). Add option?

Yes please.

> - "sockets.c / check netconn_peer": I don't see no case where
> netconn_peer should return an error. It's increase the footprint, so, is 
> it really useful (do you already have the case with cvs head code)?

netconn_peer can return ERR_CONN.

> - "sockets.c / lwip_recvfrom": I don't have test it, and for me it
> should be ok, but, it change the behavior of the function: before, the 
> function return when it add processed a netbuf. Now, it wait until got 
> "len" bytes. I note this is in this case that PBUF_FLAG_PUSH is used.

It does match standard sockets API behaviour a bit better (as used on other 
systems), but I don't think it's a big issue and it changes code size very 
little.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["The best things in life aren't things."]------      Opinions==mine


_______________________________________________
lwip-devel mailing list
address@hidden http://lists.nongnu.org/mailman/listinfo/lwip-devel

Frédéric BERNON.vcf
Description: Frédéric BERNON.vcf