buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19162, lwip_sendto: possible to corrupt...

 
 

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

bug #19162: lwip_sendto: possible to corrupt remote addr/port connection state

Submitted by:  Rob Stedman <robstedman1>
Submitted on:  Tue 27 Feb 2007 08:34:23 AM UTC  
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  Frédéric Bernon <fbernon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

( Jump to the original submission)

Fri 04 May 2007 03:23:21 PM UTC, comment #37: 

"Like it we have talk in https://savannah.nongnu.org/task/?6683 , need to add some thread-safe features if two threads call the same "socket/netconn" in the same time (in a global way, not only for this problem). "

This more generic problem would have to be continue in another task..

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Fri 04 May 2007 03:04:50 PM UTC, comment #36: 

To me too.

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 04 May 2007 02:54:59 PM UTC, comment #35: 

Looks good to me.

Jonathan Larmour <jifl>
Project Member
Fri 04 May 2007 01:44:03 PM UTC, comment #34: 

Ok, new patch file merged with Jonathan's patch.

I also add defines for netbuf_copy & netbuf_len (same "spirit" than for netbuf_fromaddr & netbuf_fromport) :

#define netbuf_copy(buf,dataptr,len) netbuf_copy_partial(buf,dataptr,len,0)
#define netbuf_len(buf)              ((buf)->p->tot_len)

And I have rename fields in netbuf (and in all the code, with a comment in CHANGELOG about that) :

struct netbuf {
  struct pbuf p, ptr;
  struct ip_addr *addr;
  u16_t port;
};

And netconn_sendto :

err_t
netconn_sendto(struct netconn conn, struct netbuf buf, struct ip_addr *addr, u16_t port)
{ if (buf!=NULL) {
    buf->addr = addr;
    buf->port = port;
    return netconn_send( conn, buf);
  }
  return ERR_VAL;
}

Ok to check in like that ?

