[clang-tidy] Add bugprone-strlen-in-array-index checker

[kovacs-levent]

Summary

According to https://wiki.sei.cmu.edu/confluence/display/c/FIO37-C.+Do+not+assume+that+fgets%28%29+or+fgetws%28%29+returns+a+nonempty+string+when+successful, bugprone-strlen-in-array-index checker is created.

The checker checks for the usage of strlen() on an array within it's own subscript operator. The main issue in the ticket is not the usage of fgets(), but rather the resulting write-outside-array-bounds error. This could occur if the array contains binary data and the terminating '\0' character is read into the array.

The check helps to narrow the scope by guarding against the usage of strlen in the subscript operator.

[kovacs-levent]

Messed up the first diff, here's the proper one.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

123

Please enclose strlen in double back-ticks.

124

Please enclose operator [] in double back-ticks.

152

Please add empty line after.

clang-tools-extra/docs/clang-tidy/checks/bugprone-strlen-in-array-index.rst

7

Please synchronize first statement with Release Notes.

8

Please enclose \0 on single back-ticks and strlen() in double back-ticks.

10

Please enclose strlen in double back-ticks.

[kovacs-levent]

Doc improvements.

[kovacs-levent]

Fix in Releasenotes.rst.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/bugprone-strlen-in-array-index.rst

18

Please Clang-format example code. (void) is not necessary in C++ code. Or change code block to C.

[kovacs-levent]

Thanks. I missed formatting rst files because the clang-format messed up the rst part and I just forgot about them.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp

38

I think this diagnostic is going to be very chatty -- doing strlen(buf) - N inside of an array index isn't dangerous unless there is a null character within the first N bytes of the buffer, and I suspect there's plenty of code that validates the buffer before performing the operation. Have you tried running this check over some large code bases to see what it's true/false positive ratio is?

This check feels like it is a very specific instance of checking for buffer overflows which would be better-handled by the static analyzer (which can track the characteristics of the buffer better through control and data flow analysis).

[steakhal]

clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp

38

I agree with you.
With the right configuration, the CSA can already catch this in some scenarios.
However, a tidy check could also be beneficial, if the false/true-positive rate is convincing enough.

I will measure this patch on several projects in the future and publish the results in a public CodeChecker instance.
@kovacs-levent, you can categorize the results and publish a small summary about them here.
Right now I'm a bit busy, but I hope we don't rush anywhere :)

[kovacs-levent]

clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp

38

After the checker was written I gave it a quick test run using CodeChecker on some open source projects (with some help by @Charusso). Unfortunately, I don't have the results now, but you're right in that there were some false positives.

However, it was kind of dependent on the project, some projects had the validation sorted out, while some didn't. I couldn't say an overall false/true-positive rate, but I'll try to find the test results. What are we aiming for regarding this rate?

Thanks @steakhal, I'm looking forward to your results.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp

38

Right now I'm a bit busy, but I hope we don't rush anywhere :)

There's no rush that I'm aware of, so take whatever time you need. :-)

However, it was kind of dependent on the project, some projects had the validation sorted out, while some didn't. I couldn't say an overall false/true-positive rate, but I'll try to find the test results. What are we aiming for regarding this rate?

We don't have a hard-and-fast rule for false positive rates, but the general guidance is that the frontend should have almost no false positives, analysis can have more false positives than the FE, and tidy can have more false positives than analysis.

I'm mostly looking for a sense as to whether this finds enough true positives that users would keep the check enabled, or whether the check is mostly false positives and likely to be disabled. I'm also curious whether the existing CSA checks catch the same true positives or could be easily modified to catch them, as I think the flow-sensitive check will lead to better results in the long-run.

[steakhal]

@kovacs-levent @aaron.ballman
On this CodeChecker instance, you can find the reports on several projects.
The relevant runs are suffixed with kovacs_D92777. @kovacs-levent, you can use the demo/demo user as well, for categorizing the reports into false/true positives.
Once you clicked on a run, you can filter out the relevant reports by the Checker's name bugprone-strlen-in-array-index.

There are some funny ones, but overall, I think it does not produce an immense amount of reports.

How should the developer 'fix' their code if they certain that the report is a false-positive? What pattern do you suggest suppressing a false-positive? @kovacs-levent
That could suppress the one I cherry-picked before.

---

In D92777#inline-873321, @aaron.ballman wrote:

