[PATCH] kconfig/menu.c: fix multiple references to expressions in menu_add_prop()

[PATCH] kconfig/menu.c: fix multiple references to expressions in menu_add_prop()

[Dirk Gouders]

menu_add_prop() applies upper menus' visibilities to actual prompts
by AND-ing the prompts visibilities with the upper menus ones.

This creates a further reference to the menu's visibilities and when
the expression reduction functions do their work, they may remove or
modify expressions that have multiple references, thus causing
unpredictable side-effects.

The following example Kconfig constructs a case where this causes
problems: a menu and a prompt which's visibilities depend on the same
symbol.  When invoking mconf with this Kconfig and pressing "Z" we
see a problem caused by a free'd expression still referenced by the
menu's visibility:

------------------------------------------------------------------------
mainmenu "Kconfig Testing Configuration"

config VISIBLE
	def_bool n

config Placeholder
	bool "Place holder"

menu "Invisible"
	visible if VISIBLE

config TEST_VAR
	bool "Test option" if VISIBLE

endmenu
------------------------------------------------------------------------

This patch fixes this problem by creating copies of the menu's
visibility expressions before AND-ing them with the prompt's one.

Signed-off-by: Dirk Gouders <dirk@gouders.net>
---
 scripts/kconfig/menu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index b5c7d90..567939c 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -143,14 +143,25 @@ struct property *menu_add_prop(enum prop_type type, char *prompt, struct expr *e
 
 		/* Apply all upper menus' visibilities to actual prompts. */
 		if(type == P_PROMPT) {
+			struct expr *dup_expr;
 			struct menu *menu = current_entry;
 
 			while ((menu = menu->parent) != NULL) {
 				if (!menu->visibility)
 					continue;
+				/*
+				 * Do not add a reference to the
+				 * menu's visibility expression but
+				 * use a copy of it.  Otherwise the
+				 * expression reduction functions
+				 * will modify expressions that have
+				 * multiple references which can
+				 * cause unwanted side-effects.
+				 */
+				dup_expr = expr_copy(menu->visibility);
+
 				prop->visible.expr
-					= expr_alloc_and(prop->visible.expr,
-							 menu->visibility);
+					= expr_alloc_and(prop->visible.expr, dup_expr);
 			}
 		}
 
-- 
1.8.2.1

Re: [PATCH] kconfig/menu.c: fix multiple references to expressions in menu_add_prop()

[Dirk Gouders]

CCing the original author of the code (commit 7ad1227818f09 "kconfig:
fix undesirable side effect of adding "visible" menu attribute").

Dirk

Dirk Gouders <dirk@gouders.net> writes:

> menu_add_prop() applies upper menus' visibilities to actual prompts
> by AND-ing the prompts visibilities with the upper menus ones.
>
> This creates a further reference to the menu's visibilities and when
> the expression reduction functions do their work, they may remove or
> modify expressions that have multiple references, thus causing
> unpredictable side-effects.
>
> The following example Kconfig constructs a case where this causes
> problems: a menu and a prompt which's visibilities depend on the same
> symbol.  When invoking mconf with this Kconfig and pressing "Z" we
> see a problem caused by a free'd expression still referenced by the
> menu's visibility:
>
> ------------------------------------------------------------------------
> mainmenu "Kconfig Testing Configuration"
>
> config VISIBLE
> 	def_bool n
>
> config Placeholder
> 	bool "Place holder"
>
> menu "Invisible"
> 	visible if VISIBLE
>
> config TEST_VAR
> 	bool "Test option" if VISIBLE
>
> endmenu
> ------------------------------------------------------------------------
>
> This patch fixes this problem by creating copies of the menu's
> visibility expressions before AND-ing them with the prompt's one.
>
> Signed-off-by: Dirk Gouders <dirk@gouders.net>
> ---
>  scripts/kconfig/menu.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index b5c7d90..567939c 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -143,14 +143,25 @@ struct property *menu_add_prop(enum prop_type type, char *prompt, struct expr *e
>  
>  		/* Apply all upper menus' visibilities to actual prompts. */
>  		if(type == P_PROMPT) {
> +			struct expr *dup_expr;
>  			struct menu *menu = current_entry;
>  
>  			while ((menu = menu->parent) != NULL) {
>  				if (!menu->visibility)
>  					continue;
> +				/*
> +				 * Do not add a reference to the
> +				 * menu's visibility expression but
> +				 * use a copy of it.  Otherwise the
> +				 * expression reduction functions
> +				 * will modify expressions that have
> +				 * multiple references which can
> +				 * cause unwanted side-effects.
> +				 */
> +				dup_expr = expr_copy(menu->visibility);
> +
>  				prop->visible.expr
> -					= expr_alloc_and(prop->visible.expr,
> -							 menu->visibility);
> +					= expr_alloc_and(prop->visible.expr, dup_expr);
>  			}
>  		}

Re: [PATCH] kconfig/menu.c: fix multiple references to expressions in menu_add_prop()

[Yann E. MORIN]

Dirk, All,

On 2013-05-21 10:54 +0200, Dirk Gouders spake thusly:
> menu_add_prop() applies upper menus' visibilities to actual prompts
> by AND-ing the prompts visibilities with the upper menus ones.
[--SNIP--]
> This patch fixes this problem by creating copies of the menu's
> visibility expressions before AND-ing them with the prompt's one.
> 
> Signed-off-by: Dirk Gouders <dirk@gouders.net>
> ---
>  scripts/kconfig/menu.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index b5c7d90..567939c 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -143,14 +143,25 @@ struct property *menu_add_prop(enum prop_type type, char *prompt, struct expr *e
>  
>  		/* Apply all upper menus' visibilities to actual prompts. */
>  		if(type == P_PROMPT) {
> +			struct expr *dup_expr;

I'd rather this variable defined below:

>  			struct menu *menu = current_entry;
>  
>  			while ((menu = menu->parent) != NULL) {

... here, in the block where it is used, since it is not relevant
outside this block.

>  				if (!menu->visibility)
>  					continue;
> +				/*
> +				 * Do not add a reference to the
> +				 * menu's visibility expression but
> +				 * use a copy of it.  Otherwise the
> +				 * expression reduction functions
> +				 * will modify expressions that have
> +				 * multiple references which can
> +				 * cause unwanted side-effects.
> +				 */
> +				dup_expr = expr_copy(menu->visibility);

I wonder if/where this should be de-allocated.

> +
>  				prop->visible.expr
> -					= expr_alloc_and(prop->visible.expr,
> -							 menu->visibility);
> +					= expr_alloc_and(prop->visible.expr, dup_expr);
>  			}
>  		}
>  

I'm testing it right now. Thanks!

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Re: [PATCH] kconfig/menu.c: fix multiple references to expressions in menu_add_prop()

[Dirk Gouders]

Thanks for reviewing and testing a patch of mine, again, Yann.


"Yann E. MORIN" <yann.morin.1998@free.fr> writes:

> Dirk, All,
>
> On 2013-05-21 10:54 +0200, Dirk Gouders spake thusly:
>> menu_add_prop() applies upper menus' visibilities to actual prompts
>> by AND-ing the prompts visibilities with the upper menus ones.
> [--SNIP--]
>> This patch fixes this problem by creating copies of the menu's
>> visibility expressions before AND-ing them with the prompt's one.
>> 
>> Signed-off-by: Dirk Gouders <dirk@gouders.net>
>> ---
>>  scripts/kconfig/menu.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>> 
>> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
>> index b5c7d90..567939c 100644
>> --- a/scripts/kconfig/menu.c
>> +++ b/scripts/kconfig/menu.c
>> @@ -143,14 +143,25 @@ struct property *menu_add_prop(enum prop_type type, char *prompt, struct expr *e
>>  
>>  		/* Apply all upper menus' visibilities to actual prompts. */
>>  		if(type == P_PROMPT) {
>> +			struct expr *dup_expr;
>
> I'd rather this variable defined below:
>
>>  			struct menu *menu = current_entry;
>>  
>>  			while ((menu = menu->parent) != NULL) {
>
> ... here, in the block where it is used, since it is not relevant
> outside this block.

Indeed, I will fix this.
I also noticed that I should fix some spelling (e.g. side effect instead
of side-effect).  I hope it is OK, to wait for your tests before sending
a v2.

>>  				if (!menu->visibility)
>>  					continue;
>> +				/*
>> +				 * Do not add a reference to the
>> +				 * menu's visibility expression but
>> +				 * use a copy of it.  Otherwise the
>> +				 * expression reduction functions
>> +				 * will modify expressions that have
>> +				 * multiple references which can
>> +				 * cause unwanted side-effects.
>> +				 */
>> +				dup_expr = expr_copy(menu->visibility);
>
> I wonder if/where this should be de-allocated.

Actually, I did not find any piece of code that systematically free()s
the allocated data structures and I also think that it would be good to
have such code, because that would have caused double-free()s and
therfore noticed us immediately when we create multiple references to
expressions.

My plan was, to first to fix this single problem and then take care for
a larger review of dynamically allocated memory.

I tested mconf for example with valgrind and the next thing I planned to
suggest is to make the kconfig code mostly "valgrind-clean".  But I
expect this to become a rather extensive change and would like to hear
if others also think it should be done.

>
>> +
>>  				prop->visible.expr
>> -					= expr_alloc_and(prop->visible.expr,
>> -							 menu->visibility);
>> +					= expr_alloc_and(prop->visible.expr, dup_expr);
>>  			}
>>  		}
>>  
>
> I'm testing it right now. Thanks!

Again, thanks for spending the time.

Dirk

> Regards,
> Yann E. MORIN.

Re: [PATCH] kconfig/menu.c: fix multiple references to expressions in menu_add_prop()

[Yann E. MORIN]

Dirk, All,

On 2013-05-23 12:25 +0200, Dirk Gouders spake thusly:
> "Yann E. MORIN" <yann.morin.1998@free.fr> writes:
> > On 2013-05-21 10:54 +0200, Dirk Gouders spake thusly:
> >> menu_add_prop() applies upper menus' visibilities to actual prompts
> >> by AND-ing the prompts visibilities with the upper menus ones.
> > [--SNIP--]
> >> This patch fixes this problem by creating copies of the menu's
> >> visibility expressions before AND-ing them with the prompt's one.
[--SNIP--]
> >> --- a/scripts/kconfig/menu.c
> >> +++ b/scripts/kconfig/menu.c
> >> @@ -143,14 +143,25 @@ struct property *menu_add_prop(enum prop_type type, char *prompt, struct expr *e
> >>  
> >>  		/* Apply all upper menus' visibilities to actual prompts. */
> >>  		if(type == P_PROMPT) {
> >> +			struct expr *dup_expr;
> >
> > I'd rather this variable defined below:
> >
> >>  			struct menu *menu = current_entry;
> >>  
> >>  			while ((menu = menu->parent) != NULL) {
> >
> > ... here, in the block where it is used, since it is not relevant
> > outside this block.
> 
> Indeed, I will fix this.
> I also noticed that I should fix some spelling (e.g. side effect instead
> of side-effect).  I hope it is OK, to wait for your tests before sending
> a v2.

If you're speaking about typoes in your patch, I'll just fix them here
(along with the variable move above), don't worry.

If you're speaking about typoes in the rest of the code, then please
submit a separate patch.

> >>  				if (!menu->visibility)
> >>  					continue;
> >> +				/*
> >> +				 * Do not add a reference to the
> >> +				 * menu's visibility expression but
> >> +				 * use a copy of it.  Otherwise the
> >> +				 * expression reduction functions
> >> +				 * will modify expressions that have
> >> +				 * multiple references which can
> >> +				 * cause unwanted side-effects.
> >> +				 */
> >> +				dup_expr = expr_copy(menu->visibility);
> >
> > I wonder if/where this should be de-allocated.
> 
> Actually, I did not find any piece of code that systematically free()s
> the allocated data structures

Indeed.
My question was a bit rhetorical.

> and I also think that it would be good to
> have such code, because that would have caused double-free()s and
> therfore noticed us immediately when we create multiple references to
> expressions.
> 
> My plan was, to first to fix this single problem and then take care for
> a larger review of dynamically allocated memory.

Yes, that would be awesome! :-)

> I tested mconf for example with valgrind and the next thing I planned to
> suggest is to make the kconfig code mostly "valgrind-clean".  But I
> expect this to become a rather extensive change and would like to hear
> if others also think it should be done.

Although a little bit of memory leak in kconfig is no big issue for the
kernel tree (since the frontends are rather short-lived programs), other
users of kconfig may use long-lived processes that would suffer from
memory leaks.

Fixing those would be a net gain, I believe.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Re: [PATCH] kconfig/menu.c: fix multiple references to expressions in menu_add_prop()

[Dirk Gouders]

"Yann E. MORIN" <yann.morin.1998@free.fr> writes:

> Dirk, All,
>
> On 2013-05-23 12:25 +0200, Dirk Gouders spake thusly:
>> "Yann E. MORIN" <yann.morin.1998@free.fr> writes:
>> > On 2013-05-21 10:54 +0200, Dirk Gouders spake thusly:
>> >> menu_add_prop() applies upper menus' visibilities to actual prompts
>> >> by AND-ing the prompts visibilities with the upper menus ones.
>> > [--SNIP--]
>> >> This patch fixes this problem by creating copies of the menu's
>> >> visibility expressions before AND-ing them with the prompt's one.
> [--SNIP--]
>> >> --- a/scripts/kconfig/menu.c
>> >> +++ b/scripts/kconfig/menu.c
>> >> @@ -143,14 +143,25 @@ struct property *menu_add_prop(enum prop_type type, char *prompt, struct expr *e
>> >>  
>> >>  		/* Apply all upper menus' visibilities to actual prompts. */
>> >>  		if(type == P_PROMPT) {
>> >> +			struct expr *dup_expr;
>> >
>> > I'd rather this variable defined below:
>> >
>> >>  			struct menu *menu = current_entry;
>> >>  
>> >>  			while ((menu = menu->parent) != NULL) {
>> >
>> > ... here, in the block where it is used, since it is not relevant
>> > outside this block.
>> 
>> Indeed, I will fix this.
>> I also noticed that I should fix some spelling (e.g. side effect instead
>> of side-effect).  I hope it is OK, to wait for your tests before sending
>> a v2.
>
> If you're speaking about typoes in your patch, I'll just fix them here
> (along with the variable move above), don't worry.

Thanks for fixing all that.

[SNIP]
>> Actually, I did not find any piece of code that systematically free()s
>> the allocated data structures
>
> Indeed.
> My question was a bit rhetorical.
>
>> and I also think that it would be good to
>> have such code, because that would have caused double-free()s and
>> therfore noticed us immediately when we create multiple references to
>> expressions.
>> 
>> My plan was, to first to fix this single problem and then take care for
>> a larger review of dynamically allocated memory.
>
> Yes, that would be awesome! :-)
>
>> I tested mconf for example with valgrind and the next thing I planned to
>> suggest is to make the kconfig code mostly "valgrind-clean".  But I
>> expect this to become a rather extensive change and would like to hear
>> if others also think it should be done.
>
> Although a little bit of memory leak in kconfig is no big issue for the
> kernel tree (since the frontends are rather short-lived programs), other
> users of kconfig may use long-lived processes that would suffer from
> memory leaks.
>
> Fixing those would be a net gain, I believe.

OK, I already started to care for a clean heap when mconf exits and
make good progress but there are still some bytes left ;-) -- and more
references to free'd expressions, I'm afraid.

I am planning to put these changes in multiple simple commits (so that
later possible bisecting will point to mainly one or few line changes)
and send them to you for review if that is OK to you.

Dirk