| Message ID | 1369126451-11930-1-git-send-email-dirk@gouders.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > } > } -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" <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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); } }
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(-)