I'm mostly looking for a sense as to whether this finds enough true positives that users would keep the check enabled, or whether the check is mostly false positives and likely to be disabled.

Yeah, we should aggregate the results first.

I'm also curious whether the existing CSA checks catch the same true positives or could be easily modified to catch them, as I think the flow-sensitive check will lead to better results in the long-run.

It can. The core,alpha.security.taint,alpha.security.ArrayBoundV2 checkers could catch it, if the strlen would propagate taint to the length value.
That way if the analyzer could not prove that the access is in bound, it could aggressively diagnose this because the subscript expression is tainted.
But currently, taint analysis and the ArrayBoundV2 are not yet ready for production.

[kovacs-levent]

This comment was removed by kovacs-levent.

[kovacs-levent]

Sorry for taking so long with the reply, I've prepared the results.

First of all,a recommended solution generally is to store the length in a variable, and use that. Most of the false-positive results are calculating the strlen() of the same string multiple times and this gets rid of that as well.

size_t len = strlen(buf);
if (len > 0) {
    buf[len-1] = '0';
}

Also this makes the check much more readable than in some of the false-positive examples.

There weren't many hits in the test data so I compiled all of them.

memcached
Positives:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=Memchached_1.6.8_Changed_kovacs_D92777&detection-status=New&sort-by=checkerId&sort-desc=false&report-id=55297
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=Memchached_1.6.8_Changed_kovacs_D92777&detection-status=New&sort-by=checkerId&sort-desc=false&report-id=55298
There's no length checking after copying from optarg.

False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=Memchached_1.6.8_Changed_kovacs_D92777&detection-status=New&sort-by=checkerId&sort-desc=false&report-id=55316
There are two of these accesses, there's a NULL check in place, so the strlen() call is guarded.

False-postivie rate: 2/4

curl
Positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Curl_curl-7_66_0_Changed_kovacs_D92777&report-id=55430
The path buffer is not checked

False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Curl_curl-7_66_0_Changed_kovacs_D92777&report-id=55429
There's a check for the first character.

False-positive rate: 1/2

vim
Positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Vim_v8.2.1920_Changed_kovacs_D92777&report-id=56169
I can't see an obvious check here for lno, if the returned token is exactly one character long this can cause problems

False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Vim_v8.2.1920_Changed_kovacs_D92777&report-id=56167
There are possibly 3 accesses here for the same buffer, and the fname buffer check is a long way up at L 509.

False-postivie rate: 1/2

postgreSQL
Positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60172
I can't see a check on boot_yytext, but it's assumed to be a quoted string.

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=62650
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=62649
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=62648
For me these are a bit wtf, so I added them. Even if there's a check on the duplicated strings, this should be more readable

False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60042
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60582
Yeah, these are a bit funny...

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60615
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60582
There's a check for the first character

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=62172
I guess this is false positive, because it overwrites the first character beforehand and pg_strdup will throw an error if the first character is a null pointer in the copied string

False-positive rate: 5/9

Tmux
In this project all the matches are false-positives unfortunately.
False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=tmux_3.0_Changed_kovacs_D92777&report-id=55401
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=tmux_3.0_Changed_kovacs_D92777&report-id=55404
It's a false positive because there's a check in place, but computing strlen(copy) twice is not pretty, the recommended code snipet avoids this.

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=tmux_3.0_Changed_kovacs_D92777&report-id=55403
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=tmux_3.0_Changed_kovacs_D92777&report-id=55406
The buffer is filled beforehand in a wierd way, but it should be safe for access.

False-positive rate: 7/7

Overall
Overall except tmux the False-positive rate for the projects individually is mainly around 50%, but in some cases the recommended snippet can improve performance and readability.

Overall false-positive rate: 16/24.

The false-positives which could be improved by the code snippet are:

• vim's and tmux's example, where strlen() is computed multiple times in some cases. https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Vim_v8.2.1920_Changed_kovacs_D92777&report-id=56167 (3 cases in total)
• For me the following snippet isn't that readable as the recommended snippet, but I think this is debatable:

if(path_to_use[0] &&
   (path_to_use[strlen(path_to_use) - 1] != '/') )

This snippet is from curl's false positive, but tmux also uses it a few times.

[steakhal]

@kovacs-levent, thorough and accurate evaluation!
See my comments inline.

---

In D92777#2526215, @kovacs-levent wrote:

First of all,a recommended solution generally is to store the length in a variable, and use that. Most of the false-positive results are calculating the strlen() of the same string multiple times and this gets rid of that as well.

