Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Aug 16, 2018

Conversation

nelhage
Copy link
Contributor

@nelhage nelhage commented Aug 5, 2018

Found via fuzzing.

@ethomson
Copy link
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
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
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
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
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.

None yet

3 participants