Re: [PATCH v4 10/15] bugreport: add config values from safelist

[Junio C Hamano <gitster@pobox.com>]

Emily Shaffer <emilyshaffer@google.com> writes:

> 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.

>
> 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);