[v3,01/10] config: initialize opts structure in repo_read_config()

Commit Message

Kazuhiro Kato via GitGitGadget April 11, 2019, 3:18 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Initialize opts structure in repo_read_config().

This change fixes a crash in later commit after a new field is added
to the structure.

In commit 3b256228a66f8587661481ef3e08259864f3ba2a, repo_read_config()
was added.  It only initializes 3 fields in the opts structure.  It is
passed to config_with_options() and then to do_git_config_sequence().
However, do_git_config_sequence() drops the opts on the floor and calls
git_config_from_file() rather than git_config_from_file_with_options(),
so that may be why this hasn't been a problem in the past.

Signed-off-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Jeff Hostetler <[e-mail]>
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jonathan Nieder April 12, 2019, 3:52 a.m. UTC | #1

Hi,

Jeff Hostetler wrote:

> Initialize opts structure in repo_read_config().

Good find.  I wonder if there are some flags we can turn on with
DEVELOPER=1 to prevent this kind of issue going undetected in the
future (or maybe this means we need to get the valgrind or ASan
testing modes to be fast enough for people to consistently run them).

[...]
>  config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Jonathan Nieder <[e-mail]>

Thanks.

Johannes Schindelin April 15, 2019, 2:34 p.m. UTC | #2

Hi Jonathan,

On Thu, 11 Apr 2019, Jonathan Nieder wrote:

> Jeff Hostetler wrote:
>
> > Initialize opts structure in repo_read_config().
>
> Good find.

Heh, it really was our CI that found it, and it was I (with valgrind's
help) who identified the problem and proposed the fix. So: thank you!

> I wonder if there are some flags we can turn on with
> DEVELOPER=1 to prevent this kind of issue going undetected in the
> future (or maybe this means we need to get the valgrind or ASan
> testing modes to be fast enough for people to consistently run them).

Sadly, I do not think that either is an option. Such uninitialized memory
is really hard to catch without in-depth analysis, so DEVELOPER=1 is out.
And `valgrind` (or the faster alternative, DrMemory) have to spend quite a
bit of time to do what they do, and it is unlikely that that could ever be
made faster.

A better approach might be static analysis (and I do not mean the diet
coke of static analysis that we run as part of our CI, but something as
powerful as Coverity).

Sadly, Coverity makes it super hard to switch off false positives
regarding e.g. our use of FLEX_ARRAY or strbuf's strbuf_slopbuf.

For quite a while, I wanted to play with [infer](https://fbinfer.com/), in
the hopes that it would be possible to do customize what cannot be
customized with Coverity. Alas, their use of OCaml (why do they make it so
hard?) puts quite the bit of a road block ahead of me in that endeavor.

Ciao,
Dscho

Patch

diff --git a/config.c b/config.c
index 0f0cdd8c0f..8e5f45fa20 100644
--- a/config.c
+++ b/config.c
@@ -2011,7 +2011,7 @@  int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
 /* Functions use to read configuration from a repository */
 static void repo_read_config(struct repository *repo)
 {
-	struct config_options opts;
+	struct config_options opts = {0};
 
 	opts.respect_includes = 1;
 	opts.commondir = repo->commondir;