[Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[Tao Wu]

The current code looks buggy, we zero ti_i while we access
ti_dst/ti_src later.

Signed-off-by: Tao Wu <lepton@google.com>
---
 slirp/tcp_subr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index dc8b4bbb50..398d6b30d3 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -148,7 +148,6 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
 		m->m_data += IF_MAXLINKHDR;
 		*mtod(m, struct tcpiphdr *) = *ti;
 		ti = mtod(m, struct tcpiphdr *);
-		memset(&ti->ti, 0, sizeof(ti->ti));
 		flags = TH_ACK;
 	} else {
 		/*
-- 
2.12.2.816.g2cccc81164-goog

Re: [Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[Thomas Huth]

On 20.04.2017 22:43, Tao Wu wrote:
> The current code looks buggy, we zero ti_i while we access
> ti_dst/ti_src later.
> 
> Signed-off-by: Tao Wu <lepton@google.com>
> ---
>  slirp/tcp_subr.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> index dc8b4bbb50..398d6b30d3 100644
> --- a/slirp/tcp_subr.c
> +++ b/slirp/tcp_subr.c
> @@ -148,7 +148,6 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
>  		m->m_data += IF_MAXLINKHDR;
>  		*mtod(m, struct tcpiphdr *) = *ti;
>  		ti = mtod(m, struct tcpiphdr *);
> -		memset(&ti->ti, 0, sizeof(ti->ti));
>  		flags = TH_ACK;
>  	} else {
>  		/*

When providing patches, please make sure to put the subsystem maintainer
on CC: (see MAINTAINERS file) - or your patch will likely be lost in the
high traffic of the qemu-devel mailing list.

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[Samuel Thibault]

Hello,

Thomas Huth, on lun. 24 avril 2017 11:15:56 +0200, wrote:
> On 20.04.2017 22:43, Tao Wu wrote:
> > The current code looks buggy, we zero ti_i while we access
> > ti_dst/ti_src later.

Indeed.

> > Signed-off-by: Tao Wu <lepton@google.com>

> >  		*mtod(m, struct tcpiphdr *) = *ti;
> >  		ti = mtod(m, struct tcpiphdr *);
> > -		memset(&ti->ti, 0, sizeof(ti->ti));

But then we don't make sure that ih_x1 is 0, which is needed for the
checksum to be correct, but possibly not set by the caller.

So please replace the memset call with setting the proper ih_x1 field to
0 (which thus needs the introductino of a switch over af like below in
the code).

Samuel

Re: [Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[lepton]

Hi Samuel,

Should I add an assert for ih_x1?  It sounds like a bug that  caller set up
a right src and dst address and without set right ih_x1.

On Thu, Apr 27, 2017 at 6:21 AM, Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Hello,
>
> Thomas Huth, on lun. 24 avril 2017 11:15:56 +0200, wrote:
> > On 20.04.2017 22:43, Tao Wu wrote:
> > > The current code looks buggy, we zero ti_i while we access
> > > ti_dst/ti_src later.
>
> Indeed.
>
> > > Signed-off-by: Tao Wu <lepton@google.com>
>
> > >             *mtod(m, struct tcpiphdr *) = *ti;
> > >             ti = mtod(m, struct tcpiphdr *);
> > > -           memset(&ti->ti, 0, sizeof(ti->ti));
>
> But then we don't make sure that ih_x1 is 0, which is needed for the
> checksum to be correct, but possibly not set by the caller.
>
> So please replace the memset call with setting the proper ih_x1 field to
> 0 (which thus needs the introductino of a switch over af like below in
> the code).
>
> Samuel
>

Re: [Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[Samuel Thibault]

Hello,

lepton, on mer. 03 mai 2017 11:35:05 -0700, wrote:
> It sounds like a bug that  caller set up a
> right src and dst address and without set right ih_x1.

I wouldn't bet on that. ih_x1 is only a filler, the caller can be using
the structure only as a C structure, and it's only here just before the
checksum computation that we really need ih_x1 to be 0.

Samuel

Re: [Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[lepton]

Hi Samuel,


There could 2 kind of bugs:

1.  For some reason, caller didn't setup anything in tcpiphdr, so there is
random data inside it.
2.  For some reason, caller setup correct src/dst address in tcpiphdr but
don't zero ix_h1

Actually I worried about bug 1 more than bug 2.

With assert in code, it's easy to catch bug 2 and bug 1.

But if I just zero ix_h1 in my code, then it's somewhat difficult to catch
bug 1. Finally the code could just send out some
random packets to some random address.

If you still think this doesn't look likely happen, I am fine with your
suggestion and will add zero for ih_x1, any comments?

On Thu, May 4, 2017 at 4:05 PM, Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Hello,
>
> lepton, on mer. 03 mai 2017 11:35:05 -0700, wrote:
> > It sounds like a bug that  caller set up a
> > right src and dst address and without set right ih_x1.
>
> I wouldn't bet on that. ih_x1 is only a filler, the caller can be using
> the structure only as a C structure, and it's only here just before the
> checksum computation that we really need ih_x1 to be 0.
>
> Samuel
>

Re: [Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[Samuel Thibault]

Hello,

lepton, on lun. 08 mai 2017 12:08:55 -0700, wrote:
> 1.  For some reason, caller didn't setup anything in tcpiphdr, so there is
> random data inside it.
> 2.  For some reason, caller setup correct src/dst address in tcpiphdr but don't
> zero ix_h1

> If you still think this doesn't look likely happen,

Well, 1) is very likely to have been spotten another way. And crashing
just because of 2) is harsh on users.

The original code was setting ih_x1 at this point, so that was the
original intent.

> I am fine with your suggestion and will add zero for ih_x1, any
> comments?

Samuel

[Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

[Tao Wu]

The current code looks buggy, we zero ti_i while we access
ti_dst/ti_src later.

Signed-off-by: Tao Wu <lepton@google.com>
---
 slirp/tcp_subr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index dc8b4bbb50..da0d53743f 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -148,7 +148,16 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
 		m->m_data += IF_MAXLINKHDR;
 		*mtod(m, struct tcpiphdr *) = *ti;
 		ti = mtod(m, struct tcpiphdr *);
-		memset(&ti->ti, 0, sizeof(ti->ti));
+		switch (af) {
+		case AF_INET:
+		    ti->ti.ti_i4.ih_x1 = 0;
+		    break;
+		case AF_INET6:
+		    ti->ti.ti_i6.ih_x1 = 0;
+		    break;
+		default:
+		    g_assert_not_reached();
+		}
 		flags = TH_ACK;
 	} else {
 		/*
-- 
2.15.0.448.gf294e3d99a-goog

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.
Type: series
Message-id: 20171108225340.10194-1-lepton@google.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20171108225340.10194-1-lepton@google.com -> patchew/20171108225340.10194-1-lepton@google.com
Switched to a new branch 'test'
566ba17a0b slirp: don't zero ti_i since we access it later.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: slirp: don't zero ti_i since we access it later....
ERROR: code indent should never use tabs
#21: FILE: slirp/tcp_subr.c:151:
+^I^Iswitch (af) {$

ERROR: code indent should never use tabs
#22: FILE: slirp/tcp_subr.c:152:
+^I^Icase AF_INET:$

ERROR: code indent should never use tabs
#23: FILE: slirp/tcp_subr.c:153:
+^I^I    ti->ti.ti_i4.ih_x1 = 0;$

ERROR: code indent should never use tabs
#24: FILE: slirp/tcp_subr.c:154:
+^I^I    break;$

ERROR: code indent should never use tabs
#25: FILE: slirp/tcp_subr.c:155:
+^I^Icase AF_INET6:$

ERROR: code indent should never use tabs
#26: FILE: slirp/tcp_subr.c:156:
+^I^I    ti->ti.ti_i6.ih_x1 = 0;$

ERROR: code indent should never use tabs
#27: FILE: slirp/tcp_subr.c:157:
+^I^I    break;$

ERROR: code indent should never use tabs
#28: FILE: slirp/tcp_subr.c:158:
+^I^Idefault:$

ERROR: code indent should never use tabs
#29: FILE: slirp/tcp_subr.c:159:
+^I^I    g_assert_not_reached();$

ERROR: code indent should never use tabs
#30: FILE: slirp/tcp_subr.c:160:
+^I^I}$

total: 10 errors, 0 warnings, 17 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

[Marc-André Lureau]

Hi

Adding Guillaume in CC, who wrote that line in commit 98c63057d2144

On Wed, Nov 8, 2017 at 11:53 PM, Tao Wu via Qemu-devel
<qemu-devel@nongnu.org> wrote:
> The current code looks buggy, we zero ti_i while we access
> ti_dst/ti_src later.

Could you described the symptoms and why you fixed it that way?

thanks

>
> Signed-off-by: Tao Wu <lepton@google.com>
> ---
>  slirp/tcp_subr.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> index dc8b4bbb50..da0d53743f 100644
> --- a/slirp/tcp_subr.c
> +++ b/slirp/tcp_subr.c
> @@ -148,7 +148,16 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m,
>                 m->m_data += IF_MAXLINKHDR;
>                 *mtod(m, struct tcpiphdr *) = *ti;
>                 ti = mtod(m, struct tcpiphdr *);
> -               memset(&ti->ti, 0, sizeof(ti->ti));
> +               switch (af) {
> +               case AF_INET:
> +                   ti->ti.ti_i4.ih_x1 = 0;
> +                   break;
> +               case AF_INET6:
> +                   ti->ti.ti_i6.ih_x1 = 0;
> +                   break;
> +               default:
> +                   g_assert_not_reached();
> +               }
>                 flags = TH_ACK;
>         } else {
>                 /*
> --
> 2.15.0.448.gf294e3d99a-goog
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

[Samuel Thibault]

Hello,

Tao Wu, on mer. 08 nov. 2017 14:53:40 -0800, wrote:
> The current code looks buggy, we zero ti_i while we access
> ti_dst/ti_src later.

Mmm, indeed, looking again at how it was introduced, it was too much.

Samuel

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

[Tao Wu(吴涛@Eng)]

Thanks. Actually this is a follow up with my previous effort to fix this
bug.
I was busy on something else and then got lost in that old thread. Now I
just checked some my local patch
to see if they've merged to upstream and then found it out.

This is old thread about this:
http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg05544.html


On Thu, Nov 9, 2017 at 2:50 AM, Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> Adding Guillaume in CC, who wrote that line in commit 98c63057d2144
>
> On Wed, Nov 8, 2017 at 11:53 PM, Tao Wu via Qemu-devel
> <qemu-devel@nongnu.org> wrote:
> > The current code looks buggy, we zero ti_i while we access
> > ti_dst/ti_src later.
>
> Could you described the symptoms and why you fixed it that way?
>
> thanks
>
> >
> > Signed-off-by: Tao Wu <lepton@google.com>
> > ---
> >  slirp/tcp_subr.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> > index dc8b4bbb50..da0d53743f 100644
> > --- a/slirp/tcp_subr.c
> > +++ b/slirp/tcp_subr.c
> > @@ -148,7 +148,16 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti,
> struct mbuf *m,
> >                 m->m_data += IF_MAXLINKHDR;
> >                 *mtod(m, struct tcpiphdr *) = *ti;
> >                 ti = mtod(m, struct tcpiphdr *);
> > -               memset(&ti->ti, 0, sizeof(ti->ti));
> > +               switch (af) {
> > +               case AF_INET:
> > +                   ti->ti.ti_i4.ih_x1 = 0;
> > +                   break;
> > +               case AF_INET6:
> > +                   ti->ti.ti_i6.ih_x1 = 0;
> > +                   break;
> > +               default:
> > +                   g_assert_not_reached();
> > +               }
> >                 flags = TH_ACK;
> >         } else {
> >                 /*
> > --
> > 2.15.0.448.gf294e3d99a-goog
> >
> >
>
>
>
> --
> Marc-André Lureau
>

Re: [Qemu-devel] [PATCH] slirp: don't zero ti_i since we access it later.

[Samuel Thibault]

Tao Wu(吴涛@Eng), on jeu. 09 nov. 2017 10:48:27 -0800, wrote:
> Thanks. Actually this is a follow up with my previous effort to fix this bug.
> I was busy on something else and then got lost in that old thread. Now I just
> checked some my local patch
> to see if they've merged to upstream and then found it out.
> 
> This is old thread about this:
> [1]http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg05544.html

I knew I had seen that proposal somewhere before :)

Thanks for the patch,
Samuel