Re: [Stable-review] [104/145] netfilter: xt_recent: fix false match

[Tim Gardner]

On 03/12/2010 11:24 PM, Willy Tarreau wrote:
> On Fri, Mar 12, 2010 at 04:27:17PM -0800, Greg KH wrote:
> > 2.6.32-stable review patch.  If anyone has any objections, please let me know.
> >
> > ----------------
> > From: Tim Gardner<tim.gardner@xxxxxxxxxxxxx>
> >
> > commit 8ccb92ad41cb311e52ad1b1fe77992c7f47a3b63 upstream.
> >
> > A rule with a zero hit_count will always match.
> >
> > Signed-off-by: Tim Gardner<tim.gardner@xxxxxxxxxxxxx>
> > Signed-off-by: Patrick McHardy<kaber@xxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman<gregkh@xxxxxxx>
> >
> > ---
> >   net/netfilter/xt_recent.c |    2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- a/net/netfilter/xt_recent.c
> > +++ b/net/netfilter/xt_recent.c
> > @@ -260,7 +260,7 @@ recent_mt(const struct sk_buff *skb, con
> >   		for (i = 0; i<  e->nstamps; i++) {
> >   			if (info->seconds&&  time_after(time, e->stamps[i]))
> >   				continue;
> > -			if (++hits>= info->hit_count) {
> > +			if (info->hit_count&&  ++hits>= info->hit_count) {
> >   				ret = !ret;
> >   				break;
> >   			}
>
> I don't know if this has any undesired side effect or not, but the
> logic is changed now since "hits" will not be increased anymore when
> info->hit_count is zero. And the code does not make it obvious to me
> what the intended purpose was.
>
> For this reason I always find it dangerous to change variables in
> if() conditions because it's where we change operations the most
> frequently when fixing bugs.
>
> Willy

Willy - I agree with you that changing variables in an if() clause can 
be dangerous. I did consider the possibility for side effects in this 
case, but decided to go with the simplest patch since 'hits' is local to 
the scope of the the surrounding else if() clause and is used in no 
other place.

rtg
--
Tim Gardner tim.gardner@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/