size_t len = strlen(buf);
if (len > 0) {
    buf[len-1] = '0';
}

Also this makes the check much more readable than in some of the false-positive examples.

There are multiple ways of checking this.
Eg.: *buf, buf[0], len > 0, len mean the same thing in this context.

In most of the cases, your checker fired for read accesses. In those cases, you could offer a simpler alternative such as:

if (path_to_use[0] && path_to_use[strlen(path_to_use) - 1] != '/')

You can guard the access using a negation and the or operator as well. You should also recognize this.

if (!path_to_use[0] || path_to_use[strlen(path_to_use) - 1] != '/')

Accessing the first character might be done using the pointer dereference operator:

if (*path_to_use && path_to_use[strlen(path_to_use) - 1] != '/')

To ignore these cases, here is a grammar in BNF form. If the access you are dealing with is a <safe_read_access>, you should ignore that.

<access> ::= <id>[ strlen(<id>) - 1 ]

<relational_operator> ::=  < | > | <= | >= | != | ==

<compare_access> ::= <access>
                 | <access> <relational_operator> <constant?>
                 | <constant?> <relational_operator> <access>

<first_character> ::= <id>[0]
                  | * <id>

<safe_read_access> ::= <first_character> && <compare_access>
                   | ! <first_character> || <compare_access>
                   |   <first_character> && <compare_access>
                   | ! <first_character> || <compare_access>

For write accesses, it's really hard to give a generic pattern for suppression. The one you proposed is the best I can think of.
But we have to find a reasonable answer for these questions to get it working:

• How should one suppress multiple warnings of the same kind? How does this compose?

if (len1 && len2) {
  buf1[len1 - 1] = 'A';
  buf2[len2 - 1] = 'B';
}

• Only the immediate parent if condition will be considered as length checks?
• What if the user doesn't want an extra branch there due to performance reasons? Could an assert(len1); assert(len2); suppress the warning?

AFAIK, walking backward the AST is really expensive, thus not recommended.
Am I right about that @aaron.ballman?

So, finding the condition of the parent if seems tricky to pull off. The same applies for asserts.

There weren't many hits in the test data so I compiled all of them.

memcached
Positives:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=Memchached_1.6.8_Changed_kovacs_D92777&detection-status=New&sort-by=checkerId&sort-desc=false&report-id=55297
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=Memchached_1.6.8_Changed_kovacs_D92777&detection-status=New&sort-by=checkerId&sort-desc=false&report-id=55298
There's no length checking after copying from optarg.

Confirmed! This bug can be triggered indeed. Nice catch.

False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=Memchached_1.6.8_Changed_kovacs_D92777&detection-status=New&sort-by=checkerId&sort-desc=false&report-id=55316
There are two of these accesses, there's a NULL check in place, so the strlen() call is guarded.

Yes, it's safe.

False-postivie rate: 2/4

curl
Positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Curl_curl-7_66_0_Changed_kovacs_D92777&report-id=55430
The path buffer is not checked

Aaah, it's really tricky.
The data->state.up.path aliases with path since path is initialized to data->state.up.path and not modified again.
And the access looks like this: !*data->state.up.path && path[strlen(path) - 1] != '/' which tries checking that the path is not empty and not ends with /.
Unfortunately, there is a negation, so it is checking the ending ONLY IF path points to an empty string.
Even if it were checking the right thing, I still agree with you. We should warn about this case as well. Aliasing is just a headache for everybody.

Really nice catch. You should probably file an issue about this.

False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Curl_curl-7_66_0_Changed_kovacs_D92777&report-id=55429
There's a check for the first character.

Yes, for this time they are checking the right thing!

False-positive rate: 1/2

vim
Positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Vim_v8.2.1920_Changed_kovacs_D92777&report-id=56169
I can't see an obvious check here for lno, if the returned token is exactly one character long this can cause problems

It is safe. You can assume there that the lno is a non-empty string (due to strtok) - which means that it has at least ONE character + a zero-terminator character.
So, it is a false-positive.

False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=Vim_v8.2.1920_Changed_kovacs_D92777&report-id=56167
There are possibly 3 accesses here for the same buffer, and the fname buffer check is a long way up at L 509.

It requires contextual information to decide. It's probably a false-positive, yup.

False-postivie rate: 1/2

postgreSQL
Positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60172
I can't see a check on boot_yytext, but it's assumed to be a quoted string.

