security.ArrayBound FP caused by the lack of modeling narrowing casts

[zufuliu]

It has false positive for following code (online at https://godbolt.org/z/9v8P684rc):

struct Foo {
    unsigned char get(unsigned char ch) const {
        return s[ch];
    }
    unsigned char s[256];
};

int bar(const Foo &foo, const char *s) {
    int j = 0;
    if (s) {
        const unsigned char ch = s[j];
        j++;
        if (static_cast<signed char>(ch) >= 0) {
            // nop
        } else {
            j += foo.get(ch);
        }
    }
    return j;
}

<source>:3:16: warning: Out of bound access to memory preceding the field 's' [clang-analyzer-security.ArrayBound]
    3 |         return s[ch];
      |                ^
[<source>:10:9: note: Assuming 's' is non-null](javascript:;)
   10 |     if (s) {
      |         ^
[<source>:10:5: note: Taking true branch](javascript:;)
   10 |     if (s) {
      |     ^
[<source>:13:13: note: Assuming 'ch' is < 0](javascript:;)
   13 |         if (static_cast<signed char>(ch) >= 0) {
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[<source>:13:9: note: Taking false branch](javascript:;)
   13 |         if (static_cast<signed char>(ch) >= 0) {
      |         ^
[<source>:16:18: note: Calling 'Foo::get'](javascript:;)
   16 |             j += foo.get(ch);
      |                  ^~~~~~~~~~~
[<source>:3:16: note: Access of the field 's' at negative byte offset](javascript:;)
    3 |         return s[ch];
      |                ^~~~~
1 warning generated.

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Zufu Liu (zufuliu)

It has false positive for following code (online at https://godbolt.org/z/9v8P684rc): ```c++ struct Foo { unsigned char get(unsigned char ch) const { return s[ch]; } unsigned char s[256]; };

int bar(const Foo &foo, const char *s) {
int j = 0;
if (s) {
const unsigned char ch = s[j];
j++;
if (static_cast<signed char>(ch) >= 0) {
// nop
} else {
j += foo.get(ch);
}
}
return j;
}

```console
<source>:3:16: warning: Out of bound access to memory preceding the field 's' [clang-analyzer-security.ArrayBound]
    3 |         return s[ch];
      |                ^
[<source>:10:9: note: Assuming 's' is non-null](javascript:;)
   10 |     if (s) {
      |         ^
[<source>:10:5: note: Taking true branch](javascript:;)
   10 |     if (s) {
      |     ^
[<source>:13:13: note: Assuming 'ch' is < 0](javascript:;)
   13 |         if (static_cast<signed char>(ch) >= 0) {
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[<source>:13:9: note: Taking false branch](javascript:;)
   13 |         if (static_cast<signed char>(ch) >= 0) {
      |         ^
[<source>:16:18: note: Calling 'Foo::get'](javascript:;)
   16 |             j += foo.get(ch);
      |                  ^~~~~~~~~~~
[<source>:3:16: note: Access of the field 's' at negative byte offset](javascript:;)
    3 |         return s[ch];
      |                ^~~~~
1 warning generated.

[steakhal]

I think this one is not the fault of the checker.
We just dont model narrowing integral casts, so it leads us in a wrong state.

I wish I'd know off the top of my head the umbrella ticket for casts. This shoulgd go under that one.

Anyone could link it?

[steakhal]

To set the expectations, it's a large subject, so it's difficult to fix.

I don't know anyone in the community right now, who would priortize this bug.

To fight against this FP, you could always put an assert before the access to guwrd it. The analyzer would likely see it and refine its constraints which should lead to thr suppression of your FP.

[NagyDonat]

I think this one is not the fault of the checker. We just dont model narrowing integral casts, so it leads us in a wrong state.

Yes, the analyzer engine doesn't model a char (a.k.a. signed char) → unsigned char cast. The analyzer introduces a symbolic value (i.e. placeholder for a certain unknown number) to represent the value of s[j], records that this symbolic value is a signed 8-bit number (char), then inaccurately records that the the variable unsigned char ch stores this symbolic value (instead of recording that ch stores the result of converting this signed value to an unsigned type).

To set the expectations, it's a large subject, so it's difficult to fix.

As far as I understand we cannot "just represent the casts accurately" because the analyzer is not smart enough to transfer knowledge between different symbols that are derived by casts from each other. (E.g. if the analyzer deduces that a symbolic value $X$ is greater than 3, it cannot deduce that the symbol "$X$ converted to unsigned char" is also greater than 3.)

I don't know anyone in the community right now, who would priortize this bug.

If false positives like this are common and problematic in real-world code, we could create a "duct-tape" fix within the security.ArrayBound checker to filter out most of them. Hacks like this cannot cover all the checkers, but they could cover a few specific ones.

[NagyDonat]

@steakhal this is probably the umbrella ticket that you're thinking of: #39492

[steakhal]

@steakhal this is probably the umbrella ticket that you're thinking of: #39492

Ah yes. Thanks!

[zufuliu]

following is also flagged:
https://godbolt.org/z/Wsc3oKacW

struct Foo {
    unsigned char get(unsigned char ch) const {
        return s[ch];
    }
    unsigned char s[256];
};

int bar(const Foo &foo, const char *s) {
    int j = 0;
    if (s) {
        const unsigned char ch = s[j];
        j++;
        if (static_cast<signed char>(ch) >= 0) { /* nop */}
        j += foo.get(ch);
    }
    return j;
}

<source>:3:16: warning: Out of bound access to memory preceding the field 's' [clang-analyzer-security.ArrayBound]
    3 |         return s[ch];
      |                ^
<source>:10:9: note: Assuming 's' is non-null
   10 |     if (s) {
      |         ^
<source>:10:5: note: Taking true branch
   10 |     if (s) {
      |     ^
<source>:13:13: note: Assuming 'ch' is < 0
   13 |         if (static_cast<signed char>(ch) >= 0) { /* nop */}
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:13:9: note: Taking false branch
   13 |         if (static_cast<signed char>(ch) >= 0) { /* nop */}
      |         ^
<source>:14:14: note: Calling 'Foo::get'
   14 |         j += foo.get(ch);
      |              ^~~~~~~~~~~
<source>:3:16: note: Access of the field 's' at negative byte offset
    3 |         return s[ch];
      |                ^~~~~
1 warning generated.

following are not flagged:
https://godbolt.org/z/zMbh1xbh5

struct Foo {
    unsigned char get(unsigned char ch) const {
        return s[ch];
    }
    unsigned char s[256];
};

int bar(const Foo &foo, const char *s) {
    int j = 0;
    if (s) {
        const unsigned char ch = s[j];
        j++;
        j += foo.get(ch);
    }
    return j;
}

https://godbolt.org/z/zb41G9nTE

int bar(const unsigned char (&foo)[256], const char *s) {
    int j = 0;
    if (s) {
        const unsigned char ch = s[j];
        j++;
        if (static_cast<signed char>(ch) >= 0) {
            // nop
        } else {
            j += foo[ch];
        }
    }
    return j;
}

https://godbolt.org/z/KjWaPqohM

struct Foo {
    unsigned char get(unsigned char ch) const {
        return s[ch];
    }
    unsigned char s[256];
};

int bar(const Foo &foo, const char *s) {
    int j = 0;
    if (s) {
        const unsigned char ch = s[j] & 0xff;
        j++;
        if (static_cast<signed char>(ch) >= 0) {
            // nop
        } else {
            j += foo.get(ch);
        }
    }
    return j;
}

[NagyDonat]

The (approximately) shortest reproducer of this bug is:

int table[256];
int foo(signed char x) {
  unsigned char y = x;
  if (x >= 0)
    return x;
  return table[y];
}

I created PR #127062 which adds this (and two related cases) to the tests of the analyzer to highlight and document that this problem exists.

I'll probably create a "duct tape" commit that silences most of the the security.ArrayBound false positives caused by this bug.

[zufuliu]

I created PR #127062 which adds this (and two related cases) to the tests of the analyzer to highlight and document that this problem exists.

Thanks, I filled issue #127188 based on the reduced code.