1. 26 Mar, 2020 19 commits
    • diff_generate: detect memory allocation errors when preparing opts · ad735bf3
      When preparing options for the two iterators that are about to be
      diffed, we allocate a common prefix for both iterators depending on
      the options passed by the user. We do not check whether the allocation
      was successful, though. In fact, this isn't much of a problem, as using
      a `NULL` prefix is perfectly fine. But in the end, we probably want to
      detect that the system doesn't have any memory left, as we're unlikely
      to be able to continue afterwards anyway.
      
      While the issue is being fixed in the newly created function
      `diff_prepare_iterator_opts`, it has been previously existing in the
      previous macro `DIFF_FROM_ITERATORS` already.
      Patrick Steinhardt committed
    • diff_generate: refactor `DIFF_FROM_ITERATORS` macro of doom · 7aa03e92
      While the `DIFF_FROM_ITERATORS` does make it shorter to implement the
      various `git_diff_foo_to_bar` functions, it is a complex and unreadable
      beast that implicitly assumes certain local variable names. This is not
      something desirable to have at all and obstructs understanding and more
      importantly debugging the code by quite a bit.
      
      The `DIFF_FROM_ITERATORS` macro basically removed the burden of having
      to derive the options for both iterators from a pair of iterator flags
      and the diff options. This patch introduces a new function that does the
      that exact and refactors all callers to manage the iterators by
      themselves.
      
      As we potentially need to allocate a shared prefix for the
      iterator, we need to tell the caller to allocate that prefix as soon as
      the options aren't required anymore. Thus, the function has a `char
      **prefix` out pointer that will get set to the allocated string and
      subsequently be free'd by the caller.
      
      While this patch increases the line count, I personally deem this to an
      acceptable tradeoff for increased readbiblity.
      Patrick Steinhardt committed
    • ignore: correct handling of nested rules overriding wild card unignore · 99b89a9c
      problem:
      filesystem_iterator loads .gitignore files in top-down order.
      subsequently, ignore module evaluates them in the order they are loaded.
      this creates a problem if we have unignored a rule (using a wild card)
      in a sub dir and ignored it again in a level further below (see the test
      included in this patch).
      
      solution:
      process ignores in reverse order.
      
      closes #4963
      buddyspike committed
    • apply: Test for EOFNL mishandling when several hunks are processed · 5e5a9cce
      Introduce an unit test to validate that git_apply__patch() properly
      handles EOFNL changes in case of patches with several hunks.
      Max Kostyukevich committed
    • apply: Fix a patch corruption related to EOFNL handling · 0126e3fc
      Use of apply's API can lead to an improper patch application and a corruption
      of the modified file.
      
      The issue is caused by mishandling of the end of file changes if there are
      several hunks to apply. The new line character is added to a line from a wrong
      hunk.
      
      The solution is to modify apply_hunk() to add the newline character at the end
      of a line from a right hunk.
      Max Kostyukevich committed
    • apply: free test data · ae9b333a
      Edward Thomson committed
    • apply: Test for git_apply_to_tree failures when new files are added · deda897a
      Introduce an unit test to validate if git_apply_to_tree() fails when an
      applied patch adds new files.
      Max Kostyukevich committed
    • apply: git_apply_to_tree fails to apply patches that add new files · d6e5c44f
      git_apply_to_tree() cannot be used apply patches with new files. An attempt
      to apply such a patch fails because git_apply_to_tree() tries to remove a
      non-existing file from an old index.
      
      The solution is to modify git_apply_to_tree() to git_index_remove() when the
      patch states that the modified files is removed.
      Max Kostyukevich committed
    • config: check if we are running in a sandboxed environment · 30cd1e1f
      On macOS the $HOME environment variable returns the path to the sandbox container instead of the actual user $HOME for sandboxed apps. To get the correct path, we have to get it from the password file entry.
      Erik Aigner committed
    • patch_parse: fix segfault due to line containing static contents · c159cceb
      With commit dedf70ad (patch_parse: do not depend on parsed buffer's
      lifetime, 2019-07-05), all lines of the patch are allocated with
      `strdup` to make lifetime of the parsed patch independent of the buffer
      that is currently being parsed. In patch b0893282 (patch_parse: ensure
      valid patch output with EOFNL, 2019-07-11), we introduced another
      code location where we add lines to the parsed patch. But as that one
      was implemented via a separate pull request, it wasn't converted to use
      `strdup`, as well. As a consequence, we generate a segfault when trying
      to deallocate the potentially static buffer that's now in some of the
      lines.
      
      Use `git__strdup` to fix the issue.
      Patrick Steinhardt committed
    • patch_parse: handle missing newline indicator in old file · fe012c60
      When either the old or new file contents have no newline at the end of
      the file, then git-diff(1) will print out a "\ No newline at end of
      file" indicator. While we do correctly handle this in the case where the
      new file has this indcator, we fail to parse patches where the old file
      is missing a newline at EOF.
      
      Fix this bug by handling and missing newline indicators in the old file.
      Add tests to verify that we can parse such files.
      Patrick Steinhardt committed
    • diff: ignore EOFNL for computing patch IDs · ef1651e6
      The patch ID is supposed to be mostly context-insignificant and
      thus only includes added or deleted lines. As such, we shouldn't honor
      end-of-file-without-newline markers in diffs.
      
      Ignore such lines to fix how we compute the patch ID for such diffs.
      Patrick Steinhardt committed
    • patch_parse: do not depend on parsed buffer's lifetime · 782bc334
      When parsing a patch from a buffer, we let the patch lines point into
      the original buffer. While this is efficient use of resources, this also
      ties the lifetime of the parsed patch to the parsed buffer. As this
      behaviour is not documented anywhere in our API it is very surprising to
      its users.
      
      Untie the lifetime by duplicating the lines into the parsed patch. Add a
      test that verifies that lifetimes are indeed independent of each other.
      Patrick Steinhardt committed
    • ci: add flaky test re-execution on Windows · 7786d7e9
      Our online tests are occasionally flaky since they hit real network
      endpoints.  Re-run them up to 5 times if they fail, to allow us to
      avoid having to fail the whole build.
      Edward Thomson committed
    • ci: add flaky test re-execution on Unix · f8a09985
      Our online tests are occasionally flaky since they hit real network
      endpoints.  Re-run them up to 5 times if they fail, to allow us to
      avoid having to fail the whole build.
      Edward Thomson committed
    • tests: apply: verify that we correctly truncate the source buffer · 2ce6eddf
      Previously, we would fail to correctly truncate the source buffer
      if the source has more than one line and ends with a non-newline
      character. In the following call, we thus truncate the source
      string in the middle of the second line. Without the bug fixed,
      we would successfully apply the patch to the source and return
      success. With the overflow being fixed, we should return an
      error now.
      Patrick Steinhardt committed
    • apply: prevent OOB read when parsing source buffer · 6f351d83
      When parsing the patch image from a string, we split the string
      by newlines to get a line-based view of it. To split, we use
      `memchr` on the buffer and limit the buffer length by the
      original length provided by the caller. This works just fine for
      the first line, but for every subsequent line we need to actually
      subtract the amount of bytes that we have already read.
      
      The above issue can be easily triggered by having a source buffer
      with at least two lines, where the second line does _not_ end in
      a newline. Given a string "foo\nb", we have an original length of
      five bytes. After having extracted the first line, we will point
      to 'b' and again try to `memchr(p, '\n', 5)`, resulting in an
      out-of-bounds read of four bytes.
      
      Fix the issue by correctly subtracting the amount of bytes
      already read.
      Erik Aigner committed
  2. 10 Dec, 2019 17 commits
  3. 04 Aug, 2019 4 commits
    • Release v0.28.3 · 7ce88e66
      Edward Thomson committed
    • commit_list: fix possible buffer overflow in `commit_quick_parse` · 3316f666
      The function `commit_quick_parse` provides a way to quickly parse
      parts of a commit without storing or verifying most of its
      metadata. The first thing it does is calculating the number of
      parents by skipping "parent " lines until it finds the first
      non-parent line. Afterwards, this parent count is passed to
      `alloc_parents`, which will allocate an array to store all the
      parent.
      
      To calculate the amount of storage required for the parents
      array, `alloc_parents` simply multiplicates the number of parents
      with the respective elements's size. This already screams "buffer
      overflow", and in fact this problem is getting worse by the
      result being cast to an `uint32_t`.
      
      In fact, triggering this is possible: git-hash-object(1) will
      happily write a commit with multiple millions of parents for you.
      I've stopped at 67,108,864 parents as git-hash-object(1)
      unfortunately soaks up the complete object without streaming
      anything to disk and thus will cause an OOM situation at a later
      point. The point here is: this commit was about 4.1GB of size but
      compressed down to 24MB and thus easy to distribute.
      
      The above doesn't yet trigger the buffer overflow, thus. As the
      array's elements are all pointers which are 8 bytes on 64 bit, we
      need a total of 536,870,912 parents to trigger the overflow to
      `0`. The effect is that we're now underallocating the array
      and do an out-of-bound writes. As the buffer is kindly provided
      by the adversary, this may easily result in code execution.
      
      Extrapolating from the test file with 67m commits to the one with
      536m commits results in a factor of 8. Thus the uncompressed
      contents would be about 32GB in size and the compressed ones
      192MB. While still easily distributable via the network, only
      servers will have that amount of RAM and not cause an
      out-of-memory condition previous to triggering the overflow. This
      at least makes this attack not an easy vector for client-side use
      of libgit2.
      Patrick Steinhardt committed
    • config: validate ownership of C:\ProgramData\Git\config before using it · d475d5d6
      When the VirtualStore feature is in effect, it is safe to let random
      users write into C:\ProgramData because other users won't see those
      files. This seemed to be the case when we introduced support for
      C:\ProgramData\Git\config.
      
      However, when that feature is not in effect (which seems to be the case
      in newer Windows 10 versions), we'd rather not use those files unless
      they come from a trusted source, such as an administrator.
      
      This change imitates the strategy chosen by PowerShell's native OpenSSH
      port to Windows regarding host key files: if a system file is owned
      neither by an administrator, a system account, or the current user, it
      is ignored.
      Johannes Schindelin committed