(file #12692)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Fri 04 May 2007 12:49:09 PM UTC, comment #33: 

Seems ok for me. About names, if no objection, I propose to replace "fromaddr" & "fromport" by "addr" & "port" in netbuf struct.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Fri 04 May 2007 10:04:48 AM UTC, comment #32: 

Yes, that's all I was thinking of. It's a small job, but I'm not in a position to test it at the moment, hence me putting it in a task (task #6848).

If you don't mind sanity checking the attached patch too, that would be good.

(file #12691)

Jonathan Larmour <jifl>
Project Member
Fri 04 May 2007 07:27:43 AM UTC, comment #31: 

Ok, I understand "Shame". But to be clear, do you want something like that :

/*pseudo-code...*/
err_t
netconn_sendto(struct netconn conn, struct netbuf buf, struct ip_addr *addr, u16_t port)
{ buf->fromaddr = addr;
  buf->fromport = port;
  return netconn_send( conn, buf);
}

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 03 May 2007 11:28:49 PM UTC, comment #30: 

I was advocating netconn_sendto, just for symmetry with sendto, and because the only overhead now would be the function itself and thus only affects people who choose to use it - it doesn't affect anything elsewhere since your patch already provides the underlying support. It's not necessary, no. That's why it's only a "shame".

Hmm, maybe the use of "shame" in this context does have a subtle meaning in English that is maybe slightly confusing. When I say "it's a shame we couldn't do X" it just means "it would be nice if we could do X".

Jonathan Larmour <jifl>
Project Member
Thu 03 May 2007 07:37:47 PM UTC, comment #29: 

>I think it's a shame there is not a netconn_sendto().


I'm not sure about what could improve the fact to add a netconn_sendto() function. Like this, it's not necessary... No?
Shame? Sorry, I don't understand...

>I also think we should not be afraid to change the names of internal variables. Anyone not using the API deserves what they get, although at the same time, netbuf_fromaddr/fromport could be made macros.


I'm agree with you, and I can do it, but is it ok for others?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 03 May 2007 07:00:55 PM UTC, comment #28: 

I think it's a shame there is not a netconn_sendto(). I also think we should not be afraid to change the names of internal variables. Anyone not using the API deserves what they get, although at the same time, netbuf_fromaddr/fromport could be made macros.

Unfortunately for the last short while I've been taken off the project that is using lwIP, so for now I'll just create this as a separate task assigned to me, although it will be a little while till I do it.

Jonathan Larmour <jifl>
Project Member
Thu 03 May 2007 06:41:16 PM UTC, comment #27: 

I don't have change netbuf field's names because it could broke existing code.

I have check error codes, and it seems to be ok.

No change to do for netconn users.

But like it we have talk in https://savannah.nongnu.org/task/?6683 , need to add some thread-safe features if two threads call the same "socket/netconn" in the same time (in a global way, not only for this problem).

Performances are increased: on my "fast" target, send UDP now reach ~90Mbps (~57Mbps before).

Rob and Bryan problems should be fixed now...

I think we can close this item, but we have to open another one for task #6683 ideas...

(file #12683)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Fri 20 Apr 2007 11:19:51 AM UTC, comment #26: 

To this general discussion: how do we go on with this? I think this is a big bug regarding thread-safety (and speed).

In contrast to patch file #12448, I'd rather somehow include the struct sockaddr, but that would include socket-structs in netconn-api so that would be bad. In that case, we could stick to port==0 = send, port != 0 = sendto.

I did not test the patch in detail, maybe I can give some detailed comment on it next week. But overall, I think it's going into the right direction (although we should take care it doesn't slow down sends on connected sockets too much).

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 12 Apr 2007 09:42:47 PM UTC, comment #25: 

> ... I just wanted you to hear a different point of view.


Yes, it's always something interesting, and that why we spend a part of the night to talk about differents items (even if we don't share same ideas on all). Thank you for your comments...

> I think if you want to make this capability available to netconn users, then there should probably be a netconn_sendto() function that wraps netconn_send, but takes and fills in the address/port in the netbuf (which should still either be renamed from fromaddr/fromport, or made a union with something like destaddr/destport).


I like this part, it's a good compromise...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 12 Apr 2007 06:50:32 PM UTC, comment #24: 

>> Fortunately users should not be accessing these fields directly - that's
> what netbuf_fromaddr() and netbuf_fromport() are for :-).
>
> Yes, but to my point of view, these functions are not a great idea, because
> you can directly access to fields (and doing that is faster and reduce
> footprint).


Maybe then they could be made macros, to preserve the API abstraction - but that's a separate issue. Functions like this are intentional to hide implementation details.

But certainly right now, people are meant to use the functions. It's ok to change the netbuf field names.

>> here are thread-safety issues with the netconn API in some places, but not
> this one - netconn_connect and _send are straightforward...
>
> I'm not agree with you on this point: if your comment about a TCP connection
> is true about segments order (it have to be process by application and/or
> protocol), I think it's wrong for UDP connections. First, an application
> could want to send the same datagram to multiple destinations (if multicast
> isn't supported, or if WAN is used, by example). But with current sequential
> api (netconn or sockets) performance is poor (need to check, connect,
> disconnect, exchange lot of messages between api_lib/api_msg...), but raw api
> allows it (see udp_sendto & raw_sendto, they already do the job at low level).


You are unlikely to need to check actually - the app will usually already know where the connection was set to point to. You also don't need to disconnect - just like lwip_sendto, you can call netconn_connect again.

So it's just a two step process: netconn_connect and netconn_send. I think it's not common at all to send the same datagram to multiple destinations. I think it's even rarer to also need to do it from multiple overlapping threads. Personally it doesn't seem like something common enough to be bending over backwards to add support in the APIs for. And for the rare users who do need this and need to do it from multiple overlapping threads - they can just do their own protection - the race condition is fairly obvious because it's a two step process (unlike inside lwip_sendto which presents only a single API function).

> Next, if two threads want to send data using the same socket or netconn, with
> both we can got the problem described by Rob in the initial description.


Only if it is the same socket to multiple destinations.

Anyway, I realise I probably sound like I have stronger feelings on this than I actually do. I did say before I was only "uneasy" about it. If you are really sure about it, I'm okay with that - I just wanted you to hear a different point of view.

I think if you want to make this capability available to netconn users, then there should probably be a netconn_sendto() function that wraps netconn_send, but takes and fills in the address/port in the netbuf (which should still either be renamed from fromaddr/fromport, or made a union with something like destaddr/destport).

> We have already talk about that, but I think that two sequential api, with
> one based on the other one, is not a good idea: I think it will be better to
> keep the BSD one, and to add some extra functions for zero-copy. So, most of
> the code will be the same, footprint will be reduced (for BSD users, of
> course :) ), less code will have to be maintain, keep source code
> compatibility with most of open source applications...). But this last
> comment is another subject (and a bigger task)... (if you want to continue to
> talk about that, I propose to continue on the forum).


