| Message ID | 20240627231919.2461945-1-elsk@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] kconfig: recursive checks drop file/lineno | expand |
On Fri, Jun 28, 2024 at 8:19 AM HONG Yifan <elsk@google.com> wrote: > > This prevents segfault when getting filename and lineno in recursive > checks. > > If the following snippet is found in Kconfig: > > [Test code 1] > > config FOO > bool > depends on BAR > select BAR > > ... without BAR defined; then if one runs `make tinyconfig`, there is a > segfault. You do not need to mention tinyconfig because the same error happens on any command. ... without BAR defined, there is a segfault. > > Kconfig:34:error: recursive dependency detected! > Kconfig:34: symbol FOO depends on BAR > make[4]: *** [scripts/kconfig/Makefile:85: allnoconfig] Segmentation fault > > This is because of the following. BAR is a fake entry created by > sym_lookup() with prop being NULL. In the recursive check, there is a > NULL check for prop to fall back to stack->sym->prop if stack->prop is > NULL. However, in this case, stack->sym points to the fake BAR entry > created by sym_lookup(), so prop is still NULL. prop was then referenced > without additional NULL checks, causing segfault. > > As the previous email thread suggests, the file and lineno for select is > also wrong: > > [Test code 2] > > config FOO > bool > > config BAR > bool > > config FOO > bool "FOO" > depends on BAR > select BAR > > $ make defconfig > *** Default configuration is based on 'x86_64_defconfig' > Kconfig:1:error: recursive dependency detected! > Kconfig:1: symbol FOO depends on BAR > Kconfig:4: symbol BAR is selected by FOO > [...] > > Kconfig:4 should be Kconfig:10. > > This patch deletes the wrong and segfault-prone filename/lineno > inference completely. With this patch, Test code 1 yields: > > error: recursive dependency detected! > symbol FOO depends on BAR > symbol BAR is selected by FOO > > Link: https://lore.kernel.org/linux-kbuild/20240620211112.500465-1-elsk@google.com/ > Signed-off-by: HONG Yifan <elsk@google.com> > > -- > v3: Rebase on top of > https://lore.kernel.org/linux-kbuild/20240626182212.3758235-1-masahiroy@kernel.org/T/#t > & resolve merge conflicts. Fix > scripts/kconfig/tests/err_recursive_dep/expected_stderr > v2: Delete all filenames/lineno completely as suggested by > masahiroy@kernel.org > --- > scripts/kconfig/symbol.c | 40 +++++++------------ > .../tests/err_recursive_dep/expected_stderr | 36 ++++++++--------- > 2 files changed, 33 insertions(+), 43 deletions(-) > > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c > index c05d188a1857..e22c8769f44f 100644 > --- a/scripts/kconfig/symbol.c > +++ b/scripts/kconfig/symbol.c > @@ -1068,10 +1068,10 @@ static void sym_check_print_recursive(struct symbol *last_sym) > { > struct dep_stack *stack; > struct symbol *sym, *next_sym; > - struct menu *menu = NULL; > struct menu *choice; > struct property *prop; > struct dep_stack cv_stack; > + enum prop_type type; > > choice = sym_get_choice_menu(last_sym); > if (choice) { > @@ -1094,49 +1094,39 @@ static void sym_check_print_recursive(struct symbol *last_sym) > if (prop == NULL) > prop = stack->sym->prop; As I said in the v2 review, please remove: if (prop == NULL) prop = stack->sym->prop; These two lines are useless. > - /* for choice values find the menu entry (used below) */ > - if (sym_is_choice(sym) || sym_is_choice_value(sym)) { > - for (prop = sym->prop; prop; prop = prop->next) { > - menu = prop->menu; > - if (prop->menu) > - break; > - } > - } > + if (prop == NULL) > + type = P_UNKNOWN; > + else > + type = prop->type; > + Bikeshed: I personally prefer this: type = prop ? prop->type : P_UNKNOWN; > if (stack->sym == last_sym) > - fprintf(stderr, "%s:%d:error: recursive dependency detected!\n", > - prop->filename, prop->lineno); > + fprintf(stderr, "error: recursive dependency detected!\n"); > > if (sym_is_choice(next_sym)) { > choice = list_first_entry(&next_sym->menus, struct menu, link); > > - fprintf(stderr, "%s:%d:\tsymbol %s is part of choice block at %s:%d\n", > - menu->filename, menu->lineno, > + fprintf(stderr, "\tsymbol %s is part of choice block at %s:%d\n", > sym->name ? sym->name : "<choice>", > choice->filename, choice->lineno); > } else if (stack->expr == &sym->dir_dep.expr) { > - fprintf(stderr, "%s:%d:\tsymbol %s depends on %s\n", > - prop->filename, prop->lineno, > + fprintf(stderr, "\tsymbol %s depends on %s\n", > sym->name ? sym->name : "<choice>", > next_sym->name); > } else if (stack->expr == &sym->rev_dep.expr) { > - fprintf(stderr, "%s:%d:\tsymbol %s is selected by %s\n", > - prop->filename, prop->lineno, > + fprintf(stderr, "\tsymbol %s is selected by %s\n", > sym->name, next_sym->name); > } else if (stack->expr == &sym->implied.expr) { > - fprintf(stderr, "%s:%d:\tsymbol %s is implied by %s\n", > - prop->filename, prop->lineno, > + fprintf(stderr, "\tsymbol %s is implied by %s\n", > sym->name, next_sym->name); > } else if (stack->expr) { > - fprintf(stderr, "%s:%d:\tsymbol %s %s value contains %s\n", > - prop->filename, prop->lineno, > + fprintf(stderr, "\tsymbol %s %s value contains %s\n", > sym->name ? sym->name : "<choice>", > - prop_get_type_name(prop->type), > + prop_get_type_name(type), > next_sym->name); > } else { > - fprintf(stderr, "%s:%d:\tsymbol %s %s is visible depending on %s\n", > - prop->filename, prop->lineno, > + fprintf(stderr, "\tsymbol %s %s is visible depending on %s\n", > sym->name ? sym->name : "<choice>", > - prop_get_type_name(prop->type), > + prop_get_type_name(type), > next_sym->name); > } > }
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index c05d188a1857..e22c8769f44f 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -1068,10 +1068,10 @@ static void sym_check_print_recursive(struct symbol *last_sym) { struct dep_stack *stack; struct symbol *sym, *next_sym; - struct menu *menu = NULL; struct menu *choice; struct property *prop; struct dep_stack cv_stack; + enum prop_type type; choice = sym_get_choice_menu(last_sym); if (choice) { @@ -1094,49 +1094,39 @@ static void sym_check_print_recursive(struct symbol *last_sym) if (prop == NULL) prop = stack->sym->prop; - /* for choice values find the menu entry (used below) */ - if (sym_is_choice(sym) || sym_is_choice_value(sym)) { - for (prop = sym->prop; prop; prop = prop->next) { - menu = prop->menu; - if (prop->menu) - break; - } - } + if (prop == NULL) + type = P_UNKNOWN; + else + type = prop->type; + if (stack->sym == last_sym) - fprintf(stderr, "%s:%d:error: recursive dependency detected!\n", - prop->filename, prop->lineno); + fprintf(stderr, "error: recursive dependency detected!\n"); if (sym_is_choice(next_sym)) { choice = list_first_entry(&next_sym->menus, struct menu, link); - fprintf(stderr, "%s:%d:\tsymbol %s is part of choice block at %s:%d\n", - menu->filename, menu->lineno, + fprintf(stderr, "\tsymbol %s is part of choice block at %s:%d\n", sym->name ? sym->name : "<choice>", choice->filename, choice->lineno); } else if (stack->expr == &sym->dir_dep.expr) { - fprintf(stderr, "%s:%d:\tsymbol %s depends on %s\n", - prop->filename, prop->lineno, + fprintf(stderr, "\tsymbol %s depends on %s\n", sym->name ? sym->name : "<choice>", next_sym->name); } else if (stack->expr == &sym->rev_dep.expr) { - fprintf(stderr, "%s:%d:\tsymbol %s is selected by %s\n", - prop->filename, prop->lineno, + fprintf(stderr, "\tsymbol %s is selected by %s\n", sym->name, next_sym->name); } else if (stack->expr == &sym->implied.expr) { - fprintf(stderr, "%s:%d:\tsymbol %s is implied by %s\n", - prop->filename, prop->lineno, + fprintf(stderr, "\tsymbol %s is implied by %s\n", sym->name, next_sym->name); } else if (stack->expr) { - fprintf(stderr, "%s:%d:\tsymbol %s %s value contains %s\n", - prop->filename, prop->lineno, + fprintf(stderr, "\tsymbol %s %s value contains %s\n", sym->name ? sym->name : "<choice>", - prop_get_type_name(prop->type), + prop_get_type_name(type), next_sym->name); } else { - fprintf(stderr, "%s:%d:\tsymbol %s %s is visible depending on %s\n", - prop->filename, prop->lineno, + fprintf(stderr, "\tsymbol %s %s is visible depending on %s\n", sym->name ? sym->name : "<choice>", - prop_get_type_name(prop->type), + prop_get_type_name(type), next_sym->name); } } diff --git a/scripts/kconfig/tests/err_recursive_dep/expected_stderr b/scripts/kconfig/tests/err_recursive_dep/expected_stderr index 05d4ced70320..fc2e860af082 100644 --- a/scripts/kconfig/tests/err_recursive_dep/expected_stderr +++ b/scripts/kconfig/tests/err_recursive_dep/expected_stderr @@ -1,38 +1,38 @@ -Kconfig:5:error: recursive dependency detected! -Kconfig:5: symbol A depends on A +error: recursive dependency detected! + symbol A depends on A For a resolution refer to Documentation/kbuild/kconfig-language.rst subsection "Kconfig recursive dependency limitations" -Kconfig:11:error: recursive dependency detected! -Kconfig:11: symbol B is selected by B +error: recursive dependency detected! + symbol B is selected by B For a resolution refer to Documentation/kbuild/kconfig-language.rst subsection "Kconfig recursive dependency limitations" -Kconfig:17:error: recursive dependency detected! -Kconfig:17: symbol C1 depends on C2 -Kconfig:21: symbol C2 depends on C1 +error: recursive dependency detected! + symbol C1 depends on C2 + symbol C2 depends on C1 For a resolution refer to Documentation/kbuild/kconfig-language.rst subsection "Kconfig recursive dependency limitations" -Kconfig:27:error: recursive dependency detected! -Kconfig:27: symbol D1 depends on D2 -Kconfig:32: symbol D2 is selected by D1 +error: recursive dependency detected! + symbol D1 depends on D2 + symbol D2 is selected by D1 For a resolution refer to Documentation/kbuild/kconfig-language.rst subsection "Kconfig recursive dependency limitations" -Kconfig:37:error: recursive dependency detected! -Kconfig:37: symbol E1 depends on E2 -Kconfig:42: symbol E2 is implied by E1 +error: recursive dependency detected! + symbol E1 depends on E2 + symbol E2 is implied by E1 For a resolution refer to Documentation/kbuild/kconfig-language.rst subsection "Kconfig recursive dependency limitations" -Kconfig:49:error: recursive dependency detected! -Kconfig:49: symbol F1 default value contains F2 -Kconfig:51: symbol F2 depends on F1 +error: recursive dependency detected! + symbol F1 default value contains F2 + symbol F2 depends on F1 For a resolution refer to Documentation/kbuild/kconfig-language.rst subsection "Kconfig recursive dependency limitations" -Kconfig:60:error: recursive dependency detected! -Kconfig:60: symbol G depends on G +error: recursive dependency detected! + symbol G depends on G For a resolution refer to Documentation/kbuild/kconfig-language.rst subsection "Kconfig recursive dependency limitations"
This prevents segfault when getting filename and lineno in recursive checks. If the following snippet is found in Kconfig: [Test code 1] config FOO bool depends on BAR select BAR ... without BAR defined; then if one runs `make tinyconfig`, there is a segfault. Kconfig:34:error: recursive dependency detected! Kconfig:34: symbol FOO depends on BAR make[4]: *** [scripts/kconfig/Makefile:85: allnoconfig] Segmentation fault This is because of the following. BAR is a fake entry created by sym_lookup() with prop being NULL. In the recursive check, there is a NULL check for prop to fall back to stack->sym->prop if stack->prop is NULL. However, in this case, stack->sym points to the fake BAR entry created by sym_lookup(), so prop is still NULL. prop was then referenced without additional NULL checks, causing segfault. As the previous email thread suggests, the file and lineno for select is also wrong: [Test code 2] config FOO bool config BAR bool config FOO bool "FOO" depends on BAR select BAR $ make defconfig *** Default configuration is based on 'x86_64_defconfig' Kconfig:1:error: recursive dependency detected! Kconfig:1: symbol FOO depends on BAR Kconfig:4: symbol BAR is selected by FOO [...] Kconfig:4 should be Kconfig:10. This patch deletes the wrong and segfault-prone filename/lineno inference completely. With this patch, Test code 1 yields: error: recursive dependency detected! symbol FOO depends on BAR symbol BAR is selected by FOO Link: https://lore.kernel.org/linux-kbuild/20240620211112.500465-1-elsk@google.com/ Signed-off-by: HONG Yifan <elsk@google.com> -- v3: Rebase on top of https://lore.kernel.org/linux-kbuild/20240626182212.3758235-1-masahiroy@kernel.org/T/#t & resolve merge conflicts. Fix scripts/kconfig/tests/err_recursive_dep/expected_stderr v2: Delete all filenames/lineno completely as suggested by masahiroy@kernel.org --- scripts/kconfig/symbol.c | 40 +++++++------------ .../tests/err_recursive_dep/expected_stderr | 36 ++++++++--------- 2 files changed, 33 insertions(+), 43 deletions(-)