I don't think YACC scanner codes really have such bugs. I think the grammar it parses ensures that the content is quoted. I did not investigate it though.

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=62650
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=62649
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=62648
For me these are a bit wtf, so I added them. Even if there's a check on the duplicated strings, this should be more readable

The same reasoning applies.

False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60042

Calling strlen 2x is inefficient. It's not a problem to warn for this.

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60582
Yeah, these are a bit funny...

I don't mind to warn for this as well. These are beyond our capabilities.

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60615

Hm, it's interesting. We should recognize this pattern as well. And ignore such cases.

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=60582
There's a check for the first character

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=PostgreSQL_REL_13_0_Changed_kovacs_D92777&report-id=62172
I guess this is false positive, because it overwrites the first character beforehand and pg_strdup will throw an error if the first character is a null pointer in the copied string

Yes, it's a false-positive. If grolist is strlen(grolist) < 3 it skips it. It can not overwrite the zero-terminator accidentally, and it remains a valid c-string, calling strlen subsequently is safe.
IMO it's perfectly fine to warn for this case as well.

False-positive rate: 5/9

Tmux
In this project all the matches are false-positives unfortunately.
False-positive:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=tmux_3.0_Changed_kovacs_D92777&report-id=55401

Wait, why the second line is highlighted?
The check happens a line above the report.

https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=tmux_3.0_Changed_kovacs_D92777&report-id=55403
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=tmux_3.0_Changed_kovacs_D92777&report-id=55406
The buffer is filled beforehand in a wierd way, but it should be safe for access.

For the first yes, but I'm not sure about the second one.
If cmd_table == NULL, then the body of the loop will be skipped. In that case strlen(s) == 0, causing an invalid access.
I wouldn't mind a warning there.

[kovacs-levent]

I'm going to be honest, I have no idea how to move forward with this checker. I guess you've made some suggestions @steakhal, but I don't have any idea how to implement these as improvements.

You gave this grammar for improving read access triggers, but this says almost nothing to me about how should I change the code.

For write accesses you even mentioned that it's tricky to give a generic pattern and that walking backwards the AST is not a good idea. What this says to me is that this whole ordeal should be avoided.

I suppose I'm looking for some sort of verdict. The results are there, whether the change is accepted or not is up to the review. If it's not accepted, then I'd like to get some guidance about how to move forward to hopefully improve the checker and get it accepted. @aaron.ballman can you please comment?

[steakhal]

In D92777#2590637, @kovacs-levent wrote:

I'm going to be honest, I have no idea how to move forward with this checker. I guess you've made some suggestions @steakhal, but I don't have any idea how to implement these as improvements.

Sorry for my complicated wording xD Let me express my thoughts in code instead.
Here is the matcher I roughly imagined:
https://godbolt.org/z/q4WsWj
The idea behind this is to explicitly recognize and ignore certain patterns. Such as *p && Val == p[strlen(p) - 1].
This way you can further reduce the number of false positives.

However, I can't see any reasonable way of suppressing this checker for write accesses.
Imagine this code:

p[0] = 'a';
// more code
// strlen returns 1 or greater
p[strlen(p) - 1] = 'X'; // safe, but you will still report it

Recommending to hoist the strlen(p) - 1 to a local variable is not a real solution, but a workaround. Recommending this pattern to the user is probably not ideal.
However, I don't have any better suggestions for this - so we stuck with this suppression mechanism.

I'd like to see how well the AST matcher suppression mechanism achieves in the wild.

[whisperity]

The tests are a bit slim. We definitely need more cases... what if the array isn't allocated locally, but received as a parameter? I take that we need both an fgets and a strlen call in the same function(?) to emit diagnostics. Are we sure it will work if the array is originally from somewhere else?

clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp

21

While certainly not natural, if we're already here, it wouldn't take more than a few additional lines to match the equivalent but "expanded" version of array indexing:

*(buf + strlen(buf) - 1) = 0;

22

What's a hard implicitCastExpr doing here? Does a cast always happen? I feel this might be a bit too restrictive... there are matcher adaptors like ignoringParenImpCasts, that route should be investigated.

26–28

strlen_s, wcslen_s? If I am reading this correctly then in case the input buffer starts with a NUL (and the safe size is large enough) then it behaves the same way strlen and will still return 0.

If we are already here, it might be worth to also check for strnlen. It is POSIX and BSD specific, but behaves the same way as strlen_s.

