1. 10 Mar, 2018 29 commits
    • openssl: fix thread-safety on non-glibc POSIX systems · 049e1de5
      While the OpenSSL library provides all means to work safely in a
      multi-threaded application, we fail to do so correctly. Quoting from
      crypto_lock(3):
      
          OpenSSL can safely be used in multi-threaded applications provided
          that at least two callback functions are set, locking_function and
          threadid_func.
      
      We do in fact provide the means to set up the locking function via
      `git_openssl_set_locking()`, where we initialize a set of locks by using
      the POSIX threads API and set the correct callback function to lock and
      unlock them.
      
      But what we do not do is setting the `threadid_func` callback. This
      function is being used to correctly locate thread-local data of the
      OpenSSL library and should thus return per-thread identifiers. Digging
      deeper into OpenSSL's documentation, the library does provide a fallback
      in case that locking function is not provided by the user. On Windows
      and BeOS we should be safe, as it simply "uses the system's default
      thread identifying API". On other platforms though OpenSSL will fall
      back to using the address of `errno`, assuming it is thread-local.
      
      While this assumption holds true for glibc-based systems, POSIX in fact
      does not specify whether it is thread-local or not. Quoting from
      errno(3p):
      
          It is unspecified whether errno is a macro or an identifier declared
          with external linkage.
      
      And in fact, with musl there is at least one libc implementation which
      simply declares `errno` as a simple `int` without being thread-local. On
      those systems, the fallback threadid function of OpenSSL will not be
      thread-safe.
      
      Fix this by setting up our own callback for this setting. As users of
      libgit2 may want to set it themselves, we obviously cannot always set
      that function on initialization. But as we already set up primitives for
      threading in `git_openssl_set_locking()`, this function becomes the
      obvious choice where to implement the additional setup.
      Patrick Steinhardt committed
    • diff_generate: fix unsetting diff flags · 3c4e0cee
      The macro `DIFF_FLAG_SET` can be used to set or unset a flag by
      modifying the diff's bitmask. While the case of setting the flag is
      handled correctly, the case of unsetting the flag was not. Instead of
      inverting the flags, we are inverting the value which is used to decide
      whether we want to set or unset the bits.
      
      The value being used here is a simple `bool` which is `false`. As that
      is being uplifted to `int` when getting the bitwise-complement, we will
      end up retaining all bits inside of the bitmask. As that's only ever
      used to set `GIT_DIFF_IGNORE_CASE`, we were actually always ignoring
      case for generated diffs.
      
      Fix that by instead getting the bitwise-complement of `FLAG`, not `VAL`.
      Patrick Steinhardt committed
    • diff: remove unused macros `DIFF_FLAG_*` · 05a753d4
      In commit 9be638ec (git_diff_generated: abstract generated diffs,
      2016-04-19), the code for generated diffs was moved out of the generic
      "diff.c" and instead into its own module. During that conversion, it was
      forgotten to remove the macros `DIFF_FLAG_IS_SET`, `DIFF_FLAG_ISNT_SET`
      and `DIFF_FLAG_SET`, which are now only used in "diff_generated.c".
      
      Remove those macros now.
      Patrick Steinhardt committed
    • Include git2/worktree.h in git2.h · 5c3a42ad
      I'm not sure if worktree.h was intentionally left out of git2.h. Looks like an oversight since it is in fact documented.
      apnadkarni committed
    • checkout: test force checkout when mode changes · e66bc08c
      Test that we can successfully force checkout a target when the file
      contents are identical, but the mode has changed.
      Edward Thomson committed
    • checkout: do not test file mode on Windows · 8631357e
      On Windows, we do not support file mode changes, so do not test
      for type changes between the disk and tree being checked out.
      
      We could have false positives since the on-disk file can only have
      an (effective) mode of 0100644 since NTFS does not support executable
      files.  If the tree being checked out did have an executable file,
      we would erroneously decide that the file on disk had been changed.
      Edward Thomson committed
    • checkout: treat files as modified if mode differs · 24388179
      When performing a forced checkout, treat files as modified when the
      workdir or the index is identical except for the mode.  This ensures
      that force checkout will update the mode to the target.  (Apply this
      check for regular files only, if one of the items was a file and the
      other was another type of item then this would be a typechange and
      handled independently.)
      Edward Thomson committed
    • transports: smart: fix memory leak when skipping symbolic refs · f41e86d6
      When we setup the revision walk for negotiating references with a
      remote, we iterate over all references, ignoring tags and symbolic
      references. While skipping over symbolic references, we forget to free
      the looked up reference, resulting in a memory leak when the next
      iteration simply overwrites the variable.
      
      Fix that issue by freeing the reference at the beginning of each
      iteration and collapsing return paths for error and success.
      Patrick Steinhardt committed
    • refs: do not use peeled OID if peeling to a tag · cda18f9b
      If a reference stored in a packed-refs file does not directly point to a
      commit, tree or blob, the packed-refs file will also will include a
      fully-peeled OID pointing to the first underlying object of that type.
      If we try to peel a reference to an object, we will use that peeled OID
      to speed up resolving the object.
      
      As a reference for an annotated tag does not directly point to a commit,
      tree or blob but instead to the tag object, the packed-refs file will
      have an accomodating fully-peeled OID pointing to the object referenced
      by that tag. When we use the fully-peeled OID pointing to the referenced
      object when peeling, we obviously cannot peel that to the tag anymore.
      
      Fix this issue by not using the fully-peeled OID whenever we want to
      peel to a tag. Note that this does not include the case where we want to
      resolve to _any_ object type. Existing code may make use from the fact
      that we resolve those to commit objects instead of tag objects, even
      though that behaviour is inconsistent between packed and loose
      references. Furthermore, some tests of ours make the assumption that we
      in fact resolve those references to a commit.
      Patrick Steinhardt committed
    • proxy: add a free function for the options's pointers · e29ab6fe
      When we duplicate a user-provided options struct, we're stuck with freeing the
      url in it. In case we add stuff to the proxy struct, let's add a function in
      which to put the logic.
      Carlos Martín Nieto committed
    • Clear the remote_ref_name buffer in git_push_update_tips() · c3fbf905
      If fetch_spec was a non-pattern, and it is not the first iteration of push_status vector, then git_refspec_transform would result in the new value appended via git_buf_puts to the previous iteration value.
      
      Forcibly clearing the buffer on each iteration to prevent this behavior.
      Slava Karpenko committed
    • sha1_position: convert do-while to while · 3ca2bb39
      If we enter the sha1_position() function with "lo == hi",
      we have no elements. But the do-while loop means that we'll
      enter the loop body once anyway, picking "mi" at that same
      value and comparing nonsense to our desired key. This is
      unlikely to match in practice, but we still shouldn't be
      looking at the memory in the first place.
      
      This bug is inherited from git.git; it was fixed there in
      e01580cfe01526ec2c4eb4899f776a82ade7e0e1.
      Jeff King committed
    • signature: don't leave a dangling pointer to the strings on parse failure · 21f77af9
      If the signature is invalid but we detect that after allocating the strings, we
      free them. We however leave that pointer dangling in the structure the caller
      gave us, which can lead to double-free.
      
      Set these pointers to `NULL` after freeing their memory to avoid this.
      Carlos Martín Nieto committed
    • ignore: honor case insensitivity for negative ignores · 4296a36b
      When computing negative ignores, we throw away any rule which does not
      undo a previous rule to optimize. But on case insensitive file systems,
      we need to keep in mind that a negative ignore can also undo a previous
      rule with different case, which we did not yet honor while determining
      whether a rule undoes a previous one. So in the following example, we
      fail to unignore the "/Case" directory:
      
          /case
          !/Case
      
      Make both paths checking whether a plain- or wildcard-based rule undo a
      previous rule aware of case-insensitivity. This fixes the described
      issue.
      Patrick Steinhardt committed
    • tests: status: additional test for negative ignores with pattern · 32cc5edc
      This test is by Carlos Martín Nieto.
      Patrick Steinhardt committed
    • ignore: keep negative rules containing wildcards · 5c15cd94
      Ignore rules allow for reverting a previously ignored rule by prefixing
      it with an exclamation mark. As such, a negative rule can only override
      previously ignored files. While computing all ignore patterns, we try to
      use this fact to optimize away some negative rules which do not override
      any previous patterns, as they won't change the outcome anyway.
      
      In some cases, though, this optimization causes us to get the actual
      ignores wrong for some files. This may happen whenever the pattern
      contains a wildcard, as we are unable to reason about whether a pattern
      overrides a previous pattern in a sane way. This happens for example in
      the case where a gitignore file contains "*.c" and "!src/*.c", where we
      wouldn't un-ignore files inside of the "src/" subdirectory.
      
      In this case, the first solution coming to mind may be to just strip the
      "src/" prefix and simply compare the basenames. While that would work
      here, it would stop working as soon as the basename pattern itself is
      different, like for example with "*x.c" and "!src/*.c. As such, we
      settle for the easier fix of just not optimizing away rules that contain
      a wildcard.
      Patrick Steinhardt committed
    • Convert port with htons() in p_getaddrinfo() · f908bb8e
      `sin_port` should be in network byte order.
      Ian Douglas Scott committed
    • Remove invalid submodule · 54d4e5de
      Fixes #4274
      Etienne Samson committed
  2. 08 Mar, 2018 6 commits
    • Merge pull request #4572 from pks-t/pks/index-secfixes · dd2d5381
      Security fixes for reading index v4
      Patrick Steinhardt committed
    • index: error out on unreasonable prefix-compressed path lengths · 6f4d04b5
      When computing the complete path length from the encoded
      prefix-compressed path, we end up just allocating the complete path
      without ever checking what the encoded path length actually is. This can
      easily lead to a denial of service by just encoding an unreasonable long
      path name inside of the index. Git already enforces a maximum path
      length of 4096 bytes. As we also have that enforcement ready in some
      places, just make sure that the resulting path is smaller than
      GIT_PATH_MAX.
      
      Reported-by: Krishna Ram Prakash R <krp@gtux.in>
      Reported-by: Vivek Parikh <viv0411.parikh@gmail.com>
      Patrick Steinhardt committed
    • index: fix out-of-bounds read with invalid index entry prefix length · 6ddd286e
      The index format in version 4 has prefix-compressed entries, where every
      index entry can compress its path by using a path prefix of the previous
      entry. Since implmenting support for this index format version in commit
      5625d86b (index: support index v4, 2016-05-17), though, we do not
      correctly verify that the prefix length that we want to reuse is
      actually smaller or equal to the amount of characters than the length of
      the previous index entry's path. This can lead to a an integer underflow
      and subsequently to an out-of-bounds read.
      
      Fix this by verifying that the prefix is actually smaller than the
      previous entry's path length.
      
      Reported-by: Krishna Ram Prakash R <krp@gtux.in>
      Reported-by: Vivek Parikh <viv0411.parikh@gmail.com>
      Patrick Steinhardt committed
    • index: convert `read_entry` to return entry size via an out-param · b6756821
      The function `read_entry` does not conform to our usual coding style of
      returning stuff via the out parameter and to use the return value for
      reporting errors. Due to most of our code conforming to that pattern, it
      has become quite natural for us to actually return `-1` in case there is
      any error, which has also slipped in with commit 5625d86b (index:
      support index v4, 2016-05-17). As the function returns an `size_t` only,
      though, the return value is wrapped around, causing the caller of
      `read_tree` to continue with an invalid index entry. Ultimately, this
      can lead to a double-free.
      
      Improve code and fix the bug by converting the function to return the
      index entry size via an out parameter and only using the return value to
      indicate errors.
      
      Reported-by: Krishna Ram Prakash R <krp@gtux.in>
      Reported-by: Vivek Parikh <viv0411.parikh@gmail.com>
      Patrick Steinhardt committed
  3. 07 Mar, 2018 5 commits