[v2,0/2] Range-check array index before access

Message

Elijah Newren via GitGitGadget March 27, 2025, 11:05 a.m. UTC

If we want to check the range of an array index, it makes much more sense to
do it before accessing the corresponding array element, not afterwards.

There are two more instances of this in the clar code, fixes for which I
offer in https://github.com/clar-test/clar/pull/115.

Changes since v1:

 * Clarified in the commit message of the second patch that this range-check
   technically was already right before the array access it wants to guard,
   but that it still makes sense to move that range-check to the beginning
   of the loop condition.

Johannes Schindelin (2):
  diff: check range before dereferencing an array element
  read-cache: check range before dereferencing an array element

 diff.c       | 2 +-
 read-cache.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)


base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1887%2Fdscho%2Frange-check-array-index-before-access-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1887/dscho/range-check-array-index-before-access-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1887

Range-diff vs v1:

 1:  ddfb44ed924 = 1:  ddfb44ed924 diff: check range before dereferencing an array element
 2:  d4e94a243b0 ! 2:  73cae301293 read-cache: check range before dereferencing an array element
     @@ Commit message
          read-cache: check range before dereferencing an array element
      
          Before accessing an array element at a given index, we should make sure
     -    that the index is within the desired bounds, not afterwards, otherwise
     -    it may not make sense to even access the array element in the first
     -    place.
     +    that the index is within the desired bounds, otherwise it makes little
     +    sense to access the array element in the first place.
      
     -    Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.
     +    In this instance, testing whether `ce->name[common]` is the trailing NUL
     +    byte is technically different from testing whether `common` is within
     +    the bounds of `previous_name`. It is also redundant, as the range-check
     +    guarantees that `previous_name->buf[common]` cannot be NUL and therefore
     +    the condition `ce->name[common] == previous_name->buf[common]` would not
     +    be met if `ce->name[common]` evaluated to NUL.
     +
     +    However, in the interest of reducing the cognitive load to reason about
     +    the correctness of this loop (so that I can focus on interesting
     +    projects again), I'll simply move the range-check to the beginning of
     +    the loop condition and keep the redundant NUL check.
     +
     +    This acquiesces CodeQL's `cpp/offset-use-before-range-check` rule.
      
          Signed-off-by: Johannes Schindelin <[e-mail]>