Messages in this thread | | | Date | Sat, 13 Mar 2010 07:40:03 -0700 | From | Tim Gardner <> | Subject | Re: [Stable-review] [104/145] netfilter: xt_recent: fix false match |
| |
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@canonical.com> >> >> commit 8ccb92ad41cb311e52ad1b1fe77992c7f47a3b63 upstream. >> >> A rule with a zero hit_count will always match. >> >> Signed-off-by: Tim Gardner<tim.gardner@canonical.com> >> Signed-off-by: Patrick McHardy<kaber@trash.net> >> Signed-off-by: Greg Kroah-Hartman<gregkh@suse.de> >> >> --- >> 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@canonical.com
| |