[PATCH] iptables: set errno correctly in iptcc_chain_index_alloc

[PATCH] iptables: set errno correctly in iptcc_chain_index_alloc

[Phil Oester]

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

As reported by Robert Barnhardt, iptcc_chain_index_alloc does not populate
errno with the appropriate ENOMEM on allocation failures.  This causes
incorrect error messages to be passed back to user such as "can't initialize
iptables table 'X'" even if the issue was caused by OOM condition.  Fix
this by passing back ENOMEM if allocation failure occurs.

This closes bugzilla #619.

Phil

Signed-off-by: Phil Oester <kernel@linuxace.com>



[-- Attachment #2: patch-enomem --]
[-- Type: text/plain, Size: 450 bytes --]

diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index f0f7815..004b0ec 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -502,7 +502,8 @@ static int iptcc_chain_index_alloc(struct xtc_handle *h)
 	h->chain_index = malloc(array_mem);
 	if (h->chain_index == NULL && array_mem > 0) {
 		h->chain_index_sz = 0;
-		return -ENOMEM;
+		errno = ENOMEM;
+		return -1;
 	}
 	memset(h->chain_index, 0, array_mem);
 	h->chain_index_sz = array_elems;

Re: [PATCH] iptables: set errno correctly in iptcc_chain_index_alloc

[Florian Westphal]

Phil Oester <kernel@linuxace.com> wrote:
> As reported by Robert Barnhardt, iptcc_chain_index_alloc does not populate
> errno with the appropriate ENOMEM on allocation failures.  This causes
> incorrect error messages to be passed back to user such as "can't initialize
> iptables table 'X'" even if the issue was caused by OOM condition.  Fix
> this by passing back ENOMEM if allocation failure occurs.

Personally I think libraries should not change errno at all.

> diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
> index f0f7815..004b0ec 100644
> --- a/libiptc/libiptc.c
> +++ b/libiptc/libiptc.c
> @@ -502,7 +502,8 @@ static int iptcc_chain_index_alloc(struct xtc_handle *h)
>  	h->chain_index = malloc(array_mem);
>  	if (h->chain_index == NULL && array_mem > 0) {
>  		h->chain_index_sz = 0;
> -		return -ENOMEM;
> +		errno = ENOMEM;
> +		return -1;
>  	}

I don't understand how this changes anything?

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
int main(void) { errno = EINVAL;
	        void *v = malloc(0xffffffffffffffff);
		        if (v == 0) perror("malloc"); }

Yields "Cannot allocate memory", not "Invalid argument".

Re: [PATCH] iptables: set errno correctly in iptcc_chain_index_alloc

[Phil Oester]

On Thu, Jul 04, 2013 at 09:42:22AM +0200, Florian Westphal wrote:
> Personally I think libraries should not change errno at all.

OK, but then we output misleading error messages.

> I don't understand how this changes anything?

Simulate an out of memory condition with this patch

@@ -500,9 +500,11 @@ static int iptcc_chain_index_alloc(struct xtc_handle *h)
              array_elems, array_mem);

        h->chain_index = malloc(array_mem);
-       if (h->chain_index == NULL && array_mem > 0) {
+       //if (h->chain_index == NULL && array_mem > 0) {
+       if (1) {
                h->chain_index_sz = 0;

With the patch, the error message returned to user:

   ...can't initialize iptables table `filter': Memory allocation problem

without the patch:

   ...can't initialize iptables table `filter': Incompatible with this kernel

The former seems better, no?

Phil

Re: [PATCH] iptables: set errno correctly in iptcc_chain_index_alloc

[Florian Westphal]

Phil Oester <kernel@linuxace.com> wrote:
> Simulate an out of memory condition with this patch
> 
> @@ -500,9 +500,11 @@ static int iptcc_chain_index_alloc(struct xtc_handle *h)
>               array_elems, array_mem);
> 
>         h->chain_index = malloc(array_mem);
> -       if (h->chain_index == NULL && array_mem > 0) {
> +       //if (h->chain_index == NULL && array_mem > 0) {
> +       if (1) {
>                 h->chain_index_sz = 0;
> With the patch, the error message returned to user:
> 
>    ...can't initialize iptables table `filter': Memory allocation problem
> 
> without the patch:
> 
>    ...can't initialize iptables table `filter': Incompatible with this kernel
> 
> The former seems better, no?

Yes, but malloc didn't fail, so malloc didn't set errno.

My point is, that we should not muck with errno, especially
after libc functions that usually already set it on error.

Re: [PATCH] iptables: set errno correctly in iptcc_chain_index_alloc

[Phil Oester]

On Thu, Jul 04, 2013 at 06:33:25PM +0200, Florian Westphal wrote:
> My point is, that we should not muck with errno, especially
> after libc functions that usually already set it on error.

  # grep -c 'errno = ' libiptc/libiptc.c 
  52

But ok, we can avoid adding yet another instance and drop this patch.

Phil