iproute2/m_ipt fix, opts was not set properly

iproute2/m_ipt fix, opts was not set properly

[Denys Fedoryshchenko]

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

Since opts global variable, if it is set to original_opts, it was done in 
local context of function, and global variable in result was pointing to 
freed memory. Sure result of that - segfault, it is possible to trigger it 
only while processing batch, cause only on next iteration opts was wrong.

There is two ways to fix that, both patches attached.
Please send me note, which way correct (at least i will learn something :-) )


[-- Attachment #2: ipt_m_v1.patch --]
[-- Type: text/x-diff, Size: 805 bytes --]

diff -Naur iproute2-original/tc/m_ipt.c iproute2-patched/tc/m_ipt.c
--- iproute2-original/tc/m_ipt.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/m_ipt.c	2008-08-05 19:20:20.000000000 +0000
@@ -162,7 +162,7 @@
 	return result;
 }
 
-static void free_opts(struct option *opts)
+static void free_opts(void)
 {
 	if (opts != original_opts) {
 		free(opts);
@@ -455,7 +455,7 @@
 		if (matches(argv[optind], "index") == 0) {
 			if (get_u32(&index, argv[optind + 1], 10)) {
 				fprintf(stderr, "Illegal \"index\"\n");
-				free_opts(opts);
+				free_opts();
 				return -1;
 			}
 			iok++;
@@ -513,7 +513,7 @@
 	*argv_p = argv;
 
 	optind = 1;
-	free_opts(opts);
+	free_opts();
 
 	return 0;
 
@@ -594,7 +594,7 @@
 		fprintf(f, " \n");
 
 	}
-	free_opts(opts);
+	free_opts();
 
 	return 0;
 }

[-- Attachment #3: ipt_m_v2.patch --]
[-- Type: text/x-diff, Size: 510 bytes --]

diff -Naur iproute2-original/tc/m_ipt.c iproute2-patched2/tc/m_ipt.c
--- iproute2-original/tc/m_ipt.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched2/tc/m_ipt.c	2008-08-05 19:22:55.000000000 +0000
@@ -162,10 +162,10 @@
 	return result;
 }
 
-static void free_opts(struct option *opts)
+static void free_opts(struct option *opts_local)
 {
-	if (opts != original_opts) {
-		free(opts);
+	if (opts_local != original_opts) {
+		free(opts_local);
 		opts = original_opts;
 		global_option_offset = 0;
 	}

Re: iproute2/m_ipt fix, opts was not set properly

[jamal]

Denys,

On Tue, 2008-05-08 at 22:24 +0300, Denys Fedoryshchenko wrote:
> Since opts global variable, if it is set to original_opts, it was done in 
> local context of function, and global variable in result was pointing to 
> freed memory. 
> Sure result of that - segfault, it is possible to trigger it 
> only while processing batch, cause only on next iteration opts was wrong.

Are you sure this fixes any segfault? example, the variant you have
below just changes a variable name:

-----
-static void free_opts(struct option *opts)
+static void free_opts(struct option *opts_local)
 {
-       if (opts != original_opts) {
-               free(opts);
+       if (opts_local != original_opts) {
+               free(opts_local);
                opts = original_opts;
                global_option_offset = 0;
        }
-------

You could respin the patch with comment "change bad smelling name of
a variable" - and that would be fine by me.

Please CC me on m_ipt or any of the other actions (as i have asked you
before).

cheers,
jamal

Re: iproute2/m_ipt fix, opts was not set properly

[Denys Fedoryshchenko]

On Wednesday 06 August 2008, jamal wrote:
> Are you sure this fixes any segfault? example, the variant you have
> below just changes a variable name:
>
> -----
> -static void free_opts(struct option *opts)
> +static void free_opts(struct option *opts_local)
>  {
> -       if (opts != original_opts) {
> -               free(opts);
> +       if (opts_local != original_opts) {
> +               free(opts_local);
>                 opts = original_opts;
>                 global_option_offset = 0;
>         }
> -------
>
> You could respin the patch with comment "change bad smelling name of
> a variable" - and that would be fine by me.

Make attention - before static void free_opts(struct option *opts) was 
shadowing global variable with same name *opts, after this patch - it will 
not shadow anymore.

Re: iproute2/m_ipt fix, opts was not set properly

[jamal]

On Wed, 2008-06-08 at 13:06 +0300, Denys Fedoryshchenko wrote:

> Make attention - before static void free_opts(struct option *opts) was 
> shadowing global variable with same name *opts, after this patch - it will 
> not shadow anymore.

Sorry Denys, unless my memory has lost the C scoping rules, i dont think
the shadowing you describe above is accurate. Let me take a closer look.

[BTW, this code was all copied from iptables - so if this bug exists
here, it likely still exists in iptables. The xtables changes will make
this code a lot more maintainable].

cheers,
jamal

Re: iproute2/m_ipt fix, opts was not set properly

[jamal]

On Wed, 2008-06-08 at 06:42 -0400, jamal wrote:

> Sorry Denys, unless my memory has lost the C scoping rules, i dont think
> the shadowing you describe above is accurate. Let me take a closer look.

Grrr. I see the bug; until you tested with batching, it probably hasnt
mattered.
I still dont see how your patch fixes it. But heres a fix that I think
will work:

1. change free_opts to be:

static void free_opts(struct option **opts)
{
        if (*opts != original_opts) {
                free(*opts);
                *opts = original_opts;
        }
}

2. Change invocations of free_opts to be:

free_opts(&opts);

cheers,
jamal

Re: iproute2/m_ipt fix, opts was not set properly

[Denys Fedoryshchenko]

On Wednesday 06 August 2008, jamal wrote:
> 1. change free_opts to be:
>
> static void free_opts(struct option **opts)
> {
>         if (*opts != original_opts) {
>                 free(*opts);
>                 *opts = original_opts;
>         }
> }
>
> 2. Change invocations of free_opts to be:
>
> free_opts(&opts);
>
> cheers,
> jamal
Does it work on my test batch?
Your way looks much more clean :-)
(I cannot test it properly, since i have modified iproute2 now)

Re: iproute2/m_ipt fix, opts was not set properly

[jamal]

On Wed, 2008-06-08 at 14:02 +0300, Denys Fedoryshchenko wrote:

> Does it work on my test batch?

I havent tested with your scenario as i am staring at your script
and trying to construct a simple case. It was just eyeballing.
The original code was not resetting the  contents "pointed to"
by opts. So on return from free_opts, nothing has changed really.

> Your way looks much more clean :-)
> (I cannot test it properly, since i have modified iproute2 now)

It should work but i will test it. The simple test case i can think of
is to have two ipt invocations with different parameters (eg mark and
set tos).
If you test it, please resubmit with new approach. I will let you know
my results (as i have a few vacation hours to kill this morning).

cheers,
jamal