Something like a bug

Something like a bug

[Rick Xu]

-- 
Thanks,
Rick
Hi, I found something strange in libnfnetlink.c, have a look at the
following function:

int nfnl_nfa_addattr_l(struct nfattr *nfa, int maxlen, int type,
               const void *data, int alen)
{
 ......

    if ((NFA_OK(nfa, nfa->nfa_len) + len) > maxlen) {
        errno = ENOSPC;
        return -1;
    }

    subnfa = (struct nfattr *)(((char *)nfa) + NFA_OK(nfa, nfa->nfa_len));

.....
}

NFA_OK looks so weird here. I think it should be:
int nfnl_nfa_addattr_l(struct nfattr *nfa, int maxlen, int type,
               const void *data, int alen)
{
 ......

    if ( NFA_ALIGN(nfa->nfa_len) + NFA_LENGTH(len) > maxlen) {
        errno = ENOSPC;
        return -1;
    }

    subnfa = (struct nfattr *)(((char *)nfa) + NFA_ALIGN( nfa->nfa_len));

.....
}

Please kindly correct me if I made a mistake.

Re: Something like a bug

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

Rick Xu wrote:
> int nfnl_nfa_addattr_l(struct nfattr *nfa, int maxlen, int type,
>                const void *data, int alen)
> {
>  ......
> 
>     if ((NFA_OK(nfa, nfa->nfa_len) + len) > maxlen) {
>         errno = ENOSPC;
>         return -1;
>     }
> 
>     subnfa = (struct nfattr *)(((char *)nfa) + NFA_OK(nfa, nfa->nfa_len));
> 
> .....
> }
> 
> NFA_OK looks so weird here. I think it should be:
> int nfnl_nfa_addattr_l(struct nfattr *nfa, int maxlen, int type,
>                const void *data, int alen)
> {
>  ......
> 
>     if ( NFA_ALIGN(nfa->nfa_len) + NFA_LENGTH(len) > maxlen) {
>         errno = ENOSPC;
>         return -1;
>     }
> 
>     subnfa = (struct nfattr *)(((char *)nfa) + NFA_ALIGN( nfa->nfa_len));
> 
> .....
> }
> 
> Please kindly correct me if I made a mistake.


Indeed, that function is completely broken but it does not have any
known client.

It seems a leftover of the days when we didn't have anything better to
nest attributes, so that it's completely useless these days. I have
fixed in SVN anyway. Patch attached.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1039 bytes --]

Index: configure.in
===================================================================
--- configure.in	(revisión: 7400)
+++ configure.in	(copia de trabajo)
@@ -4,7 +4,7 @@
 
 AC_CANONICAL_SYSTEM
 
-AM_INIT_AUTOMAKE(libnfnetlink, 0.0.33)
+AM_INIT_AUTOMAKE(libnfnetlink, 0.0.34)
 
 AC_PROG_CC
 AC_EXEEXT
Index: src/libnfnetlink.c
===================================================================
--- src/libnfnetlink.c	(revisión: 7400)
+++ src/libnfnetlink.c	(copia de trabajo)
@@ -799,16 +799,16 @@
 	assert(maxlen > 0);
 	assert(type >= 0);
 
-	if ((NFA_OK(nfa, nfa->nfa_len) + len) > maxlen) {
+	if (NFA_ALIGN(nfa->nfa_len) + len > maxlen) {
 		errno = ENOSPC;
 		return -1;
 	}
 
-	subnfa = (struct nfattr *)(((char *)nfa) + NFA_OK(nfa, nfa->nfa_len));
+	subnfa = (struct nfattr *)(((char *)nfa) + NFA_ALIGN(nfa->nfa_len));
 	subnfa->nfa_type = type;
 	subnfa->nfa_len = len;
 	memcpy(NFA_DATA(subnfa), data, alen);
-	nfa->nfa_len = (NLMSG_ALIGN(nfa->nfa_len) + len);
+	nfa->nfa_len = NFA_ALIGN(nfa->nfa_len) + len;
 
 	return 0;
 }

Re: Something like a bug

[Rick Xu]

On Tue, Apr 15, 2008 at 11:47 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Rick Xu wrote:
>  > int nfnl_nfa_addattr_l(struct nfattr *nfa, int maxlen, int type,
>  >                const void *data, int alen)
>  > {
>  >  ......
>  >
>  >     if ((NFA_OK(nfa, nfa->nfa_len) + len) > maxlen) {
>  >         errno = ENOSPC;
>  >         return -1;
>  >     }
>  >
>  >     subnfa = (struct nfattr *)(((char *)nfa) + NFA_OK(nfa, nfa->nfa_len));
>  >
>  > .....
>  > }
>  >
>  > NFA_OK looks so weird here. I think it should be:
>  > int nfnl_nfa_addattr_l(struct nfattr *nfa, int maxlen, int type,
>  >                const void *data, int alen)
>  > {
>  >  ......
>  >
>  >     if ( NFA_ALIGN(nfa->nfa_len) + NFA_LENGTH(len) > maxlen) {
>  >         errno = ENOSPC;
>  >         return -1;
>  >     }
>  >
>  >     subnfa = (struct nfattr *)(((char *)nfa) + NFA_ALIGN( nfa->nfa_len));
>  >
>  > .....
>  > }
>  >
>  > Please kindly correct me if I made a mistake.
>
>
>  Indeed, that function is completely broken but it does not have any
>  known client.
Yes, I see, I didn't find a client using this function.

