[Qemu-devel] [PATCH] slirp: Fix error reported by static code analysis and remove wrong type casts

[Qemu-devel] [PATCH] slirp: Fix error reported by static code analysis and remove wrong type casts

[Stefan Weil]

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).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

Coding style was not fixed by the patch!

 slirp/tcp_subr.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 025b374..5f3214c 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -114,9 +114,9 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
 	int win = 0;
 
 	DEBUG_CALL("tcp_respond");
-	DEBUG_ARG("tp = %lx", (long)tp);
-	DEBUG_ARG("ti = %lx", (long)ti);
-	DEBUG_ARG("m = %lx", (long)m);
+	DEBUG_ARG("tp = %p", tp);
+	DEBUG_ARG("ti = %p", ti);
+	DEBUG_ARG("m = %p", m);
 	DEBUG_ARG("ack = %u", ack);
 	DEBUG_ARG("seq = %u", seq);
 	DEBUG_ARG("flags = %x", flags);
@@ -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;
-- 
1.7.10

Re: [Qemu-devel] [PATCH] slirp: Fix error reported by static code analysis and remove wrong type casts

[Peter Maydell]

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?

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().

-- PMM

Re: [Qemu-devel] [PATCH] slirp: Fix error reported by static code analysis and remove wrong type casts

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH] slirp: Fix error reported by static code analysis and remove wrong type casts

[Peter Maydell]

On 4 September 2012 08:27, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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.

Actually it looks like you can get here with both tp and m NULL:

 * we set up a non-blocking connect
 * at some point slirp.c:slirp_select_poll() finds the fd is writable
 * ...but the send() on slirp.c line 504 fails, so we set SS_NOFDREF
   in the so->so_state
 * on line 520 we call tcp_input() with m == NULL
 * tcp_input checks for m == NULL and immediately goes to cont_conn
 * if so->so_state & SS_NOFDREF we call tcp_close, which
   frees tp and returns 0
 * so we goto dropwithreset with tp NULL and m NULL
 * where we call tcp_respond to try to send a RST or RST|ACK

So I think you have to have failed a syscall for this to happen,
and if we haven't got an outbound fd then there's not a lot we
can do, so just returning from tcp_respond() seems like the best
thing.

-- PMM