tasklwIP - A Lightweight TCP/IP stack - Tasks: task #7675, Enable to refuse data on a...

 
 

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

task #7675: Enable to refuse data on a TCP_EVENT_RECV call

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Sun 13 Jan 2008 12:30:09 PM UTC
   
 
Category:  None Should Start On:  Sat 12 Jan 2008 11:00:00 PM UTC
Should be Finished on:  Sat 12 Jan 2008 11:00:00 PM UTC Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  fbernon Percent Complete:  100%
Open/Closed:  Closed Planned Release:  1.3.0
Effort:  0.00

Jump to the original submission

Sat 19 Jan 2008 11:02:43 AM UTC, comment #11: 

Sorry, just discovered the change in rawapi.txt which covers this change...

Can you somehow delete posts here? :-)

Simon Goldschmidt <goldsimon>
Group administrator
Sat 19 Jan 2008 10:39:10 AM UTC, comment #10: 

After going through the changes made for this task, I think there might be another bug in here. Since it's only from reading the code, I might be wrong (then please tell me :):

Prior to the changes (processing the return value of TCP_EVENT_RECV), the raw API relied on the application callbacks to always deallocate the pbuf it received, regardless of the return value. Now (in tcp_input - tcp_in.c:326) we rely on the pbuf 'recv_data' still being valid after the recv callback is called. With old applications, this might lead to accessing a pbuf which is already deallocated!

Increasing recv_data->ref doesn't help either, since behaviour might be mixed (old apps free the pbuf, new apps + netconn API don't).
For a start, a simple ASSERT(recv_data->ref != 0) might work...

Note that this would not happen if we wouldn't ACK the data before trying to give it to the recv callback, but we delayed that until after 1.3.0 since it implies a rather big change in tcp_in.c :-(

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Jan 2008 09:10:17 PM UTC, comment #9: 

Check in with Jonathan corrections.

Frédéric Bernon <fbernon>
Group Member
Mon 14 Jan 2008 03:18:31 PM UTC, comment #8: 

It looks fine to me - just a few typos and clarity suggestions I would suggest, hope that's ok:

  indicate that the remote host has closed the connection. The

  callback function should delete the pbuf if it returns ERR_OK,

  else, it should not delete (the core keep it).
->
  indicate that the remote host has closed the connection. If
  there are no errors and the callback function is to return
  ERR_OK, then it must free the pbuf. Otherwise, it must not
  free the pbuf so that lwIP core code can store it.



/* If the upper layer can't received this data, keep it */
->
/* If the upper layer can't receive this data, store it */

/* Data previously received by not yet taken by upper layer */
->
/* Data previously received but not yet taken by upper layer */

Jonathan Larmour <jifl>
Group Member
Sun 13 Jan 2008 05:11:14 PM UTC, comment #7: 

