Re: [PATCH v4 10/15] bugreport: add config values from safelist
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 13, 2019, 21:45 UTC
- Message-ID
- <xmqq4ky3j3w4.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20191213004312.169753-11-emilyshaffer@google.com>
Emily Shaffer <emilyshaffer@google.com> writes:
Show 12 quoted lines
> Teach bugreport to gather the values of config options which are present > in 'bugreport-config-safelist.h'. > > Many config options are sensitive, and many Git add-ons use config > options which git-core does not know about; it is better only to gather > config options which we know to be safe, rather than excluding options > which we know to be unsafe. > > Taking the build-time generated array and putting it into a set saves us > time - since git_config_bugreport() is called for every option the user > has configured, performing option lookup in constant time is a useful > optimization.
Interesting. I actually was expecting the look-up to go the other way around. We know the safe keys, so iterate over them and grab their values, if defined. No need for hashes or anything, but just a simple linear list of safe stuff.
But that is too simple-minded. If we wanted to safelist foo.*.bar, where '*' can be anything, walking on the list of safe variables would not work. We must have a hash table that knows "foo.*.bar" is allowed, and while walking all the configuration keys, when we see foo.a.bar, we consult "foo.*.bar" as well as "foo.a.bar" to see if it is whitelisted, or something like that.
But then I am not sure if this implementation does something like this for three-level names. If not, I do not see much point in use of the hash there either.
Puzzled.
Show 113 quoted lines
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> bugreport.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/bugreport.c b/bugreport.c
> index 759cc0b0f8..1fca28f0b9 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -6,6 +6,9 @@
> #include "help.h"
> #include <gnu/libc-version.h>
> #include "run-command.h"
> +#include "config.h"
> +#include "bugreport-config-safelist.h"
> +#include "khash.h"
>
> static void get_http_version_info(struct strbuf *http_info)
> {
> @@ -18,6 +21,41 @@ static void get_http_version_info(struct strbuf *http_info)
> strbuf_addstr(http_info, "'git-http-fetch -V' not supported\n");
> }
>
> +KHASH_INIT(cfg_set, const char*, int, 0, kh_str_hash_func, kh_str_hash_equal);
> +
> +struct cfgset {
> + kh_cfg_set_t set;
> +};
> +
> +struct cfgset safelist;
> +
> +static void cfgset_init(struct cfgset *set, size_t initial_size)
> +{
> + memset(&set->set, 0, sizeof(set->set));
> + if (initial_size)
> + kh_resize_cfg_set(&set->set, initial_size);
> +}
> +
> +static int cfgset_insert(struct cfgset *set, const char *cfg_key)
> +{
> + int added;
> + kh_put_cfg_set(&set->set, cfg_key, &added);
> + printf("ESS: added %s\n", cfg_key);
> + return !added;
> +}
> +
> +static int cfgset_contains(struct cfgset *set, const char *cfg_key)
> +{
> + khiter_t pos = kh_get_cfg_set(&set->set, cfg_key);
> + return pos != kh_end(&set->set);
> +}
> +
> +static void cfgset_clear(struct cfgset *set)
> +{
> + kh_release_cfg_set(&set->set);
> + cfgset_init(set, 0);
> +}
> +
> static void get_system_info(struct strbuf *sys_info)
> {
> struct strbuf version_info = STRBUF_INIT;
> @@ -53,6 +91,36 @@ static void get_system_info(struct strbuf *sys_info)
> strbuf_complete_line(sys_info);
> }
>
> +static void gather_safelist()
> +{
> + int index;
> + int safelist_len = sizeof(bugreport_config_safelist) / sizeof(const char *);
> + cfgset_init(&safelist, safelist_len);
> + for (index = 0; index < safelist_len; index++)
> + cfgset_insert(&safelist, bugreport_config_safelist[index]);
> +
> +}
> +
> +static int git_config_bugreport(const char *var, const char *value, void *cb)
> +{
> + struct strbuf *config_info = (struct strbuf *)cb;
> +
> + if (cfgset_contains(&safelist, var))
> + strbuf_addf(config_info,
> + "%s (%s) : %s\n",
> + var, config_scope_to_string(current_config_scope()),
> + value);
> +
> + return 0;
> +}
> +
> +static void get_safelisted_config(struct strbuf *config_info)
> +{
> + gather_safelist();
> + git_config(git_config_bugreport, config_info);
> + cfgset_clear(&safelist);
> +}
> +
> static const char * const bugreport_usage[] = {
> N_("git bugreport [-o|--output <file>]"),
> NULL
> @@ -114,10 +182,12 @@ int cmd_main(int argc, const char **argv)
>
> get_bug_template(&buffer);
>
> - // add other contents
> get_header(&buffer, "System Info");
> get_system_info(&buffer);
>
> + get_header(&buffer, "Safelisted Config Info");
> + get_safelisted_config(&buffer);
> +
> report = fopen_for_writing(report_path.buf);
> strbuf_write(&buffer, report);
> fclose(report);