Skip to content

config: Port config_file_fuzzer to the new in-memory backend.#4842

Merged
pks-t merged 3 commits intolibgit2:masterfrom
nelhage:fuzz-config-memory
Oct 12, 2018
Merged

config: Port config_file_fuzzer to the new in-memory backend.#4842
pks-t merged 3 commits intolibgit2:masterfrom
nelhage:fuzz-config-memory

Conversation

@nelhage
Copy link
Copy Markdown
Contributor

@nelhage nelhage commented Oct 9, 2018

In quick testing, I see this hitting up to about 10k exec/s starting with an empty corpus on one core of my laptop, versus about 1500 for the old one.

Copy link
Copy Markdown
Member

@pks-t pks-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nelhage. This and submodules have in fact been my primary drivers to finally implement the memory backend.

if (written < 0)
continue;
total += written;
err = git_config_backend_from_string(&backend, (const char*)data, size);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: there's a space missing between const char and *) and same style comment as above

abort();
err = git_config_new(&cfg);
if (err != 0) {
goto out;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: if (err = git_config_new(&cfg)) < 0)

if (err != 0) {
goto out;
}
// Now owned by the config
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Seems like we aren't yet enforcing C90 for our fuzzing targets. I'm creating a PR for that. This should then be /* Now owned by the config */.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, we are enforcing C90. Would've expected it to at least throw a warning...

src/config_mem.c Outdated
}

if (git_buf_sets(&backend->cfg, cfg) < 0) {
if (git_buf_put(&backend->cfg, cfg, len) < 0) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be a put but a git_buf_set instead. Put would append the string, while set will replace the current string.

* @param len the length of the string pointed to by `cfg`
*/
extern int git_config_backend_from_string(git_config_backend **out, const char *cfg);
extern int git_config_backend_from_string(git_config_backend **out, const char *cfg, size_t len);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I guess

@pks-t pks-t merged commit 814e7ac into libgit2:master Oct 12, 2018
@pks-t
Copy link
Copy Markdown
Member

pks-t commented Oct 12, 2018

Thank you!

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.

2 participants