1. 26 9月, 2015 29 次提交
    • J
      use strip_suffix and xstrfmt to replace suffix · 9ae97018
      Jeff King 提交于
      When we want to convert "foo.pack" to "foo.idx", we do it by
      duplicating the original string and then munging the bytes
      in place. Let's use strip_suffix and xstrfmt instead, which
      has several advantages:
      
        1. It's more clear what the intent is.
      
        2. It does not implicitly rely on the fact that
           strlen(".idx") <= strlen(".pack") to avoid an overflow.
      
        3. We communicate the assumption that the input file ends
           with ".pack" (and get a run-time check that this is so).
      
        4. We drop calls to strcpy, which makes auditing the code
           base easier.
      
      Likewise, we can do this to convert ".pack" to ".bitmap",
      avoiding some manual memory computation.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      9ae97018
    • J
      fetch: replace static buffer with xstrfmt · 2805bb59
      Jeff King 提交于
      We parse the INFINITE_DEPTH constant into a static,
      fixed-size buffer using sprintf. This buffer is sufficiently
      large for the current constant, but it's a suspicious
      pattern, as the constant is defined far away, and it's not
      immediately obvious that 12 bytes are large enough to hold
      it.
      
      We can just use xstrfmt here, which gets rid of any question
      of the buffer size. It also removes any concerns with object
      lifetime, which means we do not have to wonder why this
      buffer deep within a conditional is marked "static" (we
      never free our newly allocated result, of course, but that's
      OK; it's global that lasts the lifetime of the whole program
      anyway).
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      2805bb59
    • J
      config: use xstrfmt in normalize_value · 3ec832c4
      Jeff King 提交于
      We xmalloc a fixed-size buffer and sprintf into it; this is
      OK because the size of our formatting types is finite, but
      that's not immediately clear to a reader auditing sprintf
      calls. Let's switch to xstrfmt, which is shorter and
      obviously correct.
      
      Note that just dropping the common xmalloc here causes gcc
      to complain with -Wmaybe-uninitialized. That's because if
      "types" does not match any of our known types, we never
      write anything into the "normalized" pointer. With the
      current code, gcc doesn't notice because we always return a
      valid pointer (just one which might point to uninitialized
      data, but the compiler doesn't know that). In other words,
      the current code is potentially buggy if new types are added
      without updating this spot.
      
      So let's take this opportunity to clean up the function a
      bit more. We can drop the "normalized" pointer entirely, and
      just return directly from each code path. And then add an
      assertion at the end in case we haven't covered any cases.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      3ec832c4
    • J
      replace trivial malloc + sprintf / strcpy calls with xstrfmt · 75faa45a
      Jeff King 提交于
      It's a common pattern to do:
      
        foo = xmalloc(strlen(one) + strlen(two) + 1 + 1);
        sprintf(foo, "%s %s", one, two);
      
      (or possibly some variant with strcpy()s or a more
      complicated length computation).  We can switch these to use
      xstrfmt, which is shorter, involves less error-prone manual
      computation, and removes many sprintf and strcpy calls which
      make it harder to audit the code for real buffer overflows.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      75faa45a
    • J
      receive-pack: convert strncpy to xsnprintf · b7115a35
      Jeff King 提交于
      This strncpy is pointless; we pass the strlen() of the src
      string, meaning that it works just like a memcpy. Worse,
      though, is that the size has no relation to the destination
      buffer, meaning it is a potential overflow.  In practice,
      it's not. We pass only short constant strings like
      "warning: " and "error: ", which are much smaller than the
      destination buffer.
      
      We can make this much simpler by just using xsnprintf, which
      will check for overflow and return the size for our next
      vsnprintf, without us having to run a separate strlen().
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      b7115a35
    • J
      http-push: replace strcat with xsnprintf · 0cc41428
      Jeff King 提交于
      We account for these strcats in our initial allocation, but
      the code is confusing to follow and verify. Let's remember
      our original allocation length, and then xsnprintf can
      verify that we don't exceed it.
      
      Note that we can't just use xstrfmt here (which would be
      even cleaner) because the code tries to grow the buffer only
      when necessary.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      0cc41428
    • J
      add_packed_git: convert strcpy into xsnprintf · 48bcc1c3
      Jeff King 提交于
      We have the path "foo.idx", and we create a buffer big
      enough to hold "foo.pack" and "foo.keep", and then strcpy
      straight into it. This isn't a bug (we have enough space),
      but it's very hard to tell from the strcpy that this is so.
      
      Let's instead use strip_suffix to take off the ".idx",
      record the size of our allocation, and use xsnprintf to make
      sure we don't violate our assumptions.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      48bcc1c3
    • J
      entry.c: convert strcpy to xsnprintf · 330c8e26
      Jeff King 提交于
      This particular conversion is non-obvious, because nobody
      has passed our function the length of the destination
      buffer. However, the interface to checkout_entry specifies
      that the buffer must be at least TEMPORARY_FILENAME_LENGTH
      bytes long, so we can check that (meaning the existing code
      was not buggy, but merely worrisome to somebody reading it).
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      330c8e26
    • J
      grep: use xsnprintf to format failure message · 19bdd3e7
      Jeff King 提交于
      This looks at first glance like the sprintf can overflow our
      buffer, but it's actually fine; the p->origin string is
      something constant and small, like "command line" or "-e
      option".
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      19bdd3e7
    • J
      compat/hstrerror: convert sprintf to snprintf · 48bdf869
      Jeff King 提交于
      This is a trivially correct use of sprintf, as our error
      number should not be excessively long. But it's still nice
      to drop an sprintf call.
      
      Note that we cannot use xsnprintf here, because this is
      compat code which does not load git-compat-util.h.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      48bdf869
    • J
      stop_progress_msg: convert sprintf to xsnprintf · f5691aa6
      Jeff King 提交于
      The usual arguments for using xsnprintf over sprintf apply,
      but this case is a little tricky. We print to a fixed-size
      buffer if we have room, and otherwise to an allocated
      buffer. So there should be no overflow here, but it is still
      good to communicate our intention, as well as to check our
      earlier math for how much space the string will need.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      f5691aa6
    • J
      find_short_object_filename: convert sprintf to xsnprintf · c3bb0ac7
      Jeff King 提交于
      We use sprintf() to format some hex data into a buffer. The
      buffer is clearly long enough, and using snprintf here is
      not necessary. And in fact, it does not really make anything
      easier to audit, as the size we feed to snprintf accounts
      for the magic extra 42 bytes found in each alt->name field
      of struct alternate_object_database (which is there exactly
      to do this formatting).
      
      Still, it is nice to remove an sprintf call and replace it
      with an xsnprintf and explanatory comment, which makes it
      easier to audit the code base for overflows.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      c3bb0ac7
    • J
      use xsnprintf for generating git object headers · ef1286d3
      Jeff King 提交于
      We generally use 32-byte buffers to format git's "type size"
      header fields. These should not generally overflow unless
      you can produce some truly gigantic objects (and our types
      come from our internal array of constant strings). But it is
      a good idea to use xsnprintf to make sure this is the case.
      
      Note that we slightly modify the interface to
      write_sha1_file_prepare, which nows uses "hdrlen" as an "in"
      parameter as well as an "out" (on the way in it stores the
      allocated size of the header, and on the way out it returns
      the ultimate size of the header).
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      ef1286d3
    • J
      archive-tar: use xsnprintf for trivial formatting · f2f02675
      Jeff King 提交于
      When we generate tar headers, we sprintf() values directly
      into a struct with the fixed-size header values. For the
      most part this is fine, as we are formatting small values
      (e.g., the octal format of "mode & 0x7777" is of fixed
      length). But it's still a good idea to use xsnprintf here.
      It communicates to readers what our expectation is, and it
      provides a run-time check that we are not overflowing the
      buffers.
      
      The one exception here is the mtime, which comes from the
      epoch time of the commit we are archiving. For sane values,
      this fits into the 12-byte value allocated in the header.
      But since git can handle 64-bit times, if I claim to be a
      visitor from the year 10,000 AD, I can overflow the buffer.
      This turns out to be harmless, as we simply overflow into
      the chksum field, which is then overwritten.
      
      This case is also best as an xsnprintf. It should never come
      up, short of extremely malformed dates, and in that case we
      are probably better off dying than silently truncating the
      date value (and we cannot expand the size of the buffer,
      since it is dictated by the ustar format). Our friends in
      the year 5138 (when we legitimately flip to a 12-digit
      epoch) can deal with that problem then.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      f2f02675
    • J
      convert trivial sprintf / strcpy calls to xsnprintf · 5096d490
      Jeff King 提交于
      We sometimes sprintf into fixed-size buffers when we know
      that the buffer is large enough to fit the input (either
      because it's a constant, or because it's numeric input that
      is bounded in size). Likewise with strcpy of constant
      strings.
      
      However, these sites make it hard to audit sprintf and
      strcpy calls for buffer overflows, as a reader has to
      cross-reference the size of the array with the input. Let's
      use xsnprintf instead, which communicates to a reader that
      we don't expect this to overflow (and catches the mistake in
      case we do).
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      5096d490
    • J
      compat/inet_ntop: fix off-by-one in inet_ntop4 · db85a8a9
      Jeff King 提交于
      Our compat inet_ntop4 function writes to a temporary buffer
      with snprintf, and then uses strcpy to put the result into
      the final "dst" buffer. We check the return value of
      snprintf against the size of "dst", but fail to account for
      the NUL terminator. As a result, we may overflow "dst" with
      a single NUL. In practice, this doesn't happen because the
      output of inet_ntop is limited, and we provide buffers that
      are way oversized.
      
      We can fix the off-by-one check easily, but while we are
      here let's also use strlcpy for increased safety, just in
      case there are other bugs lurking.
      
      As a side note, this compat code seems to be BSD-derived.
      Searching for "vixie inet_ntop" turns up NetBSD's latest
      version of the same code, which has an identical fix (and
      switches to strlcpy, too!).
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      db85a8a9
    • J
      test-dump-cache-tree: avoid overflow of cache-tree name · 04724222
      Jeff King 提交于
      When dumping a cache-tree, we sprintf sub-tree names directly
      into a fixed-size buffer, which can overflow. We can
      trivially fix this by converting to xsnprintf to at least
      notice and die.
      
      This probably should handle arbitrary-sized names, but
      there's not much point. It's used only by the test scripts,
      so the trivial fix is enough.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      04724222
    • J
      progress: store throughput display in a strbuf · 3131977d
      Jeff King 提交于
      Coverity noticed that we strncpy() into a fixed-size buffer
      without making sure that it actually ended up
      NUL-terminated. This is unlikely to be a bug in practice,
      since throughput strings rarely hit 32 characters, but it
      would be nice to clean it up.
      
      The most obvious way to do so is to add a NUL-terminator.
      But instead, this patch switches the fixed-size buffer out
      for a strbuf. At first glance this seems much less
      efficient, until we realize that filling in the fixed-size
      buffer is done by writing into a strbuf and copying the
      result!
      
      By writing straight to the buffer, we actually end up more
      efficient:
      
        1. We avoid an extra copy of the bytes.
      
        2. Rather than malloc/free each time progress is shown, we
           can strbuf_reset and use the same buffer each time.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      3131977d
    • J
      trace: use strbuf for quote_crnl output · 0bb443fd
      Jeff King 提交于
      When we output GIT_TRACE_SETUP paths, we quote any
      meta-characters. But our buffer to hold the result is only
      PATH_MAX bytes, and we could double the size of the input
      path (if every character needs quoting). We could use a
      2*PATH_MAX buffer, if we assume the input will never be more
      than PATH_MAX. But it's easier still to just switch to a
      strbuf and not worry about whether the input can exceed
      PATH_MAX or not.
      
      The original copied the "p2" pointer to "p1", advancing
      both. Since this gets rid of "p1", let's also drop "p2",
      whose name is now confusing. We can just advance the
      original "path" pointer.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      0bb443fd
    • J
      mailsplit: make PATH_MAX buffers dynamic · 1d895f19
      Jeff King 提交于
      There are several PATH_MAX-sized buffers in mailsplit, along
      with some questionable uses of sprintf.  These are not
      really of security interest, as local mailsplit pathnames
      are not typically under control of an attacker, and you
      could generally only overflow a few numbers at the end of a
      path that approaches PATH_MAX (a longer path would choke
      mailsplit long before). But it does not hurt to be careful,
      and as a bonus we lift some limits for systems with
      too-small PATH_MAX varibles.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      1d895f19
    • J
      fsck: use strbuf to generate alternate directories · c1fd0809
      Jeff King 提交于
      When fsck-ing alternates, we make a copy of the alternate
      directory in a fixed PATH_MAX buffer. We memcpy directly,
      without any check whether we are overflowing the buffer.
      This is OK if PATH_MAX is a true representation of the
      maximum path on the system, because any path here will have
      already been vetted by the alternates subsystem. But that is
      not true on every system, so we should be more careful.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      c1fd0809
    • J
      add reentrant variants of sha1_to_hex and find_unique_abbrev · af49c6d0
      Jeff King 提交于
      The sha1_to_hex and find_unique_abbrev functions always
      write into reusable static buffers. There are a few problems
      with this:
      
        - future calls overwrite our result. This is especially
          annoying with find_unique_abbrev, which does not have a
          ring of buffers, so you cannot even printf() a result
          that has two abbreviated sha1s.
      
        - if you want to put the result into another buffer, we
          often strcpy, which looks suspicious when auditing for
          overflows.
      
      This patch introduces sha1_to_hex_r and find_unique_abbrev_r,
      which write into a user-provided buffer. Of course this is
      just punting on the overflow-auditing, as the buffer
      obviously needs to be GIT_SHA1_HEXSZ + 1 bytes. But it is
      much easier to audit, since that is a well-known size.
      
      We retain the non-reentrant forms, which just become thin
      wrappers around the reentrant ones. This patch also adds a
      strbuf variant of find_unique_abbrev, which will be handy in
      later patches.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      af49c6d0
    • J
      strbuf: make strbuf_complete_line more generic · 399ad553
      Jeff King 提交于
      The strbuf_complete_line function makes sure that a buffer
      ends in a newline. But we may want to do this for any
      character (e.g., "/" on the end of a path). Let's factor out
      a generic version, and keep strbuf_complete_line as a thin
      wrapper.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      399ad553
    • J
      add git_path_buf helper function · bb3788ce
      Jeff King 提交于
      If you have a function that uses git_path a lot, but would
      prefer to avoid the static buffers, it's useful to keep a
      single scratch buffer locally and reuse it for each call.
      You used to be able to do this with git_snpath:
      
        char buf[PATH_MAX];
      
        foo(git_snpath(buf, sizeof(buf), "foo"));
        bar(git_snpath(buf, sizeof(buf), "bar"));
      
      but since 1a83c240, git_snpath has been replaced with
      strbuf_git_path. This is good, because it removes the
      arbitrary PATH_MAX limit. But using strbuf_git_path is more
      awkward for two reasons:
      
        1. It adds to the buffer, rather than replacing it. This
           is consistent with other strbuf functions, but makes
           reuse of a single buffer more tedious.
      
        2. It doesn't return the buffer, so you can't format
           as part of a function's arguments.
      
      The new git_path_buf solves both of these, so you can use it
      like:
      
        struct strbuf buf = STRBUF_INIT;
      
        foo(git_path_buf(&buf, "foo"));
        bar(git_path_buf(&buf, "bar"));
      
        strbuf_release(&buf);
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      bb3788ce
    • J
      add xsnprintf helper function · 7b03c89e
      Jeff King 提交于
      There are a number of places in the code where we call
      sprintf(), with the assumption that the output will fit into
      the buffer. In many cases this is true (e.g., formatting a
      number into a large buffer), but it is hard to tell
      immediately from looking at the code. It would be nice if we
      had some run-time check to make sure that our assumption is
      correct (and to communicate to readers of the code that we
      are not blindly calling sprintf, but have actually thought
      about this case).
      
      This patch introduces xsnprintf, which behaves just like
      snprintf, except that it dies whenever the output is
      truncated. This acts as a sort of assert() for these cases,
      which can help find places where the assumption is violated
      (as opposed to truncating and proceeding, which may just
      silently give a wrong answer).
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      7b03c89e
    • J
      fsck: don't fsck alternates for connectivity-only check · fbe85e73
      Jeff King 提交于
      Commit 02976bf8 (fsck: introduce `git fsck --connectivity-only`,
      2015-06-22) recently gave fsck an option to perform only a
      subset of the checks, by skipping the fsck_object_dir()
      call. However, it does so only for the local object
      directory, and we still do expensive checks on any alternate
      repos. We should skip them in this case, too.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      fbe85e73
    • J
      archive-tar: fix minor indentation violation · 108332c7
      Jeff King 提交于
      This looks like a simple omission from 85390709 (archive-tar:
      unindent write_tar_entry by one level, 2012-05-03).
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      108332c7
    • J
      mailsplit: fix FILE* leak in split_maildir · d270d7b7
      Jeff King 提交于
      If we encounter an error while splitting a maildir, we exit
      the function early, leaking the open filehandle. This isn't
      a big deal, since we exit the program soon after, but it's
      easy enough to be careful.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      d270d7b7
    • J
      show-branch: avoid segfault with --reflog of unborn branch · 7cd17e80
      Jeff King 提交于
      When no branch is given to the "--reflog" option, we resolve
      HEAD to get the default branch. However, if HEAD points to
      an unborn branch, resolve_ref returns NULL, and we later
      segfault trying to access it.
      Signed-off-by: NJeff King <peff@peff.net>
      Signed-off-by: NJunio C Hamano <gitster@pobox.com>
      7cd17e80
  2. 22 9月, 2015 5 次提交
  3. 21 9月, 2015 6 次提交