1. 28 Nov, 2018 11 commits
    • tests: mailmap: avoid definition of unused static variables · 2521e11c
      The mailmap testdata header contains a set of static variable
      definitions. As these variables aren't used in all places where they are
      used, they trigger the unused-const-variable warnings. As we have
      currently disabled those warnings explicitly, they are never triggered,
      but we intend to enable them.
      
      Avoid the issue by only keeping variable definitions that are actually
      used in all locations. Move the others to where they are used.
      Patrick Steinhardt committed
    • khash: move khash include into implementation files · f2f5ec84
      The current map implementations directly include the "khash.h" headers
      into their own headers to make available a set of static functions,
      defines et cetera. Besides leaking the complete khash namespace into
      files wherever khashes are used, this also triggers Clang's
      -Wunused-function warnings when some of the static functions are not
      being used at all.
      
      Fix the issue by moving the includes into the respective map
      implementation files. Add forward declares for all the map types to make
      them known.
      Patrick Steinhardt committed
    • khash: remove intricate knowledge of khash types · 852bc9f4
      Instead of using the `khiter_t`, `git_strmap_iter` and `khint_t` types,
      simply use `size_t` instead. This decouples code from the khash stuff
      and makes it possible to move the khash includes into the implementation
      files.
      Patrick Steinhardt committed
    • khash: implement map-specific foreach macros · 5bfb3b58
      The current foreach map macros simply redirect to the type-indifferent
      `kh_foreach` macro. As this type-indifferent macro directly accesses the
      structures, the current implementation makes it impossible to make the
      stuctures private to the implementation only. And making them private is
      required to move out the khash include into the implementations to
      decrease the namespace leak.
      Patrick Steinhardt committed
    • khash: implement begin/end via functions instead of macros · 382b668b
      Right now, the `git_*map_begin()` and `git_*map_end()` helpers are
      implemented via macros which simply redirect to `kh_begin` and `kh_end`.
      As these macros refer to members of the map structures, they make it
      impossible to move the khash include into the implementation files.
      
      Implement these helpers as real functions instead to further decouple
      the headers from implementations.
      Patrick Steinhardt committed
    • submodule: remove string map implementation that strips trailing slashes · ae765d00
      The submodule code currently has its own implementation of a string map,
      which overrides the hashing and hash equals functions with functions
      that ignore potential trailing slashes. These functions aren't actually
      used by our code, making them useless.
      Patrick Steinhardt committed
    • idxmap: remove unused foreach macros · 02789782
      The foreach macros of the idxmap types are not used anywhere. As we are
      about to open-code all foreach macros for the maps in order to be able
      to make the khash structure internal, removing these unused macros will
      leave a few places less that need conversion.
      Patrick Steinhardt committed
    • cmake: enable warnings for unused functions · 681c58cf
      Ever since commit 823c0e9c (Fix broken logic for attr cache invalidation,
      2014-04-17), we have completely disabled warnings for unused functions. The only
      comment that was added back then is about "annoying extra warnings" from Clang,
      but in fact we shouldn't just ignore warnings about functions which aren't used
      at all. Instead, the right thing would be to either only conditionally compile
      functions that aren't used in all configurations or, alternatively, to remove
      functions that aren't required at all.
      
      As remaining instances of unused functions have been removed in the last two
      commits, re-enable the warning.
      Patrick Steinhardt committed
    • iterator: remove unused function `tree_iterator_entry_cmp` · b2af13f2
      The function `tree_iterator_entry_cmp` has been introduced in commit be30387e
      (iterators: refactored tree iterator, 2016-02-25), but in fact it has never been
      used at all. Remove it to avoid unused function warnings as soon as we re-enable
      "-Wunused-functions".
      Patrick Steinhardt committed
    • tests: path: only compile test_canonicalize on Win32 platforms · bbf9f5a7
      The function `test_canonicalize` is only used on Win32 platforms. It will thus
      result in an unused function warning if these warnings are enabled and one is on
      a platform different than Win32.
      
      Fix the issue by only compiling in the function on Win32 platforms.
      Patrick Steinhardt committed
    • tests: move apply_helpers functions into own compilation unit · 14a9a4f3
      Currently, the "apply_helper" functions used for testing the apply logic are all
      statically defined in the "apply_helpers.h" header file. This may lead to
      warnings from the compiler in case where this header file is included, but not
      all functions it brings along are used in the compilation unit where it has been
      included into.
      
      Fix these potential warnings by moving the implementation into its own
      compilation unit "apply_helpers.c".
      Patrick Steinhardt committed
  2. 25 Nov, 2018 2 commits
  3. 21 Nov, 2018 3 commits
    • Merge pull request #4884 from libgit2/ethomson/index_iterator · 0e3e832d
      index: introduce git_index_iterator
      Patrick Steinhardt committed
    • Merge pull request #4894 from pks-t/pks/commit-author-oob · 94fce582
      commit: fix out-of-bound reads when parsing truncated author fields
      Edward Thomson committed
    • commit: fix out-of-bound reads when parsing truncated author fields · cb23c3ef
      While commit objects usually should have only one author field, our commit
      parser actually handles the case where a commit has multiple author fields
      because some tools that exist in the wild actually write them. Detection of
      those additional author fields is done by using a simple `git__prefixcmp`,
      checking whether the current line starts with the string "author ". In case
      where we are handed a non-NUL-terminated string that ends directly after the
      space, though, we may have an out-of-bounds read of one byte when trying to
      compare the expected final NUL byte.
      
      Fix the issue by using `git__prefixncmp` instead of `git_prefixcmp`.
      Unfortunately, a test cannot be easily written to catch this case. While we
      could test the last error message and verify that it didn't in fact fail parsing
      a signature (because that would indicate that it has in fact tried to parse the
      additional "author " field, which it shouldn't be able to detect in the first
      place), this doesn't work as the next line needs to be the "committer" field,
      which would error out with the same error message even if we hadn't done an
      out-of-bounds read.
      
      As objects read from the object database are always NUL terminated, this issue
      cannot be triggered in normal code and thus it's not security critical.
      Patrick Steinhardt committed
  4. 18 Nov, 2018 7 commits
  5. 15 Nov, 2018 1 commit
  6. 14 Nov, 2018 4 commits
    • index: introduce git_index_iterator · c358bbc5
      Provide a public git_index_iterator API that is backed by an index
      snapshot.  This allows consumers to provide a stable iteration even
      while manipulating the index during iteration.
      Edward Thomson committed
    • Merge pull request #4886 from pks-t/pks/strntol-truncate-leading-sign · 9189a66a
      strntol: fix out-of-bounds reads when parsing numbers with leading sign
      Edward Thomson committed
    • patch_parse: remove unused function `parse_number` · 4b84db6a
      The function `parse_number` was replaced by `git_parse_advance_digit`
      which is provided by the parser interface in commit 252f2eee (parse:
      implement and use `git_parse_advance_digit`, 2017-07-14). As there are
      no remaining callers, remove it.
      Patrick Steinhardt committed
    • strntol: fix out-of-bounds reads when parsing numbers with leading sign · 4209a512
      When parsing a number, we accept a leading plus or minus sign to return
      a positive or negative number. When the parsed string has such a leading
      sign, we set up a flag indicating that the number is negative and
      advance the pointer to the next character in that string. This misses
      updating the number of bytes in the string, though, which is why the
      parser may later on do an out-of-bounds read.
      
      Fix the issue by correctly updating both the pointer and the number of
      remaining bytes. Furthermore, we need to check whether we actually have
      any bytes left after having advanced the pointer, as otherwise the
      auto-detection of the base may do an out-of-bonuds access. Add a test
      that detects the out-of-bound read.
      
      Note that this is not actually security critical. While there are a lot
      of places where the function is called, all of these places are guarded
      or irrelevant:
      
      - commit list: this operates on objects from the ODB, which are always
        NUL terminated any may thus not trigger the off-by-one OOB read.
      
      - config: the configuration is NUL terminated.
      
      - curl stream: user input is being parsed that is always NUL terminated
      
      - index: the index is read via `git_futils_readbuffer`, which always NUL
        terminates it.
      
      - loose objects: used to parse the length from the object's header. As
        we check previously that the buffer contains a NUL byte, this is safe.
      
      - rebase: this parses numbers from the rebase instruction sheet. As the
        rebase code uses `git_futils_readbuffer`, the buffer is always NUL
        terminated.
      
      - revparse: this parses a user provided buffer that is NUL terminated.
      
      - signature: this parser the header information of objects. As objects
        read from the ODB are always NUL terminated, this is a non-issue. The
        constructor `git_signature_from_buffer` does not accept a length
        parameter for the buffer, so the buffer needs to be NUL terminated, as
        well.
      
      - smart transport: the buffer that is parsed is NUL terminated
      
      - tree cache: this parses the tree cache from the index extension. The
        index itself is read via `git_futils_readbuffer`, which always NUL
        terminates it.
      
      - winhttp transport: user input is being parsed that is always NUL
        terminated
      Patrick Steinhardt committed
  7. 13 Nov, 2018 7 commits
  8. 11 Nov, 2018 1 commit
  9. 10 Nov, 2018 1 commit
  10. 09 Nov, 2018 3 commits
    • transport/http: Include non-default ports in Host header · 83b35181
      When the port is omitted, the server assumes the default port for the
      service is used (see
      https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host). In
      cases where the client provided a non-default port, it should be passed
      along.
      
      This hasn't been an issue so far as the git protocol doesn't include
      server-generated URIs. I encountered this when implementing Rust
      registry support for Sonatype Nexus. Rust's registry uses a git
      repository for the package index. Clients look at a file in the root of
      the package index to find the base URL for downloading the packages.
      Sonatype Nexus looks at the incoming HTTP request (Host header and URL)
      to determine the client-facing URL base as it may be running behind a
      load balancer or reverse proxy. This client-facing URL base is then used
      to construct the package download base URL. When libgit2 fetches the
      index from Nexus on a non-default port, Nexus trusts the incorrect Host
      header and generates an incorrect package download base URL.
      Rick Altherr committed
    • netops: add method to return default http port for a connection · 58b60fcc
      Constant strings and logic for HTTP(S) default ports were starting to be
      spread throughout netops.c.  Instead of duplicating this again to
      determine if a Host header should include the port, move the default
      port constants and logic into an internal method in netops.{c,h}.
      Rick Altherr committed
    • signature: fix out-of-bounds read when parsing timezone offset · 52f859fd
      When parsing a signature's timezone offset, we first check whether there
      is a timezone at all by verifying that there are still bytes left to
      read following the time itself. The check thus looks like `time_end + 1
      < buffer_end`, which is actually correct in this case. After setting the
      timezone's start pointer to that location, we compute the remaining
      bytes by using the formula `buffer_end - tz_start + 1`, re-using the
      previous `time_end + 1`. But this is in fact missing the braces around
      `(tz_start + 1)`, thus leading to an overestimation of the remaining
      bytes by a length of two. In case of a non-NUL terminated buffer, this
      will result in an overflow.
      
      The function `git_signature__parse` is only used in two locations. First
      is `git_signature_from_buffer`, which only accepts a string without a
      length. The string thus necessarily has to be NUL terminated and cannot
      trigger the issue.
      
      The other function is `git_commit__parse_raw`, which can in fact trigger
      the error as it may receive non-NUL terminated commit data. But as
      objects read from the ODB are always NUL-terminated by us as a
      cautionary measure, it cannot trigger the issue either.
      
      In other words, this error does not have any impact on security.
      Patrick Steinhardt committed