>  It seems a leftover of the days when we didn't have anything better to
>  nest attributes, so that it's completely useless these days. I have
>  fixed in SVN anyway. Patch attached.
thanks for your patch,  people will be confused with the original code

>
>  --
>  "Los honestos son inadaptados sociales" -- Les Luthiers
>
> Index: configure.in
>  ===================================================================
>  --- configure.in        (revisión: 7400)
>  +++ configure.in        (copia de trabajo)
>  @@ -4,7 +4,7 @@
>
>   AC_CANONICAL_SYSTEM
>
>  -AM_INIT_AUTOMAKE(libnfnetlink, 0.0.33)
>  +AM_INIT_AUTOMAKE(libnfnetlink, 0.0.34)
>
>   AC_PROG_CC
>   AC_EXEEXT
>  Index: src/libnfnetlink.c
>  ===================================================================
>  --- src/libnfnetlink.c  (revisión: 7400)
>  +++ src/libnfnetlink.c  (copia de trabajo)
>  @@ -799,16 +799,16 @@
>         assert(maxlen > 0);
>         assert(type >= 0);
>
>  -       if ((NFA_OK(nfa, nfa->nfa_len) + len) > maxlen) {
>  +       if (NFA_ALIGN(nfa->nfa_len) + len > maxlen) {
>                 errno = ENOSPC;
>                 return -1;
>         }
>
>  -       subnfa = (struct nfattr *)(((char *)nfa) + NFA_OK(nfa, nfa->nfa_len));
>  +       subnfa = (struct nfattr *)(((char *)nfa) + NFA_ALIGN(nfa->nfa_len));
>         subnfa->nfa_type = type;
>         subnfa->nfa_len = len;
>         memcpy(NFA_DATA(subnfa), data, alen);
>  -       nfa->nfa_len = (NLMSG_ALIGN(nfa->nfa_len) + len);
>  +       nfa->nfa_len = NFA_ALIGN(nfa->nfa_len) + len;
>
>         return 0;
>   }
>
>



-- 
Thanks,
Rick
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Something like a bug

[Jan Engelhardt]

On Tuesday 2008-04-15 18:00, Rick Xu wrote:
>> Index: configure.in
>>  ===================================================================
>>  --- configure.in        (revisión: 7400)
>>  +++ configure.in        (copia de trabajo)
>>  @@ -4,7 +4,7 @@
>>
>>   AC_CANONICAL_SYSTEM
>>
>>  -AM_INIT_AUTOMAKE(libnfnetlink, 0.0.33)
>>  +AM_INIT_AUTOMAKE(libnfnetlink, 0.0.34)
>>
>>   AC_PROG_CC
>>   AC_EXEEXT

Just btw because I noticed it. AM_INIT_AUTOMAKE usually
takes 0 arguments, as per `info automake`:

     The second, deprecated, form of `AM_INIT_AUTOMAKE' has two required
     arguments: the package and the version number.  This form is
     obsolete because the PACKAGE and VERSION can be obtained from
     Autoconf's `AC_INIT' macro (which itself has an old and a new
     form).

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Something like a bug

[Pablo Neira Ayuso]

Jan Engelhardt wrote:
> On Tuesday 2008-04-15 18:00, Rick Xu wrote:
>>> Index: configure.in
>>>  ===================================================================
>>>  --- configure.in        (revisión: 7400)
>>>  +++ configure.in        (copia de trabajo)
>>>  @@ -4,7 +4,7 @@
>>>
>>>   AC_CANONICAL_SYSTEM
>>>
>>>  -AM_INIT_AUTOMAKE(libnfnetlink, 0.0.33)
>>>  +AM_INIT_AUTOMAKE(libnfnetlink, 0.0.34)
>>>
>>>   AC_PROG_CC
>>>   AC_EXEEXT
> 
> Just btw because I noticed it. AM_INIT_AUTOMAKE usually
> takes 0 arguments, as per `info automake`:
> 
>      The second, deprecated, form of `AM_INIT_AUTOMAKE' has two required
>      arguments: the package and the version number.  This form is
>      obsolete because the PACKAGE and VERSION can be obtained from
>      Autoconf's `AC_INIT' macro (which itself has an old and a new
>      form).

You seem to be more familiar with autostuff than me. I'd appreciate a
patch for this. Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Something like a bug

[Jan Engelhardt]

On Wednesday 2008-04-16 15:47, Pablo Neira Ayuso wrote:
>
>You seem to be more familiar with autostuff than me. I'd appreciate a
>patch for this. Thanks.

Index: configure.in
===================================================================
--- configure.in	(revision 7496)
+++ configure.in	(working copy)
@@ -1,10 +1,10 @@
 dnl Process this file with autoconf to create configure.
 
-AC_INIT
+AC_INIT(libnfnetlink, 0.0.34)
 
 AC_CANONICAL_SYSTEM
 
-AM_INIT_AUTOMAKE(libnfnetlink, 0.0.34)
+AM_INIT_AUTOMAKE
 
 AC_PROG_CC
 AC_EXEEXT

remove deprecated use of AC_INIT_AUTOMAKE [was Re: Something like a bug]

[Pablo Neira Ayuso]

Applied. I'd apply similar patches for other netfilter tools and libraries.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers