[PATCH v3] diff: check range before dereferencing an array element

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Before accessing an array element at a given index, it should be
verified 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. This is the point of CodeQL's
`cpp/offset-use-before-range-check` rule.

This CodeQL rule unfortunately is also triggered by the
`fill_es_indent_data()` code, even though the condition `off < len - 1`
does not even need to guarantee that the offset is in bounds (`s` points
to a NUL-terminated string, for which `s[off] == '\r'` would fail before
running out of bounds).

Let's work around this rare false positive to help us use an otherwise
mostly useful tool is a worthy thing to do.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Range-check array index before access
    
    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 v2:
    
     * Rebased on top of js/range-check-codeql-workaround.
     * Rephrased the commit message.
    
    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.

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

Range-diff vs v2:

 1:  ddfb44ed924 ! 1:  3c6e2647863 diff: check range before dereferencing an array element
     @@ Metadata
       ## Commit message ##
          diff: 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.
     +    Before accessing an array element at a given index, it should be
     +    verified 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. This is the point of CodeQL's
     +    `cpp/offset-use-before-range-check` rule.
      
     -    Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.
     +    This CodeQL rule unfortunately is also triggered by the
     +    `fill_es_indent_data()` code, even though the condition `off < len - 1`
     +    does not even need to guarantee that the offset is in bounds (`s` points
     +    to a NUL-terminated string, for which `s[off] == '\r'` would fail before
     +    running out of bounds).
     +
     +    Let's work around this rare false positive to help us use an otherwise
     +    mostly useful tool is a worthy thing to do.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
 2:  73cae301293 < -:  ----------- read-cache: check range before dereferencing an array element


 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index c89c15d98e0..18ba3060460 100644
--- a/diff.c
+++ b/diff.c
@@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)
 
 	/* skip any \v \f \r at start of indentation */
 	while (s[off] == '\f' || s[off] == '\v' ||
-	       (s[off] == '\r' && off < len - 1))
+	       (off < len - 1 && s[off] == '\r'))
 		off++;
 
 	/* calculate the visual width of indentation */

base-commit: 0f558141ed3b93b393151367b9569446cd24caab
-- 
gitgitgadget