On 2012-09-03 23:08, Peter Maydell wrote:
> On 3 September 2012 21:34, Stefan Weil <sw@weilnetz.de> wrote:
>> Report from smatch:
>> slirp/tcp_subr.c:127 tcp_respond(17) error:
>>  we previously assumed 'tp' could be null (see line 124)
>>
>> Fix this by checking 'tp' before reading its elements.
>>
>> The type casts of pointers to long are not related to the smatch report
>> but happened to be near that code. Those type casts are not allowed
>> when sizeof(pointer) != sizeof(long).
> 
> I think these would be better in a separate patch.
>> @@ -124,7 +124,7 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
>>         if (tp)
>>                 win = sbspace(&tp->t_socket->so_rcv);
>>          if (m == NULL) {
>> -               if ((m = m_get(tp->t_socket->slirp)) == NULL)
>> +               if (tp && (m = m_get(tp->t_socket->slirp)) == NULL)
>>                         return;
>>                 tlen = 0;
>>                 m->m_data += IF_MAXLINKHDR;
> 
> This doesn't look quite right -- now if tp is NULL we will skip
> the assignment to m and dereference a NULL pointer a few lines
> further on, right?

This would be correct:

if (!tp || ..)
    return

> 
> I suspect that we need either to be passed our Slirp* explicitly rather
> than via tp, or  we have to enforce that tcp_respond() is called with
> a non-NULL struct tcpcb*. I think the only cases where tp can be non-NULL
> at the moment are the two calls from the dropwithreset code in tcp_input().

Indeed, this is a "XXX Should never fail" case - according to the code
that checks tp at the call site. But as no one seriously understands
slirp details, we are better safe than sorry.

Jan