If you want to test it, add these lines in httpd.c, line 156

  if (err == ERR_OK && p != NULL) {

  { static int recv_cnt = 0;
    recv_cnt++;
    if (recv_cnt%2) {
      return ERR_MEM;
    }
  }
    /* Inform TCP that we have taken the data. */
    tcp_recved(pcb, p->tot_len);

Frédéric Bernon <fbernon>
Group Member
Sun 13 Jan 2008 05:08:32 PM UTC, comment #6: 

I attach here a patch. I think it's good. Objections to check in ?


(file #14793)

Frédéric Bernon <fbernon>
Group Member
Sun 13 Jan 2008 03:47:06 PM UTC, comment #5: 

I will produce a clean patch for that, like this, the recv_tcp will not be blocking, and we could have a solution for 1.3.0.

The nicest solution could wait post-1.3.0...


Frédéric Bernon <fbernon>
Group Member
Sun 13 Jan 2008 02:52:56 PM UTC, comment #4: 

Hmm, that sounds like it could work (although the pbuf_ref(recv_data) is not really needed).

My solution (first calling pcb->recv and don't ack / stop to process the segment if pcp->recv returns an error) is really far more complex: TCP_EVENT_RECV would have to be called from tcp_receive instead of tcp_input for that (which would also be cleaner for debugging, I think). I wouldn't want to change that now that we have 1.3.0 nearly finished...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 13 Jan 2008 02:08:26 PM UTC, comment #3: 

Perhaps "recv_data_refused" would be better than "last_recv_data" ?

Frédéric Bernon <fbernon>
Group Member
Sun 13 Jan 2008 01:24:21 PM UTC, comment #2: 


>In my opinion, the best solution would be to first call TCP_EVENT_RECV and only acknowledge the data if it returned ERR_OK. In case of an error, the data could be deleted and would eventually be retransmitted by the other side.


This would be the best solution, but it need lot of changes in tcp_in.c.

>While this produces more traffic, I think it would be better than keeping pbufs allocated in a case where the application doesn't process (and free) them fast enough: that could easily lead to running out of memory.


If the lwIP application can't read fast enought the incoming data, it seems normal we drop next packets, and, of course, tcp source will retransmit them, producing more traffic. Just, I don't suggest to keep ALL pbufs, but just one per pcb (struct pbuf *last_recv_data). It will give (most of changes are in tcp_input):

in tcp_new, add "last_recv_data = NULL;"

t1:
(in tcp_input)
TCP_EVENT_RECV(pcb, recv_data, ERR_OK, err);
if (err!=ERR_OK) {
  pbuf_ref(recv_data);
  pcb->last_recv_data = recv_data;
}

t2:
(in tcp_input, just before "err = tcp_process(pcb);" )
if (pcb->last_recv_data != NULL) {
  TCP_EVENT_RECV(pcb, pcb->last_recv_data, ERR_OK, err);
  if (err == ERR_OK) {
    pbuf_free(pcb->last_recv_data);
    pcb->last_recv_data = NULL;
  } else {
    /* drop incoming packets, pcb is "full" */
    LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_input: drop incoming packets, pcb is full\n"));
    TCP_STATS_INC(tcp.drop);
    snmp_inc_tcpinerrs();
    pbuf_free(p);
    return;
  }
}

It's not the nicest solution, but I think it fix the problem for 1.3.0.

Thoughts?


Frédéric Bernon <fbernon>
Group Member
Sun 13 Jan 2008 12:43:01 PM UTC, comment #1: 

In my opinion, the best solution would be to first call TCP_EVENT_RECV and only acknowledge the data if it returned ERR_OK. In case of an error, the data could be deleted and would eventually be retransmitted by the other side.

While this produces more traffic, I think it would be better than keeping pbufs allocated in a case where the application doesn't process (and free) them fast enough: that could easily lead to running out of memory.


However, rhe change in tcp_in.c for receiving first and then acknowledging might not be simple and so I think it should not hold back 1.3.0.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 13 Jan 2008 12:30:09 PM UTC, original submission:  

In task #7490, "Add return value to sys_mbox_post", we have see a problem with the recv_tcp callback (call by TCP_EVENT_RECV in tcp_in.c, line 308): when this callback is invoked, if the pbuf in argument can't be processed (mbox full, or any other reason), there is nothing in the tcp_input code to process this case: the pbuf will not be give in the next call to TCP_EVENT_RECV (so, we lost datas, which is not reliable like tcp should be).

In task #7490, Simon proposed : "So either we have to solve this in tcp_in.c or set the netconn that missed a packet to a fatal error mode (and maybe already abort the pcb?) so that the connection is aborted. "

I proposed: "Perhaps add a pbuf* in the tcp_pcb where we could keep the data to pass to TCP_EVENT_RECV when this one return an error, and we retry to pass it to the next tcp_input call?"

In all cases, I think the problem is more a design problem of the callback raw api: I think we should be able to stop to receive any new datas on a tcp_pcb when this one is "full".


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 #14793:  TCP_EVENT_RECV.patch added by fbernon (6KiB - 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 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
    2008-01-19 goldsimon Open/ClosedOpen Closed
    2008-01-19 goldsimon Open/ClosedClosed Open
    2008-01-14 fbernon Percent Complete40% 100%
        Assigned toNone fbernon
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.3.0
    2008-01-13 fbernon Attached File- Added TCP_EVENT_RECV.patch, #14793
        Percent Complete0% 40%

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code