[lwip-users] Bug in api_lib.c

[kuschel]

Hello all,

We have found a bug in the file api_lib.c. This bug affects the socket function 
lwip_send() that sometimes result in sending too much data over the TCP/IP 
connection under heavy loads.

Details:

Have a look into the function netconn_write() in file api_lib.c:

659: if (size > tcp_sndbuf(conn->pcb.tcp)) {
660:   /* We cannot send more than one send buffer's worth of data at a
661:      time. */
662:   len = tcp_sndbuf(conn->pcb.tcp);
663: } else {
664:   len = size;
665: }

This lines determine the currently available space in the TCP send buffer. The 
problem is the variable snd_buf that is targeted by the macro tcp_sndbuf():

#define  tcp_sndbuf(pcb)  ((pcb)->snd_buf)

The first problem is that this variable is changed (read-modify-write) by the 
TCP/IP-task and is read asynchronously by any other task working with the 
socket API. On machines that could store the variable in one single cycle back 
to memory this is not a real problem. But imagine lwip ran on a machine that 
needs two discrete stores to save the variable - a asynchronously running task 
could fetch an invalid value if the TCP/IP-task is interrupted while it stores 
the both half-words (in this case: two bytes) to memory.

The second and more important problem is that the variable snd_buf is 
referenced twice in the above code fragment. In our tests, we got a condition 
where the task that currently executes the function netconn_write() was 
interrupted just between the lines 659 and 662 by the TCP/IP task that changed 
the variable snd_buf. This is an unwanted race-condition that normally really 
seldom happens, but in our tests it was over one time every 2 minutes at a data 
rate of around 35 mbit/s (running uCOS/II on a PowerPC440 @400MHz).

The result is that the length 'len' gets a wrong value, leading to an underflow 
of the variable 'size' later in the code. We observed that in this cases 64kb 
or 128kb garbage data was transmitted (note that the variables 'len' and 'size' 
have a width of 16bit).

Solution of the problem: Before line 659, save snd_buf to a temporary variable 
and work then with this.

Also, someone must find a better way to handle the snd_buf variable over 
task-borders, maybe by protecting it with an additional semaphore.

Best Regards,
Dennis