As you probably know I disagree :-). I don't think you can sensibly add zero copy to a BSD socket API. At the very least, once you try, you haven't got a BSD socket API. There might be an argument for implementing BSD directly over the raw API, but if it's doing largely the same things as the netconn API, that wouldn't give much benefits and makes things larger if anyone uses both APIs at the same time. Anyway, another time for that discussion.

Jifl

Jonathan Larmour <jifl>
Project Member
Thu 12 Apr 2007 01:20:33 PM UTC, comment #23: 

>Fortunately users should not be accessing these fields directly - that's what netbuf_fromaddr() and netbuf_fromport() are for :-).


Yes, but to my point of view, these functions are not a great idea, because you can directly access to fields (and doing that is faster and reduce footprint).

>I was referring to the fact that fromaddr and fromport are not initialised in netbuf_new, so when a user sends a buffer with netconn_send, they may not be 0.


Ok, initialize in netbuf_new would have to be done.

>Unlike BSD, you can't send on a connection that hasn't been "connected" first.


And I don't find that wasn't a good idea to design the sequantial api like this, because raw api allow to use send and sendto... (continue on next point)

>here are thread-safety issues with the netconn API in some places, but not this one - netconn_connect and _send are straightforward...


I'm not agree with you on this point: if your comment about a TCP connection is true about segments order (it have to be process by application and/or protocol), I think it's wrong for UDP connections. First, an application could want to send the same datagram to multiple destinations (if multicast isn't supported, or if WAN is used, by example). But with current sequential api (netconn or sockets) performance is poor (need to check, connect, disconnect, exchange lot of messages between api_lib/api_msg...), but raw api allows it (see udp_sendto & raw_sendto, they already do the job at low level). Next, if two threads want to send data using the same socket or netconn, with both we can got the problem described by Rob in the initial description. So, even netconn_send is not safe-thread to this point of view. To be clear about what I think to be normal "safe-thread" operations, take a look to https://savannah.nongnu.org/task/?6683. I think we can improve sequential api to get something safe-thread and with performance near than raw api (except copy memory operations, of course).

We have already talk about that, but I think that two sequential api, with one based on the other one, is not a good idea: I think it will be better to keep the BSD one, and to add some extra functions for zero-copy. So, most of the code will be the same, footprint will be reduced (for BSD users, of course :) ), less code will have to be maintain, keep source code compatibility with most of open source applications...). But this last comment is another subject (and a bigger task)... (if you want to continue to talk about that, I propose to continue on the forum).

>We take a relaxed view about careful error checking with lwIP sometimes, with application errors. This might be one of those times?


Right! And It will be...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 12 Apr 2007 03:52:18 AM UTC, comment #22: 

>re api_msg.c changes, fromport and fromaddr being used when
>sending. I think their names would need changing in that case.
>
>Yes, It would be better, but this change the netconn API on
>"recv", and I wouldn't do that


Fortunately users should not be accessing these fields directly - that's what netbuf_fromaddr() and netbuf_fromport() are for :-).

>> In any case, these fields would now need initialising in
>> netbuf_new, otherwise they may have any value.
>
> Yes, except that now, netconn users could avoid to do the same
> job than in older lwip_send (check conn state, connect, send,
> disconnect). In a general way, this new patch give better
> performance than older (less operations), and it's thread
> safe...


