NetBSD Problem Report #16518
Received: (qmail 28842 invoked from network); 27 Apr 2002 11:56:58 -0000
Message-Id: <20020427115657.702EB11754@yamt.dyndns.org>
Date: Sat, 27 Apr 2002 20:56:57 +0900 (JST)
From: yamt@mwd.biglobe.ne.jp
Reply-To: yamt@mwd.biglobe.ne.jp
To: gnats-bugs@gnats.netbsd.org
Subject: some fixes to libpcap
X-Send-Pr-Version: 3.95
>Number: 16518
>Category: lib
>Synopsis: some fixes to libpcap
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: lib-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sat Apr 27 11:58:00 +0000 2002
>Closed-Date:
>Last-Modified: Tue Feb 20 05:23:33 +0000 2007
>Originator: YAMAMOTO Takashi
>Release: NetBSD 1.5ZC
>Organization:
>Environment:
System: NetBSD bear.yamanet 1.5ZC NetBSD 1.5ZC (build) #13: Wed Apr 3 03:54:19 JST 2002 takashi@bear.yamanet:/usr/home/takashi/work/kernel/build i386
Architecture: i386
Machine: i386
>Description:
- make tcpdump "1>1" works correctly.
- make things unsigned as in-kernel filter does.
- fix a bug that prevents optimization. (and might cause bad codes)
eg. "1 & len == 1"
- fix wrong optimizations.
eg. "0 >> len == 1", "0 - len == 1"
>How-To-Repeat:
try above examples.
>Fix:
apply following patches.
I sent them to patches@tcpdump.org, too.
Index: gencode.c
===================================================================
RCS file: /cvs/NetBSD/basesrc/lib/libpcap/gencode.c,v
retrieving revision 1.29
diff -u -r1.29 gencode.c
--- gencode.c 2001/06/01 15:49:53 1.29
+++ gencode.c 2002/04/24 18:01:24
@@ -2635,16 +2635,17 @@
s0 = xfer_to_x(a1);
s1 = xfer_to_a(a0);
- s2 = new_stmt(BPF_ALU|BPF_SUB|BPF_X);
- b = new_block(JMP(code));
- if (code == BPF_JGT || code == BPF_JGE) {
- reversed = !reversed;
- b->s.k = 0x80000000;
+ if (code == BPF_JEQ) {
+ s2 = new_stmt(BPF_ALU|BPF_SUB|BPF_X);
+ b = new_block(JMP(code));
+ sappend(s1, s2);
}
+ else {
+ b = new_block(BPF_JMP|code|BPF_X);
+ }
if (reversed)
gen_not(b);
- sappend(s1, s2);
sappend(s0, s1);
sappend(a1->s, s0);
sappend(a0->s, a1->s);
Index: optimize.c
===================================================================
RCS file: /cvs/NetBSD/basesrc/lib/libpcap/optimize.c,v
retrieving revision 1.13
diff -u -r1.13 optimize.c
--- optimize.c 2001/01/06 02:11:18 1.13
+++ optimize.c 2002/04/24 21:55:22
@@ -585,7 +585,7 @@
struct stmt *s;
int v0, v1;
{
- bpf_int32 a, b;
+ bpf_u_int32 a, b;
a = vmap[v0].const_val;
b = vmap[v1].const_val;
@@ -669,7 +669,7 @@
return;
last = s;
- while (1) {
+ for (;; s = next) {
s = this_op(s);
if (s == 0)
break;
@@ -712,23 +712,23 @@
* any local dependencies.
*/
if (ATOMELEM(b->out_use, X_ATOM))
- break;
+ continue;
if (next->s.code != (BPF_LDX|BPF_MSH|BPF_B))
add = next;
else
add = this_op(next->next);
if (add == 0 || add->s.code != (BPF_ALU|BPF_ADD|BPF_X))
- break;
+ continue;
tax = this_op(add->next);
if (tax == 0 || tax->s.code != (BPF_MISC|BPF_TAX))
- break;
+ continue;
ild = this_op(tax->next);
if (ild == 0 || BPF_CLASS(ild->s.code) != BPF_LD ||
BPF_MODE(ild->s.code) != BPF_IND)
- break;
+ continue;
/*
* XXX We need to check that X is not
* subsequently used. We know we can eliminate the
@@ -758,27 +758,21 @@
tax->s.code = NOP;
done = 0;
}
- s = next;
}
/*
* If we have a subtract to do a comparison, and the X register
* is a known constant, we can merge this value into the
* comparison.
*/
+ if (BPF_OP(b->s.code) == BPF_JEQ) {
+ /* XXX should indent correctly.
+ * just to make diff small :)
+ */
if (last->s.code == (BPF_ALU|BPF_SUB|BPF_X) &&
!ATOMELEM(b->out_use, A_ATOM)) {
val = b->val[X_ATOM];
if (vmap[val].is_const) {
- int op;
-
b->s.k += vmap[val].const_val;
- op = BPF_OP(b->s.code);
- if (op == BPF_JGT || op == BPF_JGE) {
- struct block *t = JT(b);
- JT(b) = JF(b);
- JF(b) = t;
- b->s.k += 0x80000000;
- }
last->s.code = NOP;
done = 0;
} else if (b->s.k == 0) {
@@ -797,19 +791,13 @@
*/
else if (last->s.code == (BPF_ALU|BPF_SUB|BPF_K) &&
!ATOMELEM(b->out_use, A_ATOM)) {
- int op;
- b->s.k += last->s.k;
last->s.code = NOP;
- op = BPF_OP(b->s.code);
- if (op == BPF_JGT || op == BPF_JGE) {
- struct block *t = JT(b);
- JT(b) = JF(b);
- JF(b) = t;
- b->s.k += 0x80000000;
- }
+ b->s.k += last->s.k;
+
done = 0;
}
+ }
/*
* and #k nop
* jeq #0 -> jset #k
@@ -988,18 +976,17 @@
* that is 0, and simplify. This may not seem like
* much of a simplification but it could open up further
* optimizations.
- * XXX We could also check for mul by 1, and -1, etc.
+ * XXX We could also check for mul by 1, etc.
*/
if (alter && vmap[val[A_ATOM]].is_const
&& vmap[val[A_ATOM]].const_val == 0) {
- if (op == BPF_ADD || op == BPF_OR ||
- op == BPF_LSH || op == BPF_RSH || op == BPF_SUB) {
+ if (op == BPF_ADD || op == BPF_OR) {
s->code = BPF_MISC|BPF_TXA;
vstore(s, &val[A_ATOM], val[X_ATOM], alter);
break;
}
else if (op == BPF_MUL || op == BPF_DIV ||
- op == BPF_AND) {
+ op == BPF_AND || op == BPF_RSH || op == BPF_LSH) {
s->code = BPF_LD|BPF_IMM;
s->k = 0;
vstore(s, &val[A_ATOM], K(s->k), alter);
>Release-Note:
>Audit-Trail:
From: Matthias Drochner <drochner@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: PR/16518 CVS commit: pkgsrc/net/libpcap
Date: Fri, 24 Feb 2006 23:39:44 +0000 (UTC)
Module Name: pkgsrc
Committed By: drochner
Date: Fri Feb 24 23:39:44 UTC 2006
Modified Files:
pkgsrc/net/libpcap: distinfo
Added Files:
pkgsrc/net/libpcap/patches: patch-ae
Log Message:
use unsigned ints in filter, to avoid possible portability problems
with bit shifts,
this is part of PR lib/16518 by yamt
(which is filed against base NetBSD sources, but anyway)
To generate a diff of this commit:
cvs rdiff -r1.14 -r1.15 pkgsrc/net/libpcap/distinfo
cvs rdiff -r0 -r1.1 pkgsrc/net/libpcap/patches/patch-ae
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->feedback
State-Changed-By: drochner@netbsd.org
State-Changed-When: Fri, 24 Feb 2006 23:46:38 +0000
State-Changed-Why:
it seems that all your fixes except the unsigned arithmetice have
been applied
I'll request a pullup to stable branches and close the PR if you're
happy with this.
From: Matthias Drochner <drochner@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: PR/16518 CVS commit: src/lib/libpcap
Date: Fri, 24 Feb 2006 23:41:50 +0000 (UTC)
Module Name: src
Committed By: drochner
Date: Fri Feb 24 23:41:50 UTC 2006
Modified Files:
src/lib/libpcap: optimize.c
Log Message:
use unsigned ints in filter, to avoid possible portability problems
with bit shifts,
this is part of PR lib/16518 by yamt
(the other concerns in that PR should be solved)
To generate a diff of this commit:
cvs rdiff -r1.16 -r1.17 src/lib/libpcap/optimize.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
gnats-admin@netbsd.org, drochner@netbsd.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Sat, 25 Feb 2006 18:39:57 +0900
> Synopsis: some fixes to libpcap
>
> State-Changed-From-To: open->feedback
> State-Changed-By: drochner@netbsd.org
> State-Changed-When: Fri, 24 Feb 2006 23:46:38 +0000
> State-Changed-Why:
> it seems that all your fixes except the unsigned arithmetice have
> been applied
> I'll request a pullup to stable branches and close the PR if you're
> happy with this.
thanks for looking. but i'm not sure if being different from
the tcpdump.org version is a good idea. (it's the reason why the patches
have not been committed until now.)
YAMAMOTO Takashi
From: Matthias Drochner <M.Drochner@fz-juelich.de>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: gnats-bugs@NetBSD.org, lib-bug-people@NetBSD.org,
netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Sat, 25 Feb 2006 14:28:18 +0100
yamt@mwd.biglobe.ne.jp said:
> i'm not sure if being different from the tcpdump.org version is a good
> idea
All of your patches went into the tcpdump.org sources and are
present in 0.9.4. Except that little signed integer bitshift
portability thing.
The patches are also present in the NetBSD tree (which is
at 0.8.3). Partly they were already in the original
distribution, the rest has been applied later.
(Except the bitshift thing again.)
So we are not much different from tcpdump.org in this respect.
(There are lots of other differences - cleanup, const,
format strings etc.)
> it's the reason why the patches have not been committed until now
Afaict they were committed. (Except the bitshift.)
best regards
Matthias
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
To: M.Drochner@fz-juelich.de
Cc: gnats-bugs@NetBSD.org, lib-bug-people@NetBSD.org,
netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Sat, 25 Feb 2006 23:19:50 +0900
> yamt@mwd.biglobe.ne.jp said:
> > i'm not sure if being different from the tcpdump.org version is a good
> > idea
>
> All of your patches went into the tcpdump.org sources and are
> present in 0.9.4. Except that little signed integer bitshift
> portability thing.
portability thing?
iirc, it was for correct operations, not for portability.
> The patches are also present in the NetBSD tree (which is
> at 0.8.3). Partly they were already in the original
> distribution, the rest has been applied later.
> (Except the bitshift thing again.)
> So we are not much different from tcpdump.org in this respect.
> (There are lots of other differences - cleanup, const,
> format strings etc.)
>
> > it's the reason why the patches have not been committed until now
>
> Afaict they were committed. (Except the bitshift.)
i'm talking about the part which hasn't been committed.
YAMAMOTO Takashi
From: Matthias Drochner <M.Drochner@fz-juelich.de>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: gnats-bugs@NetBSD.org, lib-bug-people@NetBSD.org,
netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Sat, 25 Feb 2006 16:16:27 +0100
yamt@mwd.biglobe.ne.jp said:
> portability thing? iirc, it was for correct operations, not for
> portability.
OK, I see that there is another case where the signed/unsigned
makes a difference: division.
So you are right, it is not just about the ">>" portability.
Well, the documentation isn't clear in this respect:
At one point it says:
P[i:n] gives the data at byte offset
``i'' in the packet, interpreted as a word (n=4), unsigned halfword (n=2),
or unsigned byte (n=1).
The halfword and byte are explicitely unsigned, the "word" not.
This might however not refer to arithmetics but just to the fact
that no sign extension happens when a shorter value is loaded
into the accumulator.
There is also the BPF_NEG which wouldn't make much sense in a
strictly unsigned world.
Otoh, since the kernel does unsigned arithmetics and has always
done, I'd definitely expect that the userland filter behaves
identically.
I somehow believe that almost noone does arithmetics on values
which use up the 32 bits, let alone division. That's why
noone really cares...
best regards
Matthias
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
To: M.Drochner@fz-juelich.de
Cc: gnats-bugs@NetBSD.org, lib-bug-people@NetBSD.org,
netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Sun, 26 Feb 2006 00:47:39 +0900
> yamt@mwd.biglobe.ne.jp said:
> > portability thing? iirc, it was for correct operations, not for
> > portability.
>
> OK, I see that there is another case where the signed/unsigned
> makes a difference: division.
> So you are right, it is not just about the ">>" portability.
>
> Well, the documentation isn't clear in this respect:
> At one point it says:
> P[i:n] gives the data at byte offset
> ``i'' in the packet, interpreted as a word (n=4), unsigned halfword (n=2),
> or unsigned byte (n=1).
> The halfword and byte are explicitely unsigned, the "word" not.
> This might however not refer to arithmetics but just to the fact
> that no sign extension happens when a shorter value is loaded
> into the accumulator.
> There is also the BPF_NEG which wouldn't make much sense in a
> strictly unsigned world.
> Otoh, since the kernel does unsigned arithmetics and has always
> done, I'd definitely expect that the userland filter behaves
> identically.
> I somehow believe that almost noone does arithmetics on values
> which use up the 32 bits, let alone division. That's why
> noone really cares...
yea, the fundamental problem is that the definition of filter program
language is somewhat vague about signedness. the part of patch was
dropped because the tcpdump.org guy who handled the patch (itojun)
was not sure which of signed or unsigned should be used.
(see "Re: some fixes for complier and optimizer in libpcap" mails
on tcpdump-workers around June 2002)
i guess it's better to bring a discussion on tcpdump-workers again
and try to fix the problem in their repository first, rather than just
being different from their version.
YAMAMOTO Takashi
From: Matthias Drochner <M.Drochner@fz-juelich.de>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: gnats-bugs@NetBSD.org, lib-bug-people@NetBSD.org,
netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Mon, 27 Feb 2006 14:34:10 +0100
yamt@mwd.biglobe.ne.jp said:
> was not sure which of signed or unsigned should be used.
For me it is pretty obvious that unsigned should be used,
to be consistent with kernel behaviour. While the manpage
doesn't tell, the kernel implementations are there and
cannot be changed anymore, just documented.
> see "Re: some fixes for complier and optimizer in libpcap" mails
> on tcpdump-workers around June 2002
The issues broght up there are unrelated afaict: Your patch
is just about the operators in arithmetics. It doesn't
affect comparisions. (The wrong optimisation of "1>1" was
fixed by another patch which went into mainstream sources.
And why ">"/"<" are not optimized rather than correctly optimized
now is also another issue. "=" is optimized appearently, see below.)
And assignments from singed to unsigned int32 values don't
change anything -- that's more a "lint" issue.
Just to illustrate things a bit:
The filter expression "(0x80000000 / 2) > 0x40000000", if not
optimized, leads to code which rejects everything, because the
kernel evaluates this to "0x40000000 > 0x40000000".
With (unpatched) optimization, the resulting program is:
(000) ld #0xc0000000
(001) ldx #0x40000000
(002) jgt x jt 3 jf 4
(003) ret #96
(004) ret #0
which accepts everything.
Optimization of "=" gets a bit further -- a call of
tcpdump "(0x80000000 / 2) = 0x40000000"
does yield
tcpdump: expression rejects all packets
while
tcpdump -O "(0x80000000 / 2) = 0x40000000"
happily receives packets.
Your patch fixes exactly these inconsistencies.
Just to be sure, I'm talking about
@@ -624,7 +624,7 @@ fold_op(s, v0, v1)
struct stmt *s;
int v0, v1;
{
- bpf_int32 a, b;
+ bpf_u_int32 a, b;
a = vmap[v0].const_val;
b = vmap[v1].const_val;
> i guess it's better to bring a discussion on tcpdump-workers again
This might be a good idea. Do you want to defent your patch once more?
I'm just working on an update of NetBSD's libpcap to 0.9.4, and I plan
to post the more essential changes to tcpdump-workers too.
(Which is merely some "const" consistency for now, and support for
the cloning /dev/bpf.)
> try to fix the problem in their repository first
We have local changes anyway, so I wouldn't wait if it is fixing
a bug. And an optimizer changing the effect of code is a bug.
(Even if only practically useless code is affected...)
best regards
Matthias
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
To: M.Drochner@fz-juelich.de
Cc: gnats-bugs@NetBSD.org, lib-bug-people@NetBSD.org,
netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Tue, 28 Feb 2006 08:18:45 +0900
> yamt@mwd.biglobe.ne.jp said:
> > was not sure which of signed or unsigned should be used.
>
> For me it is pretty obvious that unsigned should be used,
> to be consistent with kernel behaviour. While the manpage
> doesn't tell, the kernel implementations are there and
> cannot be changed anymore, just documented.
to be consistent with kernel, yes. i thought so and made the patch.
i still support the "fix libpcap optimizer to match to kernel" way.
but if it's considered a kernel bug, we can fix kernel implementations, IMO.
see below.
> Optimization of "=" gets a bit further -- a call of
> tcpdump "(0x80000000 / 2) = 0x40000000"
> does yield
> tcpdump: expression rejects all packets
> while
> tcpdump -O "(0x80000000 / 2) = 0x40000000"
> happily receives packets.
>
> Your patch fixes exactly these inconsistencies.
sure.
but, if it was "(-10)/2 == -5", one might think it's natural to
optimize it to TRUE, and consider it as a kernel bug.
given the lack of strict specification of the language, i couldn't say
which was correct.
> > i guess it's better to bring a discussion on tcpdump-workers again
>
> This might be a good idea. Do you want to defent your patch once more?
honestly speaking, i don't, as my memory about libpcap and
the patch is somewhat stale... i'm happy if you just beat me. :-)
> I'm just working on an update of NetBSD's libpcap to 0.9.4, and I plan
> to post the more essential changes to tcpdump-workers too.
> (Which is merely some "const" consistency for now, and support for
> the cloning /dev/bpf.)
great.
YAMAMOTO Takashi
From: Matthias Drochner <M.Drochner@fz-juelich.de>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: gnats-bugs@NetBSD.org, lib-bug-people@NetBSD.org,
netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Tue, 28 Feb 2006 14:17:39 +0100
yamt@mwd.biglobe.ne.jp said:
> if it's considered a kernel bug, we can fix kernel implementations
I don't think this is realistic. The kernel code used unsigned
all the time, and it was imported into x operating systems.
(I'd say signed would feel a bit more "natural", but then
we would need at least need "signed loads" of bytes/shorts,
or perhaps explicite "sign extend" operations to make it useful.)
> if it was "(-10)/2 == -5", one might think it's natural to
> optimize it to TRUE
Hmm, this would open the way to much more confusion:
"(-10)/2 > -6" would have to be FALSE because comparisions
are explicitely unsigned and documented to be so at all places.
(Well, 2 places I know of: the pcap manpage and the section
about expressions in tcpdump's.)
Imho the only way to be consistent is to state that all
operations are unsigned and that "-X" is just a shortcut
for "~X + 1".
> > bring a discussion on tcpdump-workers
> i'm happy if you just beat me
I might do so... I'm currently planning for next week and
it might well happen that I'm AFK all the time.
It might take some time.
best regards
Matthias
From: Matthias Drochner <M.Drochner@fz-juelich.de>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: gnats-bugs@NetBSD.org, lib-bug-people@NetBSD.org,
netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org
Subject: Re: lib/16518 (some fixes to libpcap)
Date: Tue, 28 Feb 2006 14:50:38 +0100
M.Drochner@fz-juelich.de said:
> "(-10)/2 > -6" would have to be FALSE
Sorry, wrong example, a good one is eg.:
"(-10)/2 > 0" would have to be TRUE
best regards
Matthias
State-Changed-From-To: feedback->open
State-Changed-By: yamt@netbsd.org
State-Changed-When: Tue, 20 Feb 2007 05:23:33 +0000
State-Changed-Why:
feedback provided.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home