Skip to content

config_file: Don't crash on options without a section - #4750

Merged
pks-t merged 5 commits into
libgit2:masterfrom
nelhage:nelhage-config-no-section
Aug 16, 2018
Merged

config_file: Don't crash on options without a section#4750
pks-t merged 5 commits into
libgit2:masterfrom
nelhage:nelhage-config-no-section

Conversation

@nelhage

@nelhage nelhage commented Aug 5, 2018

Copy link
Copy Markdown
Contributor

Found via fuzzing.

@ethomson

ethomson commented Aug 5, 2018

Copy link
Copy Markdown
Member

Nice find - but git seems to parse configuration values without a section just fine. I think that actually we should:

if (current_section) {
    git_buf_puts(&buf, current_section);
    git_buf_putc(&buf, '.');
}

for (c = var_name; *c; c++)
    git_buf_putc(&buf, git__tolower(*c));

@nelhage

nelhage commented Aug 5, 2018

Copy link
Copy Markdown
Contributor Author

Huh, I found that if I added hi = 0 to my .git/config, I get

[nelhage@penguin:~/code/libgit2]$ git status
error: key does not contain a section: hi

That said, it doesn't seem to stop parsing at the error, which I believe this patch does. I'm happy to accept your proposal.

@ethomson

ethomson commented Aug 5, 2018

Copy link
Copy Markdown
Member

I suspected it might be a warning with some commands. (git config —list did not warn, but that was the extent of what I ran.)

Would you mind throwing a comment in that suggests that we should warn too? Once warnings land we can wire it up.

Thanks!

@pks-t

pks-t commented Aug 6, 2018

Copy link
Copy Markdown
Member

I'm not sure at all what happens in case we parse a config key without section. Could you please add a test that demonstrates that we do not fail at another place and that demonstrates what's expected to happen?

@pks-t
pks-t merged commit 43e7bf7 into libgit2:master Aug 16, 2018
@pks-t

pks-t commented Aug 16, 2018

Copy link
Copy Markdown
Member

Thanks for your fix and the test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants