libmnl misc patches Nov 11

libmnl misc patches Nov 11

[Jan Engelhardt]

Please review, and if ok, apply.

__

The following changes since commit 415a51b34293eac33a7d2152b909f89e5af4ed10:

  Merge branch 'master' of git://dev.medozas.de/libmnl (2010-11-07 20:53:15 +0100)

are available in the git repository at:

  git://dev.medozas.de/libmnl master

Jan Engelhardt (5):
      socket: constify a struct sockaddr_nl
      include: use C++ headers in C++ mode
      nlmsg: use bool for all _ok functions
      attr: remove redundant check for NULL
      attr: avoid multiple definition of hidden variable

 include/libmnl/libmnl.h |   41 +++++++++++++++++++++--------------------
 src/attr.c              |   23 ++++++++---------------
 src/nlmsg.c             |    2 +-
 src/socket.c            |    2 +-
 4 files changed, 31 insertions(+), 37 deletions(-)

[PATCH 1/5] socket: constify a struct sockaddr_nl

[Jan Engelhardt]

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 src/socket.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/socket.c b/src/socket.c
index c9afd34..cc997df 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -171,7 +171,7 @@ int mnl_socket_bind(struct mnl_socket *nl, unsigned int groups, pid_t pid)
  */
 int mnl_socket_sendto(const struct mnl_socket *nl, const void *buf, size_t len)
 {
-	struct sockaddr_nl snl = {
+	static const struct sockaddr_nl snl = {
 		.nl_family = AF_NETLINK
 	};
 	return sendto(nl->fd, buf, len, 0, 
-- 
1.7.1

Re: [PATCH 1/5] socket: constify a struct sockaddr_nl

[Pablo Neira Ayuso]

On 11/11/10 00:08, Jan Engelhardt wrote:
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>

Applied, thanks.

[PATCH 2/5] include: use C++ headers in C++ mode

[Jan Engelhardt]

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/libmnl/libmnl.h |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
index 7094af2..37c502a 100644
--- a/include/libmnl/libmnl.h
+++ b/include/libmnl/libmnl.h
@@ -1,13 +1,16 @@
 #ifndef _LIBMNL_H_
 #define _LIBMNL_H_
 
-#include <stdio.h>
-#include <stdint.h>
+#ifdef __cplusplus
+#	include <cstdio>
+#	include <cstdint>
+#else
+#	include <stdbool.h> /* not in C++ */
+#	include <stdio.h>
+#	include <stdint.h>
+#endif
 #include <sys/socket.h> /* for sa_family_t */
 #include <linux/netlink.h>
-#ifndef __cplusplus
-#	include <stdbool.h>
-#endif
 
 #ifdef __cplusplus
 extern "C" {
-- 
1.7.1

Re: [PATCH 2/5] include: use C++ headers in C++ mode

[Pablo Neira Ayuso]

On 11/11/10 00:08, Jan Engelhardt wrote:
> diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
> index 7094af2..37c502a 100644
> --- a/include/libmnl/libmnl.h
> +++ b/include/libmnl/libmnl.h
> @@ -1,13 +1,16 @@
>  #ifndef _LIBMNL_H_
>  #define _LIBMNL_H_
>  
> -#include <stdio.h>
> -#include <stdint.h>
> +#ifdef __cplusplus
> +#	include <cstdio>
> +#	include <cstdint>
> +#else
> +#	include <stdbool.h> /* not in C++ */
> +#	include <stdio.h>
> +#	include <stdint.h>
> +#endif
>  #include <sys/socket.h> /* for sa_family_t */
>  #include <linux/netlink.h>
> -#ifndef __cplusplus
> -#	include <stdbool.h>
> -#endif

A c++ example compiles fine without this here, so do we really need
these extra ifdef's?

Re: [PATCH 2/5] include: use C++ headers in C++ mode

[Jan Engelhardt]

On Thursday 2010-11-11 13:57, Pablo Neira Ayuso wrote:

>On 11/11/10 00:08, Jan Engelhardt wrote:
>> diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
>> index 7094af2..37c502a 100644
>> --- a/include/libmnl/libmnl.h
>> +++ b/include/libmnl/libmnl.h
>> @@ -1,13 +1,16 @@
>>  #ifndef _LIBMNL_H_
>>  #define _LIBMNL_H_
>>  
>> -#include <stdio.h>
>> -#include <stdint.h>
>> +#ifdef __cplusplus
>> +#	include <cstdio>
>> +#	include <cstdint>
>> +#else
>> +#	include <stdbool.h> /* not in C++ */
>> +#	include <stdio.h>
>> +#	include <stdint.h>
>> +#endif
>>  #include <sys/socket.h> /* for sa_family_t */
>>  #include <linux/netlink.h>
>> -#ifndef __cplusplus
>> -#	include <stdbool.h>
>> -#endif
>
>A c++ example compiles fine without this here, so do we really need
>these extra ifdef's?

Well there is a purpose for having <cXXX> over <XXX.h> in C++. IIRC it 
was proper namespacing of Standard Library functions.

Re: [PATCH 2/5] include: use C++ headers in C++ mode

[Pablo Neira Ayuso]

On 11/11/10 13:59, Jan Engelhardt wrote:
> On Thursday 2010-11-11 13:57, Pablo Neira Ayuso wrote:
> 
>> On 11/11/10 00:08, Jan Engelhardt wrote:
>>> diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
>>> index 7094af2..37c502a 100644
>>> --- a/include/libmnl/libmnl.h
>>> +++ b/include/libmnl/libmnl.h
>>> @@ -1,13 +1,16 @@
>>>  #ifndef _LIBMNL_H_
>>>  #define _LIBMNL_H_
>>>  
>>> -#include <stdio.h>
>>> -#include <stdint.h>
>>> +#ifdef __cplusplus
>>> +#	include <cstdio>
>>> +#	include <cstdint>
>>> +#else
>>> +#	include <stdbool.h> /* not in C++ */
>>> +#	include <stdio.h>
>>> +#	include <stdint.h>
>>> +#endif
>>>  #include <sys/socket.h> /* for sa_family_t */
>>>  #include <linux/netlink.h>
>>> -#ifndef __cplusplus
>>> -#	include <stdbool.h>
>>> -#endif
>>
>> A c++ example compiles fine without this here, so do we really need
>> these extra ifdef's?
> 
> Well there is a purpose for having <cXXX> over <XXX.h> in C++. IIRC it 
> was proper namespacing of Standard Library functions.

I'm looking at other library code but I don't find any doing this. If
you can point to any, I'd appreciate it.

Re: [PATCH 2/5] include: use C++ headers in C++ mode

[Jan Engelhardt]

On Thursday 2010-11-11 14:15, Pablo Neira Ayuso wrote:
>>>> -#include <stdio.h>
>>>> -#include <stdint.h>
>>>> +#ifdef __cplusplus
>>>> +#	include <cstdio>
>>>> +#	include <cstdint>
>>>> +#else
>>>> +#	include <stdbool.h> /* not in C++ */
>>>> +#	include <stdio.h>
>>>> +#	include <stdint.h>
>>>> +#endif
>>>>  #include <sys/socket.h> /* for sa_family_t */
>>>>  #include <linux/netlink.h>
>>>> -#ifndef __cplusplus
>>>> -#	include <stdbool.h>
>>>> -#endif
>>>
>>> A c++ example compiles fine without this here, so do we really need
>>> these extra ifdef's?
>> 
>> Well there is a purpose for having <cXXX> over <XXX.h> in C++. IIRC it 
>> was proper namespacing of Standard Library functions.
>
>I'm looking at other library code but I don't find any doing this. If
>you can point to any, I'd appreciate it.

/usr/include/gmp.h from libgmp-devel, for example.

Re: [PATCH 2/5] include: use C++ headers in C++ mode

[Pablo Neira Ayuso]

On 13/11/10 19:19, Jan Engelhardt wrote:
> On Thursday 2010-11-11 14:15, Pablo Neira Ayuso wrote:
>>>>> -#include <stdio.h>
>>>>> -#include <stdint.h>
>>>>> +#ifdef __cplusplus
>>>>> +#	include <cstdio>
>>>>> +#	include <cstdint>
>>>>> +#else
>>>>> +#	include <stdbool.h> /* not in C++ */
>>>>> +#	include <stdio.h>
>>>>> +#	include <stdint.h>
>>>>> +#endif
>>>>>  #include <sys/socket.h> /* for sa_family_t */
>>>>>  #include <linux/netlink.h>
>>>>> -#ifndef __cplusplus
>>>>> -#	include <stdbool.h>
>>>>> -#endif
>>>>
>>>> A c++ example compiles fine without this here, so do we really need
>>>> these extra ifdef's?
>>>
>>> Well there is a purpose for having <cXXX> over <XXX.h> in C++. IIRC it 
>>> was proper namespacing of Standard Library functions.
>>
>> I'm looking at other library code but I don't find any doing this. If
>> you can point to any, I'd appreciate it.
> 
> /usr/include/gmp.h from libgmp-devel, for example.

Applied thanks Jan.

Re: [PATCH 2/5] include: use C++ headers in C++ mode

[Thomas Jarosch]

Hi Jan,

On Thursday, 11. November 2010 00:08:01 you wrote:
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
> ---
>  include/libmnl/libmnl.h |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
> index 7094af2..37c502a 100644
> --- a/include/libmnl/libmnl.h
> +++ b/include/libmnl/libmnl.h
> @@ -1,13 +1,16 @@
>  #ifndef _LIBMNL_H_
>  #define _LIBMNL_H_
> 
> -#include <stdio.h>
> -#include <stdint.h>
> +#ifdef __cplusplus
> +#	include <cstdio>
> +#	include <cstdint>
> +#else
> +#	include <stdbool.h> /* not in C++ */
> +#	include <stdio.h>
> +#	include <stdint.h>
> +#endif

just a side note regarding this old commit:

The header "<cstdint>" is part of the C++11 standard.
(http://www.cplusplus.com/reference/cstdint/)

g++ forces you to enable C++11 mode if you want to use it.
Tested with g++ (GCC) 4.4.4.

Do we really need this? It compiles fine without
the special headers inside the __cplusplus part.
Pablo asked the same back in 2010 ;)

Best regards,
Thomas

Re: [PATCH 2/5] include: use C++ headers in C++ mode

[Jan Engelhardt]

On Friday 2013-06-07 10:35, Thomas Jarosch wrote:

>> -#include <stdio.h>
>> -#include <stdint.h>
>> +#ifdef __cplusplus
>> +#	include <cstdio>
>> +#	include <cstdint>
>> +#else
>> +#	include <stdbool.h> /* not in C++ */
>> +#	include <stdio.h>
>> +#	include <stdint.h>
>> +#endif
>
>The header "<cstdint>" is part of the C++11 standard.
>(http://www.cplusplus.com/reference/cstdint/)
>
>g++ forces you to enable C++11 mode if you want to use it.
>Tested with g++ (GCC) 4.4.4.
>
>Do we really need this? It compiles fine without
>the special headers inside the __cplusplus part.
>Pablo asked the same back in 2010 ;)

Add an extra condition:

#if defined(__cplusplus) && __cplusplus >= 201100L

[PATCH 3/5] nlmsg: use bool for all _ok functions

[Jan Engelhardt]

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/libmnl/libmnl.h |    4 ++--
 src/attr.c              |    2 +-
 src/nlmsg.c             |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
index 37c502a..43388e5 100644
--- a/include/libmnl/libmnl.h
+++ b/include/libmnl/libmnl.h
@@ -52,7 +52,7 @@ extern struct nlmsghdr *mnl_nlmsg_put_header(void *buf);
 extern void *mnl_nlmsg_put_extra_header(struct nlmsghdr *nlh, size_t size);
 
 /* Netlink message iterators */
-extern int mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len);
+extern bool mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len);
 extern struct nlmsghdr *mnl_nlmsg_next(const struct nlmsghdr *nlh, int *len);
 
 /* Netlink sequence tracking */
@@ -121,7 +121,7 @@ extern int mnl_attr_validate(const struct nlattr *attr, enum mnl_attr_data_type
 extern int mnl_attr_validate2(const struct nlattr *attr, enum mnl_attr_data_type type, size_t len);
 
 /* TLV iterators */
-extern int mnl_attr_ok(const struct nlattr *attr, int len);
+extern bool mnl_attr_ok(const struct nlattr *attr, int len);
 extern struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len);
 
 #define mnl_attr_for_each(attr, nlh, offset)			\
diff --git a/src/attr.c b/src/attr.c
index c60e1f4..69fda0b 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -90,7 +90,7 @@ void *mnl_attr_get_payload(const struct nlattr *attr)
  * The len parameter may be negative in the case of malformed messages during
  * attribute iteration, that is why we use a signed integer.
  */
-int mnl_attr_ok(const struct nlattr *attr, int len)
+bool mnl_attr_ok(const struct nlattr *attr, int len)
 {
 	return len >= (int)sizeof(struct nlattr) &&
 	       attr->nla_len >= sizeof(struct nlattr) &&
diff --git a/src/nlmsg.c b/src/nlmsg.c
index f89a9ec..b5c5c29 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -156,7 +156,7 @@ void *mnl_nlmsg_get_payload_offset(const struct nlmsghdr *nlh, size_t offset)
  * The len parameter may become negative in malformed messages during message
  * iteration, that is why we use a signed integer.
  */
-int mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len)
+bool mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len)
 {
 	return len >= (int)sizeof(struct nlmsghdr) &&
 	       nlh->nlmsg_len >= sizeof(struct nlmsghdr) &&
-- 
1.7.1

Re: [PATCH 3/5] nlmsg: use bool for all _ok functions

[Pablo Neira Ayuso]

On 11/11/10 00:08, Jan Engelhardt wrote:
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>

Applied, thanks.

[PATCH 4/5] attr: remove redundant check for NULL

[Jan Engelhardt]

Calling mnl_attr_parse with cb==NULL is pointless, because the
function will do nothing else.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 src/attr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/attr.c b/src/attr.c
index 69fda0b..e22a8ac 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -260,7 +260,7 @@ int mnl_attr_parse(const struct nlmsghdr *nlh, unsigned int offset,
 	int len = nlh->nlmsg_len - MNL_NLMSG_HDRLEN - MNL_ALIGN(offset);
 
 	while (mnl_attr_ok(attr, len)) {
-		if (cb && (ret = cb(attr, data)) <= MNL_CB_STOP)
+		if ((ret = cb(attr, data)) <= MNL_CB_STOP)
 			return ret;
 		attr = mnl_attr_next(attr, &len);
 	}
@@ -289,7 +289,7 @@ int mnl_attr_parse_nested(const struct nlattr *nested,
 	int len = mnl_attr_get_payload_len(nested);
 
 	while (mnl_attr_ok(attr, len)) {
-		if (cb && (ret = cb(attr, data)) <= MNL_CB_STOP)
+		if ((ret = cb(attr, data)) <= MNL_CB_STOP)
 			return ret;
 		attr = mnl_attr_next(attr, &len);
 	}
-- 
1.7.1

Re: [PATCH 4/5] attr: remove redundant check for NULL

[Pablo Neira Ayuso]

On 11/11/10 00:08, Jan Engelhardt wrote:
> Calling mnl_attr_parse with cb==NULL is pointless, because the
> function will do nothing else.

Indeed, applied.

[PATCH 5/5] attr: avoid multiple definition of hidden variable

[Jan Engelhardt]

When nesting two mnl_attr_for_each loops, the __len__ variable will be
declared twice, eliciting a warning when -Wshadow is turned on. There
can also be warnings in pre-C99 because declarations and code are
mixed. Do without any temporaries that are not explicitly specified as
macro parameters.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/libmnl/libmnl.h |   24 +++++++++++-------------
 src/attr.c              |   17 +++++------------
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
index 43388e5..9520a07 100644
--- a/include/libmnl/libmnl.h
+++ b/include/libmnl/libmnl.h
@@ -122,19 +122,17 @@ extern int mnl_attr_validate2(const struct nlattr *attr, enum mnl_attr_data_type
 
 /* TLV iterators */
 extern bool mnl_attr_ok(const struct nlattr *attr, int len);
-extern struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len);
-
-#define mnl_attr_for_each(attr, nlh, offset)			\
-	int __len__ = mnl_nlmsg_get_payload_len(nlh);		\
-	for (attr = mnl_nlmsg_get_payload_offset(nlh, offset);	\
-	     mnl_attr_ok(attr, __len__);			\
-	     attr = mnl_attr_next(attr, &(__len__)))
-
-#define mnl_attr_for_each_nested(attr, nest)			\
-	int __len__ = mnl_attr_get_len(nest);			\
-	for (attr = mnl_attr_get_payload(nest);			\
-	     mnl_attr_ok(attr, __len__);			\
-	     attr = mnl_attr_next(attr, &(__len__)))
+extern struct nlattr *mnl_attr_next(const struct nlattr *attr);
+
+#define mnl_attr_for_each(attr, nlh, offset) \
+	for ((attr) = mnl_nlmsg_get_payload_offset((nlh), (offset)); \
+	     mnl_attr_ok((attr), mnl_nlmsg_get_payload_tail(nlh) - (void *)(attr)); \
+	     (attr) = mnl_attr_next(attr))
+
+#define mnl_attr_for_each_nested(attr, nest) \
+	for ((attr) = mnl_attr_get_payload(nest); \
+	     mnl_attr_ok((attr), mnl_attr_get_payload(attr) + mnl_attr_get_payload_len(attr) - (void *)(attr)); \
+	     (attr) = mnl_attr_next(attr))
 
 /* TLV callback-based attribute parsers */
 typedef int (*mnl_attr_cb_t)(const struct nlattr *attr, void *data);
diff --git a/src/attr.c b/src/attr.c
index e22a8ac..5137395 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -106,9 +106,8 @@ bool mnl_attr_ok(const struct nlattr *attr, int len)
  * as parameter. You have to use mnl_attr_ok() to ensure that the next
  * attribute is valid.
  */
-struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
+struct nlattr *mnl_attr_next(const struct nlattr *attr)
 {
-	*len -= MNL_ALIGN(attr->nla_len);
 	return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
 }
 
@@ -256,14 +255,11 @@ int mnl_attr_parse(const struct nlmsghdr *nlh, unsigned int offset,
 		   mnl_attr_cb_t cb, void *data)
 {
 	int ret = MNL_CB_OK;
-	const struct nlattr *attr = mnl_nlmsg_get_payload_offset(nlh, offset);
-	int len = nlh->nlmsg_len - MNL_NLMSG_HDRLEN - MNL_ALIGN(offset);
+	const struct nlattr *attr;
 
-	while (mnl_attr_ok(attr, len)) {
+	mnl_attr_for_each(attr, nlh, offset)
 		if ((ret = cb(attr, data)) <= MNL_CB_STOP)
 			return ret;
-		attr = mnl_attr_next(attr, &len);
-	}
 	return ret;
 }
 
@@ -285,14 +281,11 @@ int mnl_attr_parse_nested(const struct nlattr *nested,
 			  mnl_attr_cb_t cb, void *data)
 {
 	int ret = MNL_CB_OK;
-	const struct nlattr *attr = mnl_attr_get_payload(nested);
-	int len = mnl_attr_get_payload_len(nested);
+	const struct nlattr *attr;
 
-	while (mnl_attr_ok(attr, len)) {
+	mnl_attr_for_each_nested(attr, nested)
 		if ((ret = cb(attr, data)) <= MNL_CB_STOP)
 			return ret;
-		attr = mnl_attr_next(attr, &len);
-	}
 	return ret;
 }
 
-- 
1.7.1

Re: [PATCH 5/5] attr: avoid multiple definition of hidden variable

[Pablo Neira Ayuso]

On 11/11/10 00:08, Jan Engelhardt wrote:
> When nesting two mnl_attr_for_each loops, the __len__ variable will be
> declared twice, eliciting a warning when -Wshadow is turned on. There
> can also be warnings in pre-C99 because declarations and code are
> mixed. Do without any temporaries that are not explicitly specified as
> macro parameters.

I like this spot, some question below:

> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
[...]
> diff --git a/src/attr.c b/src/attr.c
> index e22a8ac..5137395 100644
> --- a/src/attr.c
> +++ b/src/attr.c
> @@ -106,9 +106,8 @@ bool mnl_attr_ok(const struct nlattr *attr, int len)
>   * as parameter. You have to use mnl_attr_ok() to ensure that the next
>   * attribute is valid.
>   */
> -struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
> +struct nlattr *mnl_attr_next(const struct nlattr *attr)
>  {
> -	*len -= MNL_ALIGN(attr->nla_len);
>  	return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
>  }

If we remove the len parameter from mnl_attr_next(), we may access
memory that may be out of the message boundary in mnl_attr_ok().

Re: [PATCH 5/5] attr: avoid multiple definition of hidden variable

[Jan Engelhardt]

On Thursday 2010-11-11 14:08, Pablo Neira Ayuso wrote:
>On 11/11/10 00:08, Jan Engelhardt wrote:
>> When nesting two mnl_attr_for_each loops, the __len__ variable will be
>> declared twice, eliciting a warning when -Wshadow is turned on. There
>> can also be warnings in pre-C99 because declarations and code are
>> mixed. Do without any temporaries that are not explicitly specified as
>> macro parameters.
>
>I like this spot, some question below:
>
>> -struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
>> +struct nlattr *mnl_attr_next(const struct nlattr *attr)
>>  {
>> -	*len -= MNL_ALIGN(attr->nla_len);
>>  	return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
>>  }
>
>If we remove the len parameter from mnl_attr_next(), we may access
>memory that may be out of the message boundary in mnl_attr_ok().

Not that I can see; mnl_attr_ok tests for len >= sizeof(struct nlattr),
and len is tail minus attr.

Re: [PATCH 5/5] attr: avoid multiple definition of hidden variable

[Pablo Neira Ayuso]

On 11/11/10 14:25, Jan Engelhardt wrote:
> 
> On Thursday 2010-11-11 14:08, Pablo Neira Ayuso wrote:
>> On 11/11/10 00:08, Jan Engelhardt wrote:
>>> When nesting two mnl_attr_for_each loops, the __len__ variable will be
>>> declared twice, eliciting a warning when -Wshadow is turned on. There
>>> can also be warnings in pre-C99 because declarations and code are
>>> mixed. Do without any temporaries that are not explicitly specified as
>>> macro parameters.
>>
>> I like this spot, some question below:
>>
>>> -struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
>>> +struct nlattr *mnl_attr_next(const struct nlattr *attr)
>>>  {
>>> -	*len -= MNL_ALIGN(attr->nla_len);
>>>  	return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
>>>  }
>>
>> If we remove the len parameter from mnl_attr_next(), we may access
>> memory that may be out of the message boundary in mnl_attr_ok().
> 
> Not that I can see; mnl_attr_ok tests for len >= sizeof(struct nlattr),
> and len is tail minus attr.

mnl_attr_for_each() in your patch is OK, sorry. But, here:

+#define mnl_attr_for_each_nested(attr, nest) \
+	for ((attr) = mnl_attr_get_payload(nest); \
+	     mnl_attr_ok((attr), mnl_attr_get_payload(attr) +
mnl_attr_get_payload_len(attr) - (void *)(attr)); \
+	     (attr) = mnl_attr_next(attr))

Once we iterate over the last attribute in the nest, we iterate again to
check if there's any next. Then, mnl_attr_get_payload may access
attr->len for an attribute that doesn't belong the nest or (if the nest
is in the end of the message) an out of bound message access.

I think that we can add mnl_attr_get_payload_tail to make tail minus
attr, like in mnl_attr_for_each().

Re: [PATCH 5/5] attr: avoid multiple definition of hidden variable

[Jan Engelhardt]

On Thursday 2010-11-11 18:47, Pablo Neira Ayuso wrote:
>
>mnl_attr_for_each() in your patch is OK, sorry. But, here:
>
>+#define mnl_attr_for_each_nested(attr, nest) \
>+	for ((attr) = mnl_attr_get_payload(nest); \
>+	     mnl_attr_ok((attr), mnl_attr_get_payload(attr) +
>mnl_attr_get_payload_len(attr) - (void *)(attr)); \
>+	     (attr) = mnl_attr_next(attr))
>
>Once we iterate over the last attribute in the nest, we iterate again to
>check if there's any next. Then, mnl_attr_get_payload may access
>attr->len for an attribute that doesn't belong the nest or (if the nest
>is in the end of the message) an out of bound message access.

Indeed. Should have been

mnl_attr_ok((attr), mnl_attr_get_payload(nest) +
	mnl_attr_get_payload_len(nest) - (void *)(attr))

>I think that we can add mnl_attr_get_payload_tail to make tail minus
>attr, like in mnl_attr_for_each().