Page MenuHomePhabricator

[Analyzer][VLASizeChecker] Check VLA size in typedef and sizeof.
ClosedPublic

Authored by balazske on Apr 29 2020, 1:47 AM.

Details

Summary

The check of VLA size was done previously for variable declarations
(of VLA type) only. Now it is done for typedef (and type-alias)
and sizeof expressions with VLA too.

Diff Detail

Event Timeline

balazske created this revision.Apr 29 2020, 1:47 AM

Overall the changes look good to me here. I had a small nit inline. But I wonder if we actually should add more code in the analyzer core to better model the sizes of memory regions corresponding to the VLAs.

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
576

At first, it might be confusing why do we need to handle typedefs at all. I think it might worth adding a comment about VLAs.

We check now whether the argument of typedef and sizeof is an incorrect VLA, but what other examples are we potentially forgetting? The warning message states that "Declared variable-length array (VLA) has negative size", but we are not actually looking for declarations, but try to guess which statements may be declarations. Did we cover all realistic cases?

Overall the changes look good to me here. I had a small nit inline. But I wonder if we actually should add more code in the analyzer core to better model the sizes of memory regions corresponding to the VLAs.

I think this is that code :) I think the existing regions/values we have can explain VLAs well, don't you think?

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
75–79

This, or at least parts of this comment should be placed for the function declaration rather :) Also, you explain the "how", but not the "why". You remove the VLALast argument in the followup patch, why do we add it here?

190–191

Should this ever happen? We seem to assert in ExprEngine.

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
576

Definitely. If there is a standard out there, it would be great to quote it. Also, it might be worth going on a bit of a tangent:

Although we're mostly looking for variable declarations here, the size of variable-length arrays are evaluated at the typedef, as specified by §x.x.x.x.

Also, whats up with using? VLAs can occur in C++ as well, can they not?

balazske marked 4 inline comments as done.May 7 2020, 6:16 AM

I do not know what other than typedef or sizeof (and variable declaration) can contain VLA. To my knowledge VLA is not normally supported in C++ and should not be used anyway (there are better ways for doing it) so some funny C++ things with VLA may not work. Anyway the patch is about "Check VLA size in typedef and sizeof", later other things can be added if required. The CERT rule ARR32-C (this is to be supported by the change) mentions only typedef and sizeof.

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
75–79

It explains "how" because it is a comment inside the function. I can add comment to the function too. VLALast is used in this change, the next change contains additional refactoring that makes it unneeded, because the evaluation of size expressions (that uses VLA and VLALast) is moved into this function.

190–191

This was part of the old code, it may be wrong but not related to this change.

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
576

There is TypedefNameDecl used and not TypedefDecl, so this should work for using too. (But main intent is to work on C source code, as VLA is not C++ friendly.) I found the typedef in a C standard draft at 6.7.7 (that tells about VLA size). The size expression must be evaluated when the typedef is encountered.

balazske updated this revision to Diff 262661.May 7 2020, 8:13 AM
balazske marked 2 inline comments as done.

Added better comments.

I'd split this patch into two as well.

  1. [NFC] Refactoring parts
  2. The actual extra additions about sizeof and typedef.

WDYT?

Other than that looks good.

balazske marked 3 inline comments as done.May 7 2020, 8:22 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
577

Should we not do something else with the VLA size expressions (not only call the checker callbacks) because they should be evaluated, are these handled in some other way automatically? (The CFG should contain these expressions already so these should be evaluated by the engine, and this place is only to make the checkers aware of typedef statements.)

This change is relatively small and the refactoring like part (introduction of checkVLA if I think correct?) is connected to its use. The other change is add of a new function (callback). This is probably small enough to go into one change and we can see why the new function checkVLA is needed. So my vote is to not split this change.

Szelethus accepted this revision.May 13 2020, 7:24 AM

I'd split this patch into two as well.

  1. [NFC] Refactoring parts
  2. The actual extra additions about sizeof and typedef.

WDYT?

Other than that looks good.

I wouldn't change this now, but for the future, changes like splitting functions into checkVLA and checkVLASize should deserve their own revision, because the actual new logic would only take a few lines, and a few minutes to review. The biggest pain point is that both this, and the followup patch change the same code and it makes it difficult to know whats the new functionality and what is just reorganization.

This revision is now accepted and ready to land.May 13 2020, 7:24 AM
martong accepted this revision.May 13 2020, 9:51 AM

This change is relatively small and the refactoring like part (introduction of checkVLA if I think correct?) is connected to its use. The other change is add of a new function (callback). This is probably small enough to go into one change and we can see why the new function checkVLA is needed. So my vote is to not split this change.

Okay, you convinced me. LGTM!

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
577

Yeah, I agree with @balazske. This could be used for anything else in the future, not just for VLA. It is more generic than that.

This revision was automatically updated to reflect the committed changes.