Skip to content

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

Merged
pks-t merged 5 commits intolibgit2: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 intolibgit2:masterfrom
nelhage:nelhage-config-no-section

Conversation

@nelhage
Copy link
Copy Markdown
Contributor

@nelhage nelhage commented Aug 5, 2018

Found via fuzzing.

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Aug 5, 2018

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
Copy link
Copy Markdown
Contributor Author

nelhage commented Aug 5, 2018

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
Copy link
Copy Markdown
Member

ethomson commented Aug 5, 2018

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
Copy link
Copy Markdown
Member

pks-t commented Aug 6, 2018

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
Copy link
Copy Markdown
Member

pks-t commented Aug 16, 2018

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