reflogs: fix behaviour around reflogs with newlines - #5275
Conversation
The code style of `git_stash_save` doesn't really match our current coding style. Update it to match our current policies more closely.
Currently, the reflog disallows any entries that have a message with newlines, as that would effectively break the reflog format, which may contain a single line per entry, only. Upstream git behaves a bit differently, though, especially when considering stashes: instead of rejecting any reflog entry with newlines, git will simply replace newlines with spaces. E.g. executing 'git stash push -m "foo\nbar"' will create a reflog entry with "foo bar" as entry message. This commit adjusts our own logic to stop rejecting commit messages with newlines. Previously, this logic was part of `git_reflog_append`, only. There is a second place though where we add reflog entries, which is the serialization code in the filesystem refdb. As it didn't contain any sanity checks whatsoever, the refdb would have been perfectly happy to write malformatted reflog entries to the disk. This is being fixed with the same logic as for the reflog itself.
The refdb_fs code to parse the reflog currently uses a hand-rolled parser. Convert it to use our `git_parse_ctx` structure instead.
In previous versions, libgit2 could be coerced into writing reflog messages with embedded newlines into the reflog by using `git_stash_save` with a message containing newlines. While the root cause is fixed now, it was noticed that upstream git is in fact able to read such corrupted reflog messages just fine. Make the reflog parser more lenient in order to just skip over malformatted reflog lines to bring us in line with git. This requires us to change an existing test that verified that we do indeed _fail_ to parse such logs.
5e9b256 to
7968e90
Compare
|
@pks-t Looks nice! Consider enumeration typedef enum git_reflog_component {
git_reflog_component_init, // init entry.
git_reflog_component_oid_old, // length > 0 && length <= OIDSIZE (20) && is_hex(character)
git_reflog_component_oid_cur, // length > 0 && length <= OIDSIZE (20) && is_hex(character)
git_reflog_component_commiter, // length > 0 && contains(name) && contains(email) && contains(time)
git_reflog_component_msg, // length >= 0 && last_at_('\n')
git_reflog_component_finalize, // finalize. Put entry into array.
} git_reflog_component;Or simpler typedef enum parsing_step {
init,
oid_old,
oid_cur,
commiter,
msg,
finalize,
};And how it could be used. component = git_reflog_component_init;
while(buf_size > GIT_REFLOG_SIZE_MIN) {
switch (component) {
case git_reflog_component_init: {
entry = git__calloc(1, sizeof(git_reflog_entry));
GIT_ERROR_CHECK_ALLOC(entry);
entry->committer = git__calloc(1, sizeof(git_signature));
GIT_ERROR_CHECK_ALLOC(entry->committer);
component = git_reflog_component_oid_old;
break;
}
case git_reflog_component_oid_old: {
if (git_oid_fromstrn(&entry->oid_old, buf, GIT_OID_HEXSZ) < 0) {
// error occurred.
// we should reset cycle.
// and also we don't need to change component.
// cleanup.
invalid_character_position = git_oid_find_invalid_charater_position_at_strn(buf, GIT_OID_HEXSZ);
if (invalid_character_position < 0) { // error occurred, so, cleanup and leave.
error_occurred = -1;
}
else { // invalid character, seek to position following invalid character and try to get oid again.
git_reflog_entry__free(entry);
valid_character_position = 1 + invalid_character_position;
seek_forward_new(valid_character_position);
component = git_reflog_component_init;
}
}
else {
seek_forward_new(GIT_OID_HEXSZ + 1);
component = git_reflog_component_oid_cur;
}
break;
}
case git_reflog_component_oid_cur: {
if (git_oid_fromstrn(&entry->oid_cur, buf, GIT_OID_HEXSZ) < 0) {
error_occurred = -1;
}
else {
seek_forward_new(GIT_OID_HEXSZ + 1);
component = git_reflog_component_commiter;
}
break;
}
case git_reflog_component_commiter: {
ptr = buf;
/* Seek forward to the end of the signature. */
while (*buf && *buf != '\t' && *buf != '\n')
seek_forward_new(1);
if (git_signature__parse(entry->committer, &ptr, buf + 1, NULL, *buf) < 0) {
error_occurred = -1;
}
else {
component = git_reflog_component_msg;
}
break;
}
case git_reflog_component_msg: {
if (*buf == '\t') {
/* We got a message. Read everything till we reach LF. */
seek_forward_new(1);
ptr = buf;
while (*buf && *buf != '\n') seek_forward_new(1);
entry->msg = git__strndup(ptr, buf - ptr);
GIT_ERROR_CHECK_ALLOC(entry->msg);
} else
entry->msg = NULL;
while (*buf && *buf == '\n' && buf_size > 1) seek_forward_new(1);
component = git_reflog_component_finalize;
break;
}
case git_reflog_component_finalize: {
if (git_vector_insert(&log->entries, entry) < 0) {
error_occurred = -1;
}
else {
component = git_reflog_component_init;
}
break;
}
}
if (error_occurred < 0) {
break;
}
}What do you think about this approach in parsing? |
|
I thought about doing that, too. I've converted the patch parser to use the exact same mechanism via a state machine. For this case here I didn't deem it worthwhile enough, as there is not a lot of variance. I'd propose the following: let's merge the conversion to |
|
@pks-t Yes, nice decision :) By the way, how do you run tests and debug them? In Mac I use only terminal and sublime text. This approach I don't appreciate, cause I need to 'printf'ying every step. Yes, it is old school, but I need to inspect every structure to know what I should output to console. Not too much convenient at first commits in big C project. |
|
On Sun, Oct 20, 2019 at 02:02:17AM -0700, Dmitry Lobanov wrote:
@pks-t Yes, nice decision :)
I can't wait to see this PR is merged :)
Cool, thanks!
By the way, how do you run tests and debug them? In Mac I use
only terminal and sublime text. This approach I don't
appreciate, cause I need to 'printf'ying every step. Yes, it is
old school, but I need to inspect every structure to know what
I should output to console. Not too much convenient at first
commits in big C project.
My toolset includes vim and gdb, only, so it probably is not too
comfortable either. It works just fine for me and forces me to
understand the codebase better, though, due to the lack of code
completion and stuff like that. While possible to implement in
vim, I've noticed that my understanding of the code base grows if
I have to manually look up functions I want to use instead of
just relying on function names.
On some occasions, I also use valgrind to debug memory errors,
but I have to start into a virtual machine to use that. I didn't
yet have any luck to get it to cleanly compile and run on my
musl libc-based system.
|
|
(I'm like, still 😮 and 😨 at your choice of workflow @pks-t, but I don't intend to change my reviews in any way 🤣🙏.) |
And neither should you. With an increasing diversity of people and different toolsets, it is more likely that we catch different types of bugs. And in the end, I'm happy to receive reviews in the first place ;) |
|
@pks-t Is it possible to ask for review and merge this PR as soon as possible? It fixes a blocking issue... |
|
Thanks, @pks-t. That was an enjoyable read. |
|
Thanks @ethomson! |
This pull request has been inspired by #5266 and aims to fix two things:
git_stash_savewith a commit message containing newlines, as we print the newlines verbosely into the stash's reflog.This pull request fixes both issues. It makins our parser more lenient with regards to broken lines in the reflog. Last, it will sanitize reflog messages to not contain any newlines anymore. While we previously rejected such messages in one location, we didn't at another place in libgit2 (refdb_fs). And as git just replaces newlines with spaces, I've modified both places to accept reflog messages with newlines, but replace them with spaces now.