I was referring to the fact that fromaddr and fromport are not initialised in netbuf_new, so when a user sends a buffer with netconn_send, they may not be 0.

In answer to what you did say though, the netconn API has a different model to BSD. There's no benefit from avoiding doing the same job at all because the netconn API never had that paradigm in the first place! With the netconn API, it's nice and simple: you always connect first, then you use send. Unlike BSD, you can't send on a connection that hasn't been "connected" first.

> The problem is that netconn & sockets have the same problem.
> Even if you want to use netconn, you will got the same problem
> explain in first comments about this bug. The problem is that
> the original design of ALL the sequential API (netconn or
> sockets) wasn't do to be thread safe.


But with the netconn API, this part /is/ thread-safe. There are thread-safety issues with the netconn API in some places, but not this one - netconn_connect and _send are straightforward. Each function operates separately. If multiple threads tried to do things with them at the same time, that would be no different than with sockets on Linux. For example, if two threads try to write a connection at the same time, that's safe, but you can't guarantee the order. The same is true for Linux. So when it comes to the relative ordering of different netconn calls - that's an application issue. The order of close and write would be even more important :-).

> About the opengroup link, some others things are missing (using
> NULL is only possible if you use a TCP connection, or if the
> UDP connection is already "connect"ed, etc...)


We take a relaxed view about careful error checking with lwIP sometimes, with application errors. This might be one of those times?

Thank you for working on this

Jifl

Jonathan Larmour <jifl>
Project Member
Thu 12 Apr 2007 03:27:07 AM UTC, comment #21: 

>re api_msg.c changes, fromport and fromaddr being used when sending. I think their names would need changing in that case.


Yes, It would be better, but this change the netconn API on "recv", and I wouldn't do that

>In any case, these fields would now need initialising in netbuf_new, otherwise they may have any value.


Yes, except that now, netconn users could avoid to do the same job than in older lwip_send (check conn state, connect, send, disconnect). In a general way, this new patch give better performance than older (less operations), and it's thread safe...

> But I'm uneasy about additions within the netconn API to support the BSD API. These changes do nothing for netconn API users - they're only to fit the BSD API requirements. Given that, I think it would therefore be better to be like before and change the connection temporarily. I've mentioned tcpip_callback() already a few times just this evening, but perhaps that would be better than creating a new message type just for the BSD sockets.


The problem is that netconn & sockets have the same problem. Even if you want to use netconn, you will got the same problem explain in first comments about this bug. The problem is that the original design of ALL the sequential API (netconn or sockets) wasn't do to be thread safe.

> The changed lwip_sendto seems to return lwip error codes, and not set errno.


Yes, this is a draft, and this is not the first thing I would have to check (the port & addr "host" or "network" order is very checked yet). But, yes, it's necessary.

> If that's fixed, lwip_send would also need changing so errno is only set in TCP branch.


Ok

