commit fd9a631c56ff326bea2956b675f205cd474def4e
author Jeff King <peff@peff.net> Tue Aug 04 03:46:52 2020 -0400
committer Junio C Hamano <gitster@pobox.com> Tue Aug 04 09:31:02 2020 -0700
tree bc438dbd4fa2532080a5ee409febbcafe014a324
parent aec0bba106d8bce829671ca8659ad338aa677e9f

revision: avoid out-of-bounds read/write on empty pathspec

Running t4216 with ASan results in it complaining of an out-of-bounds
read in prepare_to_use_bloom_filter(). The issue is this code to strip a
trailing slash:

  last_index = pi->len - 1;
  if (pi->match[last_index] == '/') {

because we have no guarantee that pi->len isn't zero. This can happen if
the pathspec is ".", as we translate that to an empty string. And if
that read of random memory does trigger the conditional, we'd then do an
out-of-bounds write:

  path_alloc = xstrdup(pi->match);
  path_alloc[last_index] = '\0';

Let's make sure to check the length before subtracting. Note that for an
empty pathspec, we'd end up bailing from the function a few lines later,
which makes it tempting to just:

  if (!pi->len)
          return;

early here. But our code here is stripping a trailing slash, and we need
to check for emptiness after stripping that slash, too. So we'd have two
blocks, which would require repeating some cleanup code.

Instead, just skip the trailing-slash for an empty string. Setting
last_index at all in the case is awkward since it will have a nonsense
value (and it uses an "int", which is a too-small type for a string
anyway). So while we're here, let's:

  - drop last_index entirely; it's only used in two spots right next to
    each other and writing out "pi->len - 1" in both is actually easier
    to follow

  - use xmemdupz() to duplicate the string. This is slightly more
    efficient, but more importantly makes the intent more clear by
    allocating the correct-sized substring in the first place. It also
    eliminates any question of whether path_alloc is as long as
    pi->match (which it would not be if pi->match has any embedded NULs,
    though in practice this is probably impossible).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>