RFC: Probably wrong code in nf_nat_snmp_basic.c

RFC: Probably wrong code in nf_nat_snmp_basic.c

[Thomas Jarosch]

Hi,

consider this piece of code from
net/ipv4/netfilter/nf_nat_snmp_basic.c:1198

---------------------------------------------
/*
    * Determine mappping for application layer addresses based
    * on NAT manipulations for the packet.
    */
if (dir == IP_CT_DIR_ORIGINAL) {
    /* SNAT traps */
    map.from = NOCT1(&ct->tuplehash[dir].tuple.src.u3.ip);
    map.to = NOCT1(&ct->tuplehash[!dir].tuple.dst.u3.ip);
} else {
    /* DNAT replies */
    map.from = NOCT1(&ct->tuplehash[dir].tuple.src.u3.ip);
    map.to = NOCT1(&ct->tuplehash[!dir].tuple.dst.u3.ip);
}
---------------------------------------------

cppcheck reported: (style) Found duplicate branches for if and else.


Either I need glasses or the code is exactly the same :)

Cheers,
Thomas

Re: RFC: Probably wrong code in nf_nat_snmp_basic.c

[Pablo Neira Ayuso]

On Thu, Nov 17, 2011 at 10:14:02PM +0100, Thomas Jarosch wrote:
> Hi,
> 
> consider this piece of code from
> net/ipv4/netfilter/nf_nat_snmp_basic.c:1198
> 
> ---------------------------------------------
> /*
>     * Determine mappping for application layer addresses based
>     * on NAT manipulations for the packet.
>     */
> if (dir == IP_CT_DIR_ORIGINAL) {
>     /* SNAT traps */
>     map.from = NOCT1(&ct->tuplehash[dir].tuple.src.u3.ip);
>     map.to = NOCT1(&ct->tuplehash[!dir].tuple.dst.u3.ip);
> } else {
>     /* DNAT replies */
>     map.from = NOCT1(&ct->tuplehash[dir].tuple.src.u3.ip);
>     map.to = NOCT1(&ct->tuplehash[!dir].tuple.dst.u3.ip);
> }
> ---------------------------------------------
> 
> cppcheck reported: (style) Found duplicate branches for if and else.
> 
> 
> Either I need glasses or the code is exactly the same :)

Indeed. I'll be happy to take a patch for this. If you don't make it
in the following days, no problem, I'll take of this myself. Thanks
for the report.

Re: RFC: Probably wrong code in nf_nat_snmp_basic.c

[Thomas Jarosch]

On Friday, 18. November 2011 01:34:29 Pablo Neira Ayuso wrote:
> > if (dir == IP_CT_DIR_ORIGINAL) {
> > 
> >     /* SNAT traps */
> >     map.from = NOCT1(&ct->tuplehash[dir].tuple.src.u3.ip);
> >     map.to = NOCT1(&ct->tuplehash[!dir].tuple.dst.u3.ip);
> > 
> > } else {
> > 
> >     /* DNAT replies */
> >     map.from = NOCT1(&ct->tuplehash[dir].tuple.src.u3.ip);
> >     map.to = NOCT1(&ct->tuplehash[!dir].tuple.dst.u3.ip);
> > 
> > }
> > ---------------------------------------------
> > 
> > cppcheck reported: (style) Found duplicate branches for if and else.
> > 
> > 
> > Either I need glasses or the code is exactly the same :)
> 
> Indeed. I'll be happy to take a patch for this. If you don't make it
> in the following days, no problem, I'll take of this myself. Thanks
> for the report.

The if() statement is probably in there for a reason. I'm not familiar with 
the internal details of the ct stuff, perhaps map.from/map.to
should be swapped or something like that.

James, any idea what's going on?

Cheers,
Thomas

Re: RFC: Probably wrong code in nf_nat_snmp_basic.c

[Thomas Jarosch]

Hi Pablo,

On Friday, 18. November 2011 09:43:29 Thomas Jarosch wrote:
> > Indeed. I'll be happy to take a patch for this. If you don't make it
> > in the following days, no problem, I'll take of this myself. Thanks
> > for the report.
> 
> The if() statement is probably in there for a reason. I'm not familiar
> with the internal details of the ct stuff, perhaps map.from/map.to
> should be swapped or something like that.
> 
> James, any idea what's going on?

Hmm, James is probably busy.

Would be nice if you could take over this, Pablo.

Thanks,
Thomas