> According to the standards, send(s,buf,len,fl) is the same as sendto(s,buf,len,fl,NULL,0) (see "Application usage" at http://www.opengroup.org/onlinepubs/007908799/xns/send.html ). So allowing a NULL sockaddr in lwip_sendto would be better than having to set up a dummy struct sockaddr in lwip_send to avoid the assert. I think lwip_send() (and lwip_write()) can be made to just call lwip_sendto() directly.


Ok for NULL parameter for sockaddr... About the opengroup link, some others things are missing (using NULL is only possible if you use a TCP connection, or if the UDP connection is already "connect"ed, etc...)

Thank you for these comments

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 12 Apr 2007 02:47:33 AM UTC, comment #20: 

I have some queries about the patch:

  • re api_msg.c changes, fromport and fromaddr being used when sending. I think their names would need changing in that case.
  • In any case, these fields would now need initialising in netbuf_new, otherwise they may have any value.
  • But I'm uneasy about additions within the netconn API to support the BSD API. These changes do nothing for netconn API users - they're only to fit the BSD API requirements. Given that, I think it would therefore be better to be like before and change the connection temporarily. I've mentioned tcpip_callback() already a few times just this evening, but perhaps that would be better than creating a new message type just for the BSD sockets.
  • The changed lwip_sendto seems to return lwip error codes, and not set errno.
  • If that's fixed, lwip_send would also need changing so errno is only set in TCP branch.
  • According to the standards, send(s,buf,len,fl) is the same as sendto(s,buf,len,fl,NULL,0) (see "Application usage" at http://www.opengroup.org/onlinepubs/007908799/xns/send.html ). So allowing a NULL sockaddr in lwip_sendto would be better than having to set up a dummy struct sockaddr in lwip_send to avoid the assert. I think lwip_send() (and lwip_write()) can be made to just call lwip_sendto() directly.
Jonathan Larmour <jifl>
Project Member
Mon 09 Apr 2007 09:20:56 PM UTC, comment #19: 

New patch file for the whole problem is attached. I'm not at the office this week, so, I can't just build it, but not test it. So, befaore next week, see it like a draft.

The idea is to directly call the "sendto" functions already available in raw api (udp_sendto and raw_sendto). I just change netconn_send to replace a pbuf by a netbuf (which contains field fromaddr & fromport).

So, send any comments...

I have to check some details in raw api, but I think it can work just like this...

(file #12448)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 05 Apr 2007 05:02:03 PM UTC, comment #18: 

A little patch to avoid a potential error during lwip_sendto (part of comment #12):

"When you receive lot of datas on a raw or udp socket, you can easily got a "not enough memory" on send (same resource pool - MEMP_NETBUF). It will be better to replace netbuf_new in lwip_send by a local variable"

(file #12408)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 28 Mar 2007 09:06:27 AM UTC, comment #17: 

Ok, add new bug report

Ok, I propose a patch today

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 28 Mar 2007 08:48:56 AM UTC, comment #16: 

For netbuf_ref bug - we really should be discussing this separately.  Could you open a new bug report for it?  If making this change I'd also like to see the return code changed so that the caller is notified of the error.

For the sockets api changes, I think we really need to see a patch before making a decision on this.  It's not particularly clear at the moment what you're proposing and how it fixes lwip_sendto()

Kieran Mansley <kieranm>
Project Member
Wed 28 Mar 2007 08:16:57 AM UTC, comment #15: 

For netbuf_ref bug, I propose to commit directly.

For the define, it's minor, but it will simplify code (but I don't think it will really "moves part of the sockets-code into the application lib", or I don't understand, it would be just a different call)

About last part on api_msg.c, I have discover that udp_sendto already exist !!! So, the change would have to be simple.

For patch, I really would like, but before, I have to commit others ones (and now, it seems to be good) before propose a nice patch (I don't want to risk to merge changes from different patchs).

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 28 Mar 2007 08:08:37 AM UTC, comment #14: 

There seems to be a lot of changes being proposed here that have nothing to do with the original bug report.  Perhaps I've misunderstood, but if that's the case can we open new bugs/tasks for these issues?

Kieran Mansley <kieranm>
Project Member
Wed 28 Mar 2007 06:37:19 AM UTC, comment #13: 

re comment #12:
About the defines: I don't know what to think about that: I have to libs containing the stack code and the application code. Using defines here moves part of the sockets-code into the application lib.
On the other hand, it can make send() faster...

About the sendto-solution: I don't really get what you're proposing. A simple patch would make this clearer... :)

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 27 Mar 2007 08:28:40 PM UTC, comment #12: 

Some remarks. In netbuf_ref, pbuf_alloc result is not tested.

void
netbuf_ref(struct netbuf buf, const void dataptr, u16_t size)
{
  if (buf->p != NULL) {
    pbuf_free(buf->p);
  }
  buf->p = pbuf_alloc(PBUF_TRANSPORT, 0, PBUF_REF);
  if (buf->p == NULL) { <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
     return;            <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  }                     <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  buf->p->payload = (void*)dataptr;
  buf->p->len = buf->p->tot_len = size;
  buf->ptr = buf->p;
}

When you receive lot of datas on a raw or udp socket, you can easily got a "not enough memory" on send (same resource pool - MEMP_NETBUF). It will be better to replace netbuf_new in lwip_send by a local variable :

int
lwip_send(int s, const void *data, int size, unsigned int flags)
{
  struct lwip_socket *sock;
  struct netbuf buf;
  err_t err;

  LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_send(%d, data=%p, size=%d, flags=0x%x)\n", s, data, size, flags));

  sock = get_socket(s);
  if (!sock)
    return -1;

  switch (netconn_type(sock->conn)) {
  case NETCONN_RAW:
  case NETCONN_UDP:
  case NETCONN_UDPLITE:
  case NETCONN_UDPNOCHKSUM:
    /* init a buffer */
    buf.p = buf.ptr = NULL;

    /* make the buffer point to the data that should be sent */
    netbuf_ref(&buf, data, size);
    if (buf.p!=NULL) {
      /* send the data */
      err = netconn_send(sock->conn, &buf);
      /* free packet buffer */
      pbuf_free(buf->p);
    } else {
      err = ERR_MEM;
    }

    break;
  case NETCONN_TCP:
    err = netconn_write(sock->conn, data, size, NETCONN_COPY);
    break;
  default:
    err = ERR_ARG;
    break;
  }
  if (err != ERR_OK) {
    LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_send(%d) err=%d\n", s, err));
    sock_set_errno(sock, err_to_errno(err));
    return -1;
  }

  LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_send(%d) ok size=%d\n", s, size));
  sock_set_errno(sock, 0);
  return size;
}

I propose to replace lwip_read & lwip_recv with define using lwip_recvfrom. Same about lwip_write on lwip_send.

With lwip_sendto on a connection-oriented socket (TCP), the to and tolen parameters are ignored, making sendto equivalent to lwip_send. I propose to handle that at the beginning of lwip_sendto with this line :
if (netconn_type(sock->conn)==NETCONN_TCP) return lwip_send( s, data, size, flags);

With lwip_send on a message-oriented socket (UDPxx, RAW), the call have to be process like a lwip_sendto to "default" address (the one connected). I propose to handle that at the beginning of lwip_send with this line :

if (netconn_type(sock->conn)!=NETCONN_TCP) return lwip_sendto( s, data, size, flags, NULL, 0);
We have to upgrade the lwip_sendto assert for that (or pass a real "struct sockaddr" with 0 in s_addr). I consider is not necessary to handle a "undefined" netconn_type to avoid recursive calls, because only lwip_socket can create netconn, and all types are defined.

With these first changes, we can tell that lwip_send is for TCP, and lwip_sendto for others (UDP & RAW)...

Next step is to set in lwip_sendto fromaddr & fromport with (??) "to" parameters, and to call netconn_send. No change in netconn_send. In do_send, we can add this part of lwip_sendto (not like this, but this is the idea):

  /* get the peer if currently connected */
  connected = (netconn_peer(sock->conn, &addr, &port) == ERR_OK);
  remote_addr.addr = ((struct sockaddr_in *)to)->sin_addr.s_addr;
  remote_port = ((struct sockaddr_in *)to)->sin_port;
  netconn_connect(sock->conn, &remote_addr, ntohs(remote_port));

  // raw_send or udp_send

 

  /* reset the remote address and port number of the connection */
  if (connected)
    netconn_connect(sock->conn, &addr, port);
  else
  netconn_disconnect(sock->conn);

......

I have other patchs in the pipe I wait to commit tomorrow, and next, I could propose a patch file...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 21 Mar 2007 02:07:07 PM UTC, comment #11: 

Hello,
 I was also thinking that adding a new api message could be a good solution. In reference as to why an ICMP destination unreachable is sent I think I have the answer.  When the TFTP client requests a file from the server it connects on a well known port (69). The file transfer actually happens on a ephemeral port (usually in 32000 to 33000 ish range). When the failure occurs the pcb hasn't closed it connection to port 69 yet when the first data packet from the file transfer is sent on the ephemeral port. This results in lwIP notifying the server that the destination is unreachable (I assume because it thinks its connection is currently in use).

Bryan Schwichtenberg <schwichb>
Wed 21 Mar 2007 10:30:04 AM UTC, comment #10: 

Yes.

Kieran Mansley <kieranm>
Project Member
Wed 21 Mar 2007 10:06:59 AM UTC, comment #9: 

Right, but when you have to send a multiple receivers a same datagram, I don't see what else you can do. I don't want to change something in the core, but "just" in api_msg (yes, it will be in tcpip_thread context, but no more code will be executed...).

Are you agree that I propose a patch?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 21 Mar 2007 09:56:52 AM UTC, comment #8: 

Removing lwip_sendto() is not the only solution, but definitely the easiest!

Pushing the sendto() functionality deeper into the stack would work, but I'm reluctant to add special code in the core stack to support a particular API call.  However, if we want to support sendto(), this is probably the best way to do it.

Kieran Mansley <kieranm>
Project Member
Wed 21 Mar 2007 09:25:10 AM UTC, comment #7: 

lwip_sendto()? Uhmm, I don't like lwip_sendto() implementation, but remove would be the only solution? :)

I don't like it because it's not efficient : 3 messages exchange between api & tcpip_thread, for netconn_connect, lwip_send, & netconn_connect/netconn_disconnect.

Each time : 1 alloc(tcpip_apimsg)/1 free(tcpip_thread), 2 post+fetch (tcpip_apimsg post, tcpip_thread fetch, tcpip_thread post, tcpip_apimsh fetch)...

alloc/free cause potential problems (same memp's pool is use by the driver & by api_lib, if you receive lot of packets, you can fall in a "not enough memory" problem, and, most of time, lwIP doesn't like that).

My solution: add a do_sendto in api_msg and do the process in tcpip_thread context, in a atomic way. With this solution, two problems can be solved (Bryan & Rob), and sendto will be faster.

I can propose a patch if you want (I just terminate a mail about another bug). Good for you Kieran?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 21 Mar 2007 08:55:28 AM UTC, comment #6: 

Hmm, interesting.  I think I understand what you're describing.  One thing I'm not sure about is why the response from the linux stack results in an ICMP destination unreachable - if the socket is still connected to the address/port used for the send, surely the reply (which I assume is bound for that address/port) will be received correctly?

However, I can see that there needs to be some protection in this function.  Implementing it in the current architecture would be tricky, and it would become something of a special case.  I'm tempted to "fix" this bug by removing the lwip_sendto() function.

Kieran Mansley <kieranm>
Project Member
Tue 20 Mar 2007 08:47:58 PM UTC, comment #5: 

Some additional clarification. When I say internal lwIP thread I am referring to our receive interrupt which is passing the data back to the lwIP receiver thread.

Bryan Schwichtenberg <schwichb>
Tue 20 Mar 2007 08:41:03 PM UTC, comment #4: 

Hello,
  Sorry for the confusion. When I said one thread I was referring to on "application" thread outside of lwIP.  There is still the receive thread running inside lwIP. So I believe the problem is similar.  As I look at the lwip_sendto() function there is a lwip_send() (which evently puts and event into conn-mbox) followed by a netconn_disconnect() (which also posts to conn->mbox).  In our case the server on the other side responds to the lwip_send() before the netconn_disconnect() is executed. The result is that lwIP sends an ICMP packet to the server saying detination unreachable. Essentially stopping the file transfer.

Bryan Schwichtenberg <schwichb>
Tue 20 Mar 2007 08:27:31 PM UTC, comment #3: 

Hello,

Not sure this is the same problem than Rob. I fI have understand comment #1, Rob got a problem if several application's thread try to use the same socket in same time. And he got a problem in this sequence (from lwip_sendto):

1 poll current connection state
2 force connect state supplied by caller using netconn_connect
3 send data with lwip_send
4 restore previous polled connection state

But this problem is more "application side" (or API safe-thread).

In your problem, if you have only one thread, the problem can't be the same. Can you give me more informations about "the server responds so quickly to the lwip_send() that the receive thread internal to lwIP is putting data into the queue before the netconn_disconnect() can be added to the queue" ?

What kind of problem is? Crash, remote addr/port connection state corrupted?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 20 Mar 2007 08:04:15 PM UTC, comment #2: 

Hello,
  I'm relatively new here, but I have noticed the same issue as listed in the original comment with a different perspective as to root cause from comment #1.

Our application is using UDP under a TFTP client. The client side is a relatively slow embedded processor and the server is a fast Linux based PC.  We have only one application thread on the client side accessing the lwIP stack. However, we run into the same issue because the server responds so quickly to the lwip_send() that the receive thread internal to lwIP is putting data into the queue before the netconn_disconnect() can be added to the queue. I think this means that having application control access won't work. There needs to be multi-threaded protection wthin lwIP. Thanks.

Bryan Schwichtenberg <schwichb>
Tue 27 Feb 2007 08:53:30 AM UTC, comment #1: 

You're right about the assumption that there's only one calling thread per netconn.  That is a deliberate decision to keep things simple, and in most cases will be true.  If a resource of any sort is used from two or more threads, it is usually up to the threads to synchronize access to it, and in this respect a netconn is no different.

It does differ to what people might expect though, so perhaps we need to document this aspect of the API better.

An alternative would be to overhaul the sockets API as you suggest and make it thread safe.  I think that is unlikely to happen, but leave the option open for others if they wish to do this.

Kieran Mansley <kieranm>
Project Member
Tue 27 Feb 2007 08:34:23 AM UTC, original submission:  

lwip_sendto appears to work as a series of steps that should be atomic.

1 poll current connection state
2 force connect state supplied by caller using netconn_connect
3 send data with lwip_send
4 restore previous polled connection state

I think there is a problem in that steps 1..4 may not be atomic in that there is no multithreading protection mechanism used.

Could struct netconn be extended to include a lock()/unlock() semphore?

My understaning is clients call lwip_xyx() functions which in turn call netconn_xyz() functions.  These post messages to tcpip_thread() which actually 'does the work' before posting back to a netconn semaphore that the client caller waits on.

This whole mechanism implies there's only one calling thread accessing a given netcon at any one instant.  This may not be true.  A netcon lock()/unlock() mechanism could be used to protect lwip from multithreaded access to the same netconn.

Rob Stedman <robstedman1>

 

Attached Files
file #12692:  lwip_sendto.patch added by fbernon (12KiB - text/x-patch)
file #12691:  lwip.netbuf.macro.patch added by jifl (2KiB - text/x-patch)
file #12683:  lwip_sendto.patch added by fbernon (8KiB - text/x-patch)
file #12448:  lwip_sendto.patch added by fbernon (7KiB - text/x-patch)
file #12408:  lwip_send.patch added by fbernon (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 jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by schwichb (Posted a comment)
  • -email is unavailable- added by fbernon (Updated the item)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by robstedman1 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-04 fbernon StatusWorks For Me => Fixed
        Open/ClosedOpen => Closed
    2007-05-04 fbernon Attached File- => Added lwip_sendto.patch, #12692
    2007-05-04 jifl Attached File- => Added lwip.netbuf.macro.patch, #12691
    2007-05-03 fbernon Attached File- => Added lwip_sendto.patch, #12683
        StatusIn Progress => Works For Me
    2007-04-09 fbernon Attached File- => Added lwip_sendto.patch, #12448
    2007-04-05 fbernon Attached File- => Added lwip_send.patch, #12408
        StatusNone => In Progress
    2007-03-21 kieranm Assigned tokieranm => fbernon
    2007-03-21 kieranm Severity1 - Wish => 3 - Normal
        Item GroupChange Request => Faulty Behaviour
        Assigned tofbernon => kieranm
    2007-03-04 fbernon Assigned toNone => fbernon
    2007-02-27 kieranm Severity3 - Normal => 1 - Wish
        Item GroupFaulty Behaviour => Change Request

    Back to the top


    Powered by Savane 3.5