31–32

Why is the inner matcher duplicated here? What cases do we have a direct (?) - and a non-direct (descendant) one?

clang-tools-extra/test/clang-tidy/checkers/bugprone-strlen-in-array-index.cpp

16–17

Such code examples might be covered by non-permissive copyright, which might be incompatible with LLVM's licence. I was unable to find anything wrt. copyright on SEI CERT's Confluence, which in this case makes the work to be (strictly) copyrighted by default. I am not sure what the actual situation is, but such a direct mention of copying could lead to problems.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "SignalHandlerCheck.h"
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "SpuriouslyWakeUpFunctionsCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
+#include "StrlenInArrayIndexCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousIncludeCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "TerminatingContinueCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SpuriouslyWakeUpFunctionsCheck>(
         "bugprone-spuriously-wake-up-functions");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "bugprone-string-integer-assignment");
     CheckFactories.registerCheck<StringLiteralWithEmbeddedNulCheck>(
         "bugprone-string-literal-with-embedded-nul");
+    CheckFactories.registerCheck<StrlenInArrayIndexCheck>(
+        "bugprone-strlen-in-array-index");
     CheckFactories.registerCheck<SuspiciousEnumUsageCheck>(
         "bugprone-suspicious-enum-usage");
     CheckFactories.registerCheck<SuspiciousIncludeCheck>(
         "bugprone-suspicious-include");
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
         "bugprone-suspicious-memset-usage");
     CheckFactories.registerCheck<SuspiciousMissingCommaCheck>(
         "bugprone-suspicious-missing-comma");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   SignalHandlerCheck.cpp
   SignedCharMisuseCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   SpuriouslyWakeUpFunctionsCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
+  StrlenInArrayIndexCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousIncludeCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   TerminatingContinueCheck.cpp

clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.h

