| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAK7LNATRxYCx9sE1haw815votyA5K8GQrMPPh0Q+n3kYa=JsgA@mail.gmail.com>
Date: Fri, 9 Feb 2018 18:19:19 +0900
From: Masahiro Yamada <yamada.masahiro@...ionext.com>
To: Ulf Magnusson <ulfalizer@...il.com>
Cc: Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Kees Cook <keescook@...omium.org>,
Nicolas Pitre <nicolas.pitre@...aro.org>,
"Luis R . Rodriguez" <mcgrof@...e.com>,
Randy Dunlap <rdunlap@...radead.org>,
Sam Ravnborg <sam@...nborg.org>,
Michal Marek <michal.lkml@...kovi.net>,
Martin Schwidefsky <schwidefsky@...ibm.com>,
Pavel Machek <pavel@....cz>,
linux-s390 <linux-s390@...r.kernel.org>,
Jiri Kosina <jkosina@...e.cz>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 4/7] kconfig: support new special property shell=
2018-02-09 14:30 GMT+09:00 Ulf Magnusson <ulfalizer@...il.com>:
> On Fri, Feb 09, 2018 at 01:19:09AM +0900, Masahiro Yamada wrote:
>> This works with bool, int, hex, string types.
>>
>> For bool, the symbol is set to 'y' or 'n' depending on the exit value
>> of the command.
>>
>> For int, hex, string, the symbol is set to the value to the stdout
>> of the command. (only the first line of the stdout)
>>
>> The following shows how to write this and how it works.
>>
>> --------------------(example Kconfig)------------------
>> config srctree
>> string
>> option env="srctree"
>>
>> config CC
>> string
>> option env="CC"
>>
>> config CC_HAS_STACKPROTECTOR
>> bool
>> option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
>>
>> config CC_HAS_STACKPROTECTOR_STRONG
>> bool
>> option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
>>
>> config CC_VERSION
>> int
>> option shell="$srctree/scripts/gcc-version.sh $CC | sed 's/^0*//'"
>> help
>> gcc-version.sh returns 4 digits number. Unfortunately, the preceding
>> zero would cause 'number is invalid'. Cut it off.
>>
>> config CC_IS_CLANG
>> bool
>> option shell="$CC --version | grep -q clang"
>>
>> config CC_IS_GCC
>> bool
>> option shell="$CC --version | grep -q gcc"
>> -----------------------------------------------------
>>
>> $ make alldefconfig
>> scripts/kconfig/conf --alldefconfig Kconfig
>> #
>> # configuration written to .config
>> #
>> $ cat .config
>> #
>> # Automatically generated file; DO NOT EDIT.
>> # Linux Kernel Configuration
>> #
>> CONFIG_CC_HAS_STACKPROTECTOR=y
>> CONFIG_CC_HAS_STACKPROTECTOR_STRONG=y
>> CONFIG_CC_VERSION=504
>> # CONFIG_CC_IS_CLANG is not set
>> CONFIG_CC_IS_GCC=y
>>
>> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
>
> I know this is just an RFC/incomplete, but in case it's helpful:
Your comments are really helpful, as always!
>> ---
>>
>> scripts/kconfig/expr.h | 1 +
>> scripts/kconfig/kconf_id.c | 1 +
>> scripts/kconfig/lkc.h | 1 +
>> scripts/kconfig/menu.c | 3 ++
>> scripts/kconfig/symbol.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 80 insertions(+)
>>
>> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
>> index c16e82e..83029f92 100644
>> --- a/scripts/kconfig/expr.h
>> +++ b/scripts/kconfig/expr.h
>> @@ -183,6 +183,7 @@ enum prop_type {
>> P_IMPLY, /* imply BAR */
>> P_RANGE, /* range 7..100 (for a symbol) */
>> P_ENV, /* value from environment variable */
>> + P_SHELL, /* shell command */
>> P_SYMBOL, /* where a symbol is defined */
>> };
>>
>> diff --git a/scripts/kconfig/kconf_id.c b/scripts/kconfig/kconf_id.c
>> index 3ea9c5f..0db9d1c 100644
>> --- a/scripts/kconfig/kconf_id.c
>> +++ b/scripts/kconfig/kconf_id.c
>> @@ -34,6 +34,7 @@ static struct kconf_id kconf_id_array[] = {
>> { "defconfig_list", T_OPT_DEFCONFIG_LIST, TF_OPTION },
>> { "env", T_OPT_ENV, TF_OPTION },
>> { "allnoconfig_y", T_OPT_ALLNOCONFIG_Y, TF_OPTION },
>> + { "shell", T_OPT_SHELL, TF_OPTION },
>> };
>>
>> #define KCONF_ID_ARRAY_SIZE (sizeof(kconf_id_array)/sizeof(struct kconf_id))
>> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
>> index 4e23feb..8d05042 100644
>> --- a/scripts/kconfig/lkc.h
>> +++ b/scripts/kconfig/lkc.h
>> @@ -60,6 +60,7 @@ enum conf_def_mode {
>> #define T_OPT_DEFCONFIG_LIST 2
>> #define T_OPT_ENV 3
>> #define T_OPT_ALLNOCONFIG_Y 4
>> +#define T_OPT_SHELL 5
>>
>> struct kconf_id {
>> const char *name;
>> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
>> index 9922285..6254dfb 100644
>> --- a/scripts/kconfig/menu.c
>> +++ b/scripts/kconfig/menu.c
>> @@ -216,6 +216,9 @@ void menu_add_option(int token, char *arg)
>> case T_OPT_ENV:
>> prop_add_env(arg);
>> break;
>> + case T_OPT_SHELL:
>> + prop_add_shell(arg);
>> + break;
>> case T_OPT_ALLNOCONFIG_Y:
>> current_entry->sym->flags |= SYMBOL_ALLNOCONFIG_Y;
>> break;
>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
>> index 893eae6..02ac4f4 100644
>> --- a/scripts/kconfig/symbol.c
>> +++ b/scripts/kconfig/symbol.c
>> @@ -4,6 +4,7 @@
>> */
>>
>> #include <ctype.h>
>> +#include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>> #include <regex.h>
>> @@ -1370,6 +1371,8 @@ const char *prop_get_type_name(enum prop_type type)
>> return "prompt";
>> case P_ENV:
>> return "env";
>> + case P_SHELL:
>> + return "shell";
>> case P_COMMENT:
>> return "comment";
>> case P_MENU:
>> @@ -1420,3 +1423,74 @@ static void prop_add_env(const char *env)
>> else
>> menu_warn(current_entry, "environment variable %s undefined", env);
>> }
>> +
>> +static void prop_add_shell(const char *cmd)
>> +{
>> + struct symbol *sym, *sym2;
>> + struct property *prop;
>> + char *expanded_cmd;
>> + FILE *p;
>> + char stdout[256];
>
> Maybe this could be called stdout_buf to avoid confusing it with the
> stdio streams. Those are macros too, though glibc just does
>
> #define stdout stdout
Right. This is confusing. Will rename.
>> + int ret, len;
>> +
>> + sym = current_entry->sym;
>> + for_all_properties(sym, prop, P_SHELL) {
>> + sym2 = prop_get_symbol(prop);
>> + if (strcmp(sym2->name, cmd))
>> + menu_warn(current_entry, "redefining shell command symbol from %s",
>> + sym2->name);
>> + return;
>> + }
>> +
>> + prop = prop_alloc(P_SHELL, sym);
>> + prop->expr = expr_alloc_symbol(sym_lookup(cmd, SYMBOL_CONST));
>> +
>> + expanded_cmd = sym_expand_string_value(cmd);
>> +
>> + /* surround the command with ( ) in case it is piped commands */
>> + len = strlen(expanded_cmd);
>> + expanded_cmd = xrealloc(expanded_cmd, len + 3);
>> + memmove(expanded_cmd + 1, expanded_cmd, len);
>> + expanded_cmd[0] = '(';
>> + expanded_cmd[len + 1] = ')';
>> + expanded_cmd[len + 2] = 0;
>> +
>> + switch (sym->type) {
>> + case S_BOOLEAN:
>> + /* suppress both stdout and stderr. */
>> + len = strlen(expanded_cmd) + strlen(" >/dev/null 2>&1") + 1;
>> + expanded_cmd = realloc(expanded_cmd, len);
>
> Could use the new xrealloc().
Oops. I added xrealloc() for this, but missed to use it here, somehow.
>> + strcat(expanded_cmd, " >/dev/null 2>&1");
>> +
>> + ret = system(expanded_cmd);
>> + sym_add_default(sym, ret == 0 ? "y" : "n");
>> + break;
>> + case S_INT:
>> + case S_HEX:
>> + case S_STRING:
>> + /* suppress only stderr. we want to get stdout. */
>> + len = strlen(expanded_cmd) + strlen(" 2>/dev/null") + 1;
>> + expanded_cmd = realloc(expanded_cmd, len);
>
> Could use the new xrealloc().
>
>> + strcat(expanded_cmd, " 2>/dev/null");
>> +
>> + p = popen(expanded_cmd, "r");
>
> Should be pclose()d.
Good catch!
>> + if (!p)
>
> Could warn.
>
> Maybe it'd be helpful to warn if pclose() != 0 as well (non-zero exit
> status or obscure error).
Yes.
>> + goto free;
>> + if (fgets(stdout, sizeof(stdout), p)) {
>> + stdout[sizeof(stdout) - 1] = 0;
>
> fgets() already guarantees null termination if any characters are read.
>
> strncpy() is that evil one...
Oh, I was misunderstanding the fgets() behavior.
You are right. Will remove this unneeded termination.
>> + len = strlen(stdout);
>> + if (stdout[len - 1] == '\n')
>> + stdout[len - 1] = 0;
>> + } else {
>> + stdout[0] = 0;
>> + }
>> + sym_add_default(sym, stdout);
>> + break;
>> + default:
>> + menu_warn(current_entry, "unsupported type for shell command\n");
>> + break;
>> + }
>> +
>> +free:
>> + free(expanded_cmd);
>> +}
>> --
>> 2.7.4
>>
>
> I think the general behavior makes sense for user-assignable
> 'option shell' symbols too (though I don't know if they'd ever get used in
> practice):
>
> - The output of the shell command turns into a regular default on
> user-assignable symbols. User values can override that default.
>
> - For savedefconfig, user-assignable symbols get written out only if
> they have been changed from the default given by the shell
> command. They will be recalculated when the defconfig is used if
> they weren't changed.
>
> Maybe there's some too-obscure-to-worry about cases there, but it
> seems to work out pretty well.
>
Observant comments.
Both "option env=" and "option shell="
are turned into 'default' property after all.
config FOO
string
option env="foo"
could be written
config FOO
string
default env="foo"
(syntax is not so important)
Having multiple defaults with visibility control could be useful.
config FOO
string "foo"
default env="foo1" if CASE1
default env="foo2" if CASE2
shell= seems the same logic.
--
Best Regards
Masahiro Yamada
Powered by blists - more mailing lists