Skip to content

reflogs: fix behaviour around reflogs with newlines - #5275

Merged
ethomson merged 5 commits into
libgit2:masterfrom
pks-t:pks/reflogs-with-newlines
Nov 2, 2019
Merged

reflogs: fix behaviour around reflogs with newlines#5275
ethomson merged 5 commits into
libgit2:masterfrom
pks-t:pks/reflogs-with-newlines

Conversation

@pks-t

@pks-t pks-t commented Oct 18, 2019

Copy link
Copy Markdown
Member

This pull request has been inspired by #5266 and aims to fix two things:

  1. We fail to parse corrupted reflogs, which git is reading just fine.
  2. We corrupt the reflog if somebody calls git_stash_save with 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.

pks-t added 4 commits October 18, 2019 09:16
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.
@pks-t
pks-t force-pushed the pks/reflogs-with-newlines branch from 5e9b256 to 7968e90 Compare October 18, 2019 11:30
@lolgear

lolgear commented Oct 18, 2019

Copy link
Copy Markdown
Contributor

@pks-t Looks nice!
I would like to discuss another approach on which I am working.

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?

@pks-t

pks-t commented Oct 19, 2019

Copy link
Copy Markdown
Member Author

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 git_parse_ctx, which was my longterm goal when I first introduced the parsing interface, to provide a common parsing interface for all of our code and thus reduce duplication. I'd still love to see the conversion to use a state machine with the then-converted code after the merge, just to see whether it makes sense for such a linear parser. Are you fine with that?

@lolgear

lolgear commented Oct 20, 2019

Copy link
Copy Markdown
Contributor

@pks-t Yes, nice decision :)
I can't wait to see this PR is merged :)

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.

@pks-t

pks-t commented Oct 21, 2019 via email

Copy link
Copy Markdown
Member Author

@tiennou

tiennou commented Oct 22, 2019

Copy link
Copy Markdown
Contributor

@lolgear I've filed #5278, which is all my "private" developer stuff. There's a build.sh script that should setup an appropriate .xcodeproj, though you may have to fiddle with your brewed paths. Thanks for asking 😉, HTH !

@tiennou

tiennou commented Oct 22, 2019

Copy link
Copy Markdown
Contributor

(I'm like, still 😮 and 😨 at your choice of workflow @pks-t, but I don't intend to change my reviews in any way 🤣🙏.)

@pks-t

pks-t commented Oct 24, 2019

Copy link
Copy Markdown
Member Author

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 ;)

@lolgear

lolgear commented Nov 2, 2019

Copy link
Copy Markdown
Contributor

@pks-t Is it possible to ask for review and merge this PR as soon as possible? It fixes a blocking issue...

@lolgear lolgear mentioned this pull request Nov 2, 2019
2 tasks
@ethomson

ethomson commented Nov 2, 2019

Copy link
Copy Markdown
Member

Thanks, @pks-t. That was an enjoyable read.

@ethomson
ethomson merged commit bf2911d into libgit2:master Nov 2, 2019
@pks-t
pks-t deleted the pks/reflogs-with-newlines branch November 5, 2019 10:13
@pks-t

pks-t commented Nov 5, 2019

Copy link
Copy Markdown
Member Author

Thanks @ethomson!

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.

4 participants