+//===--- StrlenInArrayIndex.h - clang-tidy *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRLENINARRAYINDEXCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRLENINARRAYINDEXCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Checks for the usage of strlen() on an array within its own [] operator.
+/// This can lead to write-outside-bounds error if used in the wrong context.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-strlen-in-array-index.html
+class StrlenInArrayIndexCheck : public ClangTidyCheck {
+public:
+  StrlenInArrayIndexCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRLENINARRAYINDEX_H

clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp

+//===--- StrlenInArrayIndex.cpp - clang-tidy -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "StrlenInArrayIndexCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void StrlenInArrayIndexCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      arraySubscriptExpr(
+          allOf(hasBase(implicitCastExpr(hasSourceExpression(
+                    declRefExpr(to(varDecl().bind("buf")))))),
+                hasIndex(allOf(
+                    hasDescendant(callExpr(
+                        allOf(callee(functionDecl(
+                                  hasAnyName("::strlen", "::std::strlen",
+                                             "::wcslen", "::std::wcslen"))),
+                              hasArgument(0, declRefExpr(to(varDecl(
+                                                 equalsBoundNode("buf")))))))),
+                    anyOf(hasDescendant(binaryOperator(hasOperatorName("-"))),
+                          binaryOperator(hasOperatorName("-")))))))
+          .bind("arrayExpr"),
+      this);
+}
+
+void StrlenInArrayIndexCheck::check(const MatchFinder::MatchResult &Result) {
+  diag(Result.Nodes.getNodeAs<ArraySubscriptExpr>("arrayExpr")->getBeginLoc(),
+       "strlen with subtraction could cause out-of bounds memory access "
+       "in array's own subscript expression.");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

 //===--- CERTTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../bugprone/BadSignalToKillThreadCheck.h"
 #include "../bugprone/ReservedIdentifierCheck.h"
 #include "../bugprone/SignalHandlerCheck.h"
 #include "../bugprone/SignedCharMisuseCheck.h"
 #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h"
+#include "../bugprone/StrlenInArrayIndexCheck.h"
 #include "../bugprone/UnhandledSelfAssignmentCheck.h"
 #include "../google/UnnamedNamespaceInHeaderCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "../performance/MoveConstructorInitCheck.h"
 #include "../readability/UppercaseLiteralSuffixCheck.h"

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc
   <clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc>` check.
 
 - New :doc:`bugprone-redundant-branch-condition
   <clang-tidy/checks/bugprone-redundant-branch-condition>` check.
 
   Finds condition variables in nested ``if`` statements that were also checked
   in the outer ``if`` statement and were not changed.
 
+- New :doc:`bugprone-strlen-in-array-index
+  <clang-tidy/checks/bugprone-strlen-in-array-index>` check.
+
+  Finds ``strlen`` function calls if they are used on an array within the
+  array's own index ``operator[]``. This could cause out-of-bounds memory access
+  in some special scenarios.
+
+
 - New :doc:`concurrency-mt-unsafe <clang-tidy/checks/concurrency-mt-unsafe>`
   check.
 
   Finds thread-unsafe functions usage. Currently knows about POSIX and
   Glibc function sets.
 
 - New :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone-signal-handler>` check.
 
   Finds functions registered as signal handlers that call non asynchronous-safe
   functions.
 
 - New :doc:`cert-sig30-c
   <clang-tidy/checks/cert-sig30-c>` check.
 
   Alias to the :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone-signal-handler>` check.
 
 - New :doc:`readability-function-cognitive-complexity
   <clang-tidy/checks/readability-function-cognitive-complexity>` check.
 
   Flags functions with Cognitive Complexity metric exceeding the configured limit.
 
+New check aliases
+^^^^^^^^^^^^^^^^^
+
+- New alias :doc:`cert-fio37-c
+  <clang-tidy/checks/cert-fio37-c>` to
+  :doc:`bugprone-strlen-in-array-index
+  <clang-tidy/checks/bugprone-strlen-in-array-index>` was added.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`modernize-loop-convert
   <clang-tidy/checks/modernize-loop-convert>` check.
 
   Now able to transform iterator loops using ``rbegin`` and ``rend`` methods.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-strlen-in-array-index.rst

+.. title:: clang-tidy - bugprone-strlen-in-array-index
+
+bugprone-strlen-in-array-index
+==============================
+
+Finds ``strlen`` function calls if they are used on an array within the array's
+own index ``operator[]``. If there's a `\0` character in the array, ``strlen``
+won't return the correct length (this can happen with binary data). This can
+cause access-outside-array-bounds error, if the array is indexed with ``strlen``.
+
+Example:
+
+.. code-block:: c++
+
+enum { BUFFER_SIZE = 1024 };
+ 
+void func() {
+  char buf[BUFFER_SIZE];
+  fgets(buf, sizeof(buf), stdin) == NULL);  // read "" or '\0' as first char
+  buf[strlen(buf) - 1] = '\0';              // error
+}

clang-tools-extra/test/clang-tidy/checkers/bugprone-strlen-in-array-index.cpp

+// RUN: %check_clang_tidy %s bugprone-strlen-in-array-index %t
+
+typedef void *FILE;
+
+extern FILE *stdin;
+#define NULL ((char *)0)
+extern unsigned int strlen(const char *str);
+extern char *fgets(char *str, int num, FILE *stream);
+extern const char *strchr(const char *str, int character);
+extern char *strchr(char *str, int character);
+
+namespace std {
+extern unsigned int strlen(const char *str);
+}
+
+// Examples were copied from the cert-fio37-c issue (with some added calls):
+// https://wiki.sei.cmu.edu/confluence/display/c/FIO37-C.+Do+not+assume+that+fgets%28%29+or+fgetws%28%29+returns+a+nonempty+string+when+successful
+
+void func() {
+  unsigned int BUFFER_SIZE = 1024;
+  char buf[BUFFER_SIZE];
+
+  if (fgets(buf, sizeof(buf), stdin) == NULL) {
+    /* Handle error */
+  }
+  // buf's first character can be '\0', or buf can be empty!
+  buf[strlen(buf) - 1] = '\0';
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: strlen with subtraction could cause out-of bounds memory access in array's own subscript expression.
+  buf[std::strlen(buf) - 1] = '\0';
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: strlen with subtraction could cause out-of bounds memory access in array's own subscript expression.
+  buf[::strlen(buf) - 1] = '\0';
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: strlen with subtraction could cause out-of bounds memory access in array's own subscript expression.
+}
+
+void func2() {
+  unsigned int BUFFER_SIZE = 1024;
+  char buf[BUFFER_SIZE];
+  char *p;
+
+  if (fgets(buf, sizeof(buf), stdin)) {
+    p = strchr(buf, '\n');
+    if (p) {
+      *p = '\0';
+    }
+  } else {
+    /* Handle error */
+  }
+
+  buf[strlen(buf)] = '\0';     // no-warning
+  buf[strlen(buf) + 1] = '\0'; // no-warning
+}