slirp: don't zero ti_i since we acccess it later.

PATCH In review Tao Wu 6 participants 13 messages Apr 20, 2017 – Nov 10, 2017

lore

Share & tools

lore

Tao Wu

author

2017-04-20 20:43 UTC #0 lore raw

PATCH 1/1 slirp: don't zero ti_i since we acccess it later.

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

Signed-off-by Tao Wu <[email protected]>

---
 slirp/tcp_subr.c | 1 -
 1 file changed, 1 deletion(-)

slirp/tcp_subr.c +0 -1

index dc8b4bbb50..398d6b30d3 100644
@@ -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 {
 		/*

Signature · 1 line

2.12.2.816.g2cccc81164-goog

Thomas Huth

2017-04-24 09:16 UTC #1 lore raw

Quoted 13 lines · Tao Wu

The current code looks buggy, we zero ti_i while we access ti_dst/ti_src later.
Signed-off-by Tao Wu <[email protected]>
---
 slirp/tcp_subr.c | 1 -
 1 file changed, 1 deletion(-)
slirp/tcp_subr.c +0 -0
index dc8b4bbb50..398d6b30d3 100644

slirp/tcp_subr.c tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m, · line 153

Quoted 7 lines

slirp/tcp_subr.c +0 -1

@@ -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

Samuel Thibault

2017-04-27 21:46 UTC #2 lore raw

Hello,

Quoted 3 lines · Thomas Huth, on lun. 24 avril 2017 11:15:56 +0200, wrote:

Quoted 2 lines · Tao Wu
The current code looks buggy, we zero ti_i while we access ti_dst/ti_src later.

Indeed.

Quoted 5 lines

slirp/tcp_subr.c tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti, struct mbuf *m, · line 151
Quoted 5 lines
Signed-off-by Tao Wu <[email protected]>
slirp/tcp_subr.c +0 -1
 		*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

lepton

author

2017-05-03 18:35 UTC #3 lore raw

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.

Quoted 24 lines · Samuel Thibault

Hello,
Quoted 3 lines · Thomas Huth, on lun. 24 avril 2017 11:15:56 +0200, wrote:
Quoted 2 lines · Tao Wu
The current code looks buggy, we zero ti_i while we access ti_dst/ti_src later.
Indeed.
Quoted 5 lines
Quoted 5 lines
Signed-off-by Tao Wu <[email protected]>
            *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

Samuel Thibault

2017-05-04 23:05 UTC #4 lore raw

Hello,

Quoted 2 lines · 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

lepton

author

2017-05-08 19:08 UTC #5 lore raw

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?

Quoted 12 lines · Samuel Thibault

Hello,
Quoted 2 lines · 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

Samuel Thibault

2017-05-08 19:54 UTC #6 lore raw

Hello,

Quoted 6 lines · 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.

Quoted 2 lines

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

Samuel

Tao Wu

2017-11-08 22:53 UTC #7 lore raw

PATCH 1/1 slirp: don't zero ti_i since we access it later.

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

Signed-off-by Tao Wu <[email protected]>

---
 slirp/tcp_subr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

slirp/tcp_subr.c +10 -1

index dc8b4bbb50..da0d53743f 100644
@@ -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 {
 		/*

Signature · 1 line

2.15.0.448.gf294e3d99a-goog

Marc-André Lureau

2017-11-09 10:50 UTC #8 lore raw

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

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

Quoted 2 lines · Tao Wu via Qemu-devel

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

Quoted 32 lines

Signed-off-by Tao Wu <[email protected]>

---
 slirp/tcp_subr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

slirp/tcp_subr.c +10 -2

index dc8b4bbb50..da0d53743f 100644
@@ -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

Signature · 1 line

Marc-André Lureau

Tao Wu(吴涛@Eng)

2017-11-09 18:48 UTC #9 lore raw

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

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

Quoted 52 lines · Marc-André Lureau

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

Quoted 2 lines · Tao Wu via Qemu-devel

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

slirp/tcp_subr.c tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti,

Quoted 11 lines

Signed-off-by Tao Wu <[email protected]>

---
 slirp/tcp_subr.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

slirp/tcp_subr.c +0 -0

index dc8b4bbb50..da0d53743f 100644
@@ -148,7 +148,16 @@ tcp_respond(struct tcpcb *tp, struct tcpiphdr *ti,

struct mbuf *m,

Quoted 18 lines

slirp/tcp_subr.c +10 -2

                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 {
                /*
--

Quoted 3 lines

2.15.0.448.gf294e3d99a-goog

--
Marc-André Lureau

Samuel Thibault

2017-11-09 18:50 UTC #10 lore raw

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

Quoted 7 lines · 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

Samuel Thibault

2017-11-09 17:52 UTC #11 lore raw

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

In reply to Tao Wu

Hello,

Quoted 2 lines · 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

[email protected]

bot

2017-11-10 07:13 UTC #12 lore raw

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

In reply to Tao Wu

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

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: [email protected]

=== 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/[email protected] -> patchew/[email protected]
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 [email protected]