git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[RFC PATCH 0/5] Fix some fast-import parsing issues

From
Elijah Newren <newren@gmail.com>
Date
Feb 20, 2019, 22:58 UTC
Message-ID
<20190220225846.10658-1-newren@gmail.com>

I found a few issues with parsing in fast-import (dating back to git-2.6.0). I was thrown off track for a while by the fact that the .git/fast_import_crash_$PID file erroneously claimed that the penultimate command it read was somehow truncated; e.g. for input of the form:

    commit refs/heads/master
    mark :1
    author Full Name <user@organization.tld> 1000000000 +0100
    committer Full Name <user@organization.tld> 1000000000 +0100
    data 53
    Commit message for intentionally empty orphan commit
    get-mark :1
    
the crash file was reporting
    Most Recent Commands Before Crash
    ---------------------------------
      commit refs/heads/master
      mark :1
      author Full Name <user@organization.tld> 1000000000 +0100
      committer Full Name <user@organization.tld> 1000000000 +0100
      data 53
    * get-mark :1
    
In other words, it dropped the commit message line entirely, which
made me wonder if I was having weird buffering or flushing issues
until I was able to narrow it down to the simple testcase above and
could duplicate with
    cat seven-line-test-file | git fast-import
I don't know if there are other ways to get fast-import to give a
corrupted "Most Recent Commands Before Crash" report, but this one
doesn't trigger after my other fixes below.

Switching gears from the bad reporting to the original parsing bug though, it is clear fast-import somehow mishandled get-mark directives after empty orphan commits. Digging into it, I think it was caused by following a precedent set by a somewhat unsafe implementation of trying to allow cat-blob directives to appear in the middle of a commit. My spelunking showed the following commits seemed to be the most relevant as to how we got to the current state:

    777f80d7429b ("fast-import: Allow cat-blob requests at arbitrary
                 points in stream", 2010-11-28)
    8dc6a373d201 ("fast-import: add 'ls' command", 2010-12-02)
    97313bef2a16 ("fast-import: use skip_prefix for parsing input",
                  2014-06-18)
    28c7b1f7b7b7 ("fast-import: add a get-mark command", 2015-07-01)

I've cc'ed the relevant folks, and have a few patches that fix the issue and I think make the parser more robust against future issues in a way that I think is safe enough for backward compatibility, but "backward compatible enough" might concern some folks; if so, please take a look at patches 4 and 5.

Elijah Newren (5):
  t9300: demonstrate bug with get-mark and empty orphan commits
  git-fast-import.txt: fix wording about where ls command can appear
  fast-import: check most prominent commands first
  fast-import: only allow cat-blob requests where it makes sense
  fast-import: fix erroneous handling of get-mark with empty orphan
    commits
 Documentation/git-fast-import.txt | 22 +++++++++---------
 fast-import.c                     | 31 ++++++++++++++------------
 t/t9300-fast-import.sh            | 37 +++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 24 deletions(-)
-- 
2.21.0.rc2.5.g8f70af2367
Next: Elijah Newren
Message 1 of 15 in “Fix some fast-import parsing issues”
  1. 0/5 Fix some fast-import parsing issuesElijah Newren, Feb 20, 2019
  2. 5/5 fast-import: fix erroneous handling of get-mark with empty orphan commitsElijah Newren, Feb 20, 2019
  3. 2/5 git-fast-import.txt: fix wording about where ls command can appearElijah Newren, Feb 20, 2019
  4. 3/5 fast-import: check most prominent commands firstElijah Newren, Feb 20, 2019
  5. 4/5 fast-import: only allow cat-blob requests where it makes senseElijah Newren, Feb 20, 2019
  6. 1/5 t9300: demonstrate bug with get-mark and empty orphan commitsElijah Newren, Feb 20, 2019
  7. Elijah NewrenMar 8, 2019
  8. Junio C HamanoApr 1, 2019
  9. Elijah NewrenApr 1, 2019
  10. 0/5 Fix some fast-import parsing issuesElijah Newren, Apr 1, 2019
  11. 1/5 t9300: demonstrate bug with get-mark and empty orphan commitsElijah Newren, Apr 1, 2019
  12. 5/5 fast-import: fix erroneous handling of get-mark with empty orphan commitsElijah Newren, Apr 1, 2019
  13. 3/5 fast-import: check most prominent commands firstElijah Newren, Apr 1, 2019
  14. 4/5 fast-import: only allow cat-blob requests where it makes senseElijah Newren, Apr 1, 2019
  15. 2/5 git-fast-import.txt: fix wording about where ls command can appearElijah Newren, Apr 1, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.