bugprone-sizeof-expression new false positives in pre20240924 clang-tidy, not reported by pre20240917

[douzzer]

We're seeing a slew of new false positives of this form:

  CC       wolfcrypt/src/src_libwolfssl_la-dilithium.lo
/tmp/tmp.4346_11739/wolfssl_test_workdir.26775/wolfssl/wolfcrypt/src/dilithium.c:5534:21: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic; this scaled value will be scaled again by the '+' operator [bugprone-sizeof-expression]
5534 |             s2 = s1 + params->s1Sz / sizeof(*s1);
|                     ^                ~~~~~~~~~~~
/tmp/tmp.4346_11739/wolfssl_test_workdir.26775/wolfssl/wolfcrypt/src/dilithium.c:5534:21: note: '+' in pointer arithmetic internally scales with 'sizeof(sword32)' == 4

These weren't reported by pre20240917.

A workaround with pre20240924 is to parenthesize thusly:

            s2 = s1 + (params->s1Sz / sizeof(*s1));

These parentheses should not be necessary -- the / has higher precedence than the + and the incumbent code pattern is quite common.

[nicovank]

#106061, CC @whisperity FYI

[whisperity]

Hah, the parentheses are a very good idea to silence these, and to be fair, I don't remember us actually planning that some parens here and there will silence the warning!

Deciding whether something is really a false positive is always a hard thing to do. Unfortunately, the code above, at least from what is visible in the report, is still suspicious, as per SEI-CERT ARR39-C. "Do not add or subtract a scaled integer to a pointer", which the improvement landed in #106061 had as a goal to match. Now, a code or a project can always decide that they want one or more rules to not apply to them.

The underlying issue here is that the connection that s1Sz expresses the size of (some buffer?) in bytes is actually connected to *s1 only exists in the minds of the programmer. If we want to write better code that expresses invariants and design decisions cleanly, this is a design that has more downsides than expressing "count of decltype(*s1) elements".

[douzzer]

Thanks @whisperity for the CERT link. Good resource.

FWIW, ARR39-C doesn't seem to anticipate or address divisive scaling in this construct, and is tolerant of division in a related construct (arr[sizeof(arr)/sizeof(arr[0])]), typo in original). Here, the source code under test has an internal record of an object size in bytes, and is converting that quantity by division, to allow correct use as an operand in pointer arithmetic.

This is a common code pattern and not inherently dangerous. It would be a shame to see projects disable bugprone-sizeof-expression globally just to avoid noise around this. After all, it helpfully warns on various actually-dangerous code patterns!

For our part we've just patched out the problematic code, allowing us to keep it globally active:

--- ./clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp.dist	2024-09-24 00:16:41.000000000 -0500
+++ ./clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp	2024-09-30 14:20:01.782122506 -0500
@@ -307,7 +307,7 @@ void SizeofExpressionCheck::registerMatc
                  offsetOfExpr()))
           .bind("sizeof-in-ptr-arithmetic-scale-expr");
   const auto PtrArithmeticIntegerScaleExpr = binaryOperator(
-      hasAnyOperatorName("*", "/"),
+      hasOperatorName("*"),
       // sizeof(...) * sizeof(...) and sizeof(...) / sizeof(...) is handled
       // by this check on another path.
       hasOperands(expr(hasType(isInteger()), unless(SizeofLikeScaleExpr)),