Re: [PATCH v3 4/9] bugreport: add config values from whitelist

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 28, 2019, 14:14 UTC
Message-ID: <nycvar.QRO.7.76.6.1910281506040.46@tvgsbejvaqbjf.bet>
In-Reply-To: <20191025025129.250049-5-emilyshaffer@google.com>

Hi Emily,

On Thu, 24 Oct 2019, Emily Shaffer wrote:

Show 7 quoted lines

> Teach bugreport to gather the values of config options which are present
> in 'git-bugreport-config-whitelist'.
>
> 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.

Should we still have the `// bugreport-exclude` comments, then?

Show 5 quoted lines

>
> Reading the whitelist into memory and sorting it saves us time -
> since git_config_bugreport() is called for every option the user has
> configured, limiting the file IO to one open/read/close and performing
> option lookup in sublinear time is a useful optimization.

Maybe we even want a hashmap? That would reduce the time complexity even further.

Show 26 quoted lines

> diff --git a/bugreport.c b/bugreport.c
> index ada54fe583..afa4836ab1 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -1,10 +1,24 @@
>  #include "cache.h"
>
>  #include "bugreport.h"
> +#include "config.h"
> +#include "exec-cmd.h"
>  #include "help.h"
>  #include "run-command.h"
>  #include "version.h"
>
> +/**
> + * A sorted list of config options which we will add to the bugreport. Managed
> + * by 'gather_whitelist(...)'.
> + */
> +struct string_list whitelist = STRING_LIST_INIT_DUP;
> +struct strbuf configs_and_values = STRBUF_INIT;
> +
> +// git version --build-options
> +// uname -a
> +// curl-config --version
> +// ldd --version
> +// echo $SHELL

These comments probably want to move to a single, C style comment, and they probably want to be introduced together with `get_system_info()`.

I also have to admit that I might have missed where `$SHELL` was added to the output...

Show 23 quoted lines

>  void get_system_info(struct strbuf *sys_info)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
> @@ -53,3 +67,39 @@ void get_system_info(struct strbuf *sys_info)
>  	argv_array_clear(&cp.args);
>  	strbuf_reset(&std_out);
>  }
> +
> +void gather_whitelist(struct strbuf *path)
> +{
> +	struct strbuf tmp = STRBUF_INIT;
> +	strbuf_read_file(&tmp, path->buf, 0);
> +	string_list_init(&whitelist, 1);
> +	string_list_split(&whitelist, tmp.buf, '\n', -1);
> +	string_list_sort(&whitelist);
> +}
> +
> +int git_config_bugreport(const char *var, const char *value, void *cb)
> +{
> +	if (string_list_has_string(&whitelist, var)) {
> +		strbuf_addf(&configs_and_values,
> +			    "%s : %s\n",
> +			    var, value);

A quite useful piece of information would be the config source. Not sure whether we can do that outside of `config.c` yet...

Show 11 quoted lines

> +	}
> +
> +	return 0;
> +}
> +
> +void get_whitelisted_config(struct strbuf *config_info)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +
> +	strbuf_addstr(&path, git_exec_path());
> +	strbuf_addstr(&path, "/git-bugreport-config-whitelist");

Hmm. I would have expected this patch to come directly after the patch 2/9 that generates that white-list, and I would also have expected that to be pre-sorted, and compiled in.

Do you want users to _edit_ the file in the exec path? In general, that path will be write-protected, though. A better alternative would probably be to compile in a hard-coded list, and to allow including more values e.g. by offering command-line options to specify config setting patterns. But if we allow patterns, we might actually want to have those exclusions to prevent sensitive data from being included.

Show 19 quoted lines

> +
> +	gather_whitelist(&path);
> +	strbuf_init(&configs_and_values, whitelist.nr);
> +
> +	git_config(git_config_bugreport, NULL);
> +
> +	strbuf_reset(config_info);
> +	strbuf_addbuf(config_info, &configs_and_values);
> +}
> diff --git a/bugreport.h b/bugreport.h
> index ba216acf3f..7413e7e1be 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -5,3 +5,10 @@
>   * The previous contents of sys_info will be discarded.
>   */
>  void get_system_info(struct strbuf *sys_info);
> +
> +/**

I also frequently use JavaDoc-style `/**`, but I am not sure that this is actually desired in Git's source code ;-)

Show 14 quoted lines

> + * Adds the values of the config items listed in
> + * 'git-bugreport-config-whitelist' to config_info. The previous contents of
> + * config_info will be discarded.
> + */
> +void get_whitelisted_config(struct strbuf *sys_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 7232d31be7..70fe0d2b85 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -56,6 +56,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	get_system_info(&buffer);
>  	strbuf_write(&buffer, report);
>
> +	add_header(report, "Whitelisted Config");

Quite honestly, I would like to avoid the term "whitelist" for good. How about "Selected config settings" instead?

Thanks, Dscho

Show 10 quoted lines

> +	get_whitelisted_config(&buffer);
> +	strbuf_write(&buffer, report);
> +
>  	fclose(report);
>
>  	launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>