Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

donat.nagy (Donát Nagy)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 13 2018, 4:03 AM (263 w, 4 d)

Recent Activity

Tue, Sep 19

donat.nagy accepted D159519: [clang][AST][ASTImporter] improve AST comparasion on VarDecl & GotoStmt.

LGTM. I'm not very familiar with this area, but the change seems to be a very clean improvement.

Tue, Sep 19, 2:38 AM · Restricted Project, Restricted Project, Restricted Project

Mon, Sep 18

donat.nagy added a comment to D158156: [analyzer] Add C++ array delete checker.

Looks good, thanks for the improvements!

Mon, Sep 18, 8:57 AM · Restricted Project, Restricted Project

Tue, Sep 12

donat.nagy added inline comments to D158156: [analyzer] Add C++ array delete checker.
Tue, Sep 12, 3:46 AM · Restricted Project, Restricted Project
donat.nagy added inline comments to D158156: [analyzer] Add C++ array delete checker.
Tue, Sep 12, 2:12 AM · Restricted Project, Restricted Project

Thu, Sep 7

donat.nagy accepted D159479: [ASTImport]CXXBoolLiteralExpr should be handled explicitly in statement comparation.

Today we learned that truth is different from falsehood...

Thu, Sep 7, 4:18 AM · Restricted Project, Restricted Project, Restricted Project

Wed, Sep 6

donat.nagy requested changes to D158813: [analyzer] MPIChecker: MPI_Waitall should respect count arg.

I reviewed this change and collected my suggestions in comments.

Wed, Sep 6, 7:54 AM · Restricted Project, Restricted Project

Tue, Sep 5

donat.nagy added inline comments to D158156: [analyzer] Add C++ array delete checker.
Tue, Sep 5, 4:44 AM · Restricted Project, Restricted Project

Mon, Sep 4

donat.nagy added a comment to D159412: [analyzer]FieldRegion in getStaticSize should return size of pointee type.

Please add a testcase that demonstrates this issue (fails when your change in MemRegion.cpp isn't added) and shows that your commit fixes it.

Mon, Sep 4, 2:40 AM · Restricted Project, Restricted Project, Restricted Project

Sep 1 2023

donat.nagy added a comment to D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well.

As I thought more about this commit I realized that I have two vague but significant concerns. I'm sorry that I wasn't able to describe these before you started to dig into the details of the heuristics.

Sep 1 2023, 3:49 AM · Restricted Project, Restricted Project

Aug 31 2023

donat.nagy added inline comments to D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent.
Aug 31 2023, 9:46 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D159107: [analyzer] ArrayBoundCheckerV2 should disallow forming lvalues to out-of-bounds locations.

I don't think that the &arr[N] issue is too serious: we can just increment the array extent when the parent expression of the array subscript operator is the unary operator &. If the past-the-end pointer ends up dereferenced later, the current code is sufficient to report it as a bug (as the checker monitors all dereferences).

My instinct suggests that there is more behind the curtain.

As a rough solution we can simply say that this checker ignores &arr[idx] (because it's just a different way of writing arr + idx) and only checks expressions where "real" dereference happens. This way the checker would won't emit false positives on past-the-end pointers and still report every out of bound memory access (including off-by-one errors).

Aug 31 2023, 6:00 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D159107: [analyzer] ArrayBoundCheckerV2 should disallow forming lvalues to out-of-bounds locations.

I don't think that the &arr[N] issue is too serious: we can just increment the array extent when the parent expression of the array subscript operator is the unary operator &. If the past-the-end pointer ends up dereferenced later, the current code is sufficient to report it as a bug (as the checker monitors all dereferences).

Aug 31 2023, 2:33 AM · Restricted Project, Restricted Project

Aug 30 2023

donat.nagy added inline comments to D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent.
Aug 30 2023, 1:10 PM · Restricted Project, Restricted Project
donat.nagy accepted D159108: [analyzer] CStringChecker should check the first byte of the destination of strcpy, strncpy.

LGTM if the test results are good.

Aug 30 2023, 3:38 AM · Restricted Project, Restricted Project
donat.nagy accepted D159109: [analyzer] CStringChecker buffer access checks should check the first bytes.

LGTM if the test results are also good.

Aug 30 2023, 3:37 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D159107: [analyzer] ArrayBoundCheckerV2 should disallow forming lvalues to out-of-bounds locations.

Good direction of development, this will be useful for providing better bug reports (in addition to ensuring correct behavior some situations).

Aug 30 2023, 3:27 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well.

I was thinking about adding an improvement like this for the sake of consistency, but I fear that this might cause a surprising amount of false positives. (Did you test it on larger codebases?)

I'm just done backporting the first batch to our fork and scheduling a large-scale measurement.
But my gut instinct suggests there won't be too many new reports, as I believe "tainted" pointers shouldn't be really a thing, except for really exotic cases, like scanf a pointer - which is dubious at best in presence of ASLR.

As far as I understand (correct me if I'm wrong), there are situations when TaintChecker marks the memory region of e.g. a returned string as tainted to mark that the contents of the region are unreliable. (There may be other more exotic situations when even the pointer value or the extent becomes unreliable e.g. your testcases where you scanf into a pointer.)

Taint can only bind to symbols, thus a memregion per-say cannot be tainted, only the "conjured address of it" or the "symbol bound to the pointee's/referred lvalue" can be tainted.

Thanks for the clarification! As I dig deeper I see that the functions that look like "let's taint a MemRegion" are in fact only affecting SymbolicRegions (targeting the corresponding symbol). This taint may be seen by the logic that checks the taintedness of the ElementRegion -- but as it's limited to symbolic regions it probably won't appear in the underflow test [which is skipped for symbolic regions in unknown space]. (There are also some calls which taint the pointee's/referred value -- that won't affect us.)

Aug 30 2023, 3:17 AM · Restricted Project, Restricted Project
donat.nagy accepted D159106: [analyzer] ArrayBoundCheckerV2 should listen to check::Bind as well.

This seems to be a straightforward improvement over the current situation; LGTM if you test(ed) it on some real-life code (to ensure that it doesn't introduce a corner case that crashes).

Aug 30 2023, 2:11 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well.

I was thinking about adding an improvement like this for the sake of consistency, but I fear that this might cause a surprising amount of false positives. (Did you test it on larger codebases?)

Aug 30 2023, 1:50 AM · Restricted Project, Restricted Project

Aug 28 2023

donat.nagy added a comment to D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent.

Yes, adding tests that demonstrate the current behavior is a good idea.

Aug 28 2023, 8:17 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent.

Oh, so we would canonicalize towards a signed extent type. I see. I think I'm okay with that.
However, I think this needs a little bit of polishing and testing when the region does not point to the beginning of an array or object, but rather inside an object, or like this:

int nums[] = {1,2,3};
char *p = (char*)&nums[1];

clang_analyzer_dumpExtent(p);
clang_analyzer_dumpElementCount(p);
++p;
clang_analyzer_dumpExtent(p);
clang_analyzer_dumpElementCount(p);
++p;
int *q = (int*)p;
clang_analyzer_dumpExtent(q);
clang_analyzer_dumpElementCount(q);
Aug 28 2023, 7:21 AM · Restricted Project, Restricted Project
donat.nagy committed rG8a5cfdf7851d: [analyzer][NFC] Remove useless class BuiltinBug (authored by donat.nagy).
[analyzer][NFC] Remove useless class BuiltinBug
Aug 28 2023, 6:26 AM · Restricted Project, Restricted Project
donat.nagy closed D158855: [analyzer][NFC] Remove useless class BuiltinBug.
Aug 28 2023, 6:26 AM · Restricted Project, Restricted Project
donat.nagy accepted D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent.

Thanks for the updates! I didn't spot any important issue, so I'm accepting this commit, but let's wait for the opinion of @steakhal before merging this.

Aug 28 2023, 4:30 AM · Restricted Project, Restricted Project
donat.nagy accepted D158968: [analyzer] Fix assertion on casting SVal to NonLoc inside the IteratorRange checker.

LGTM. I'm not familiar with the Iterator checker family, but this is a very straightforward change.

Aug 28 2023, 1:29 AM · Restricted Project, Restricted Project
donat.nagy added inline comments to D158855: [analyzer][NFC] Remove useless class BuiltinBug.
Aug 28 2023, 1:25 AM · Restricted Project, Restricted Project

Aug 25 2023

donat.nagy added inline comments to D158855: [analyzer][NFC] Remove useless class BuiltinBug.
Aug 25 2023, 8:46 AM · Restricted Project, Restricted Project
donat.nagy requested review of D158855: [analyzer][NFC] Remove useless class BuiltinBug.
Aug 25 2023, 8:39 AM · Restricted Project, Restricted Project

Aug 24 2023

donat.nagy added a comment to D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent.

Thanks for creating this commit, it's a useful improvement!

Aug 24 2023, 7:10 AM · Restricted Project, Restricted Project

Aug 23 2023

donat.nagy added a comment to D158499: [analyzer] Compute FAM dynamic size.

One of the observable issue with inconsistent size type is

void clang_analyzer_eval(int);

typedef unsigned long long size_t;
void *malloc(unsigned long size);
void free(void *);

void symbolic_longlong_and_int0(long long len) {
  char *a = malloc(5);
  (void)a[len + 1]; // no-warning
  // len: [-1,3]
  clang_analyzer_eval(-1 <= len && len <= 3); // expected-warning {{TRUE}}
  clang_analyzer_eval(0 <= len);              // expected-warning {{UNKNOWN}}
  clang_analyzer_eval(len <= 2);              // expected-warning {{UNKNOWN}}
  free(a);
}

which is extracted from clang/test/Analysis/array-bound-v2-constraint-check.c,
with DynamicMemoryModeling turned on,
the second warning does not hold anymore: clang_analyzer_eval(0 <= len);
will be reported as TRUE which is not expected.

DynamicMemoryModeling will record the extent of allocated memory as 5ULL,
ArrayBoundV2 will do len + 1 < 5ULL assumption, simplified to len < 4ULL,
which casts len to unsigned, dropping -1, similar to

void clang_analyzer_eval(int);

void test(int len) {
  if (len >= -1 && len <= 4U) { // len is promoted into unsigned, thus can never be negative
      clang_analyzer_eval(0 <= len);              // TRUE
  }
}
Aug 23 2023, 7:36 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker.

The change looks promising, I only have minor remarks.

Aug 23 2023, 7:09 AM · Restricted Project, Restricted Project

Aug 21 2023

donat.nagy added a comment to D158156: [analyzer] Add C++ array delete checker.

One more thing to mention. Its usually illadvised to rename files or do changes that would seriously impact git blame, unless we have a really good reason doing so.
Aesthetics usually don't count one, especially if the name is not user-facing. However, maintainability is another axis, thus as it's always, not black and white.

Aug 21 2023, 10:47 AM · Restricted Project, Restricted Project
donat.nagy committed rG3e014038b373: [analyzer] Improve underflow handling in ArrayBoundV2 (authored by donat.nagy).
[analyzer] Improve underflow handling in ArrayBoundV2
Aug 21 2023, 8:19 AM · Restricted Project, Restricted Project
donat.nagy closed D157104: [analyzer] Improve underflow handling in ArrayBoundV2.
Aug 21 2023, 8:19 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D157104: [analyzer] Improve underflow handling in ArrayBoundV2.

Yup, there was a superfluous line break that was corrected by git clang-format; thanks for bringing it to my attention. As this is a very trivial change, I'll merge the fixed commit directly, without uploading it into Phabricator.

Aug 21 2023, 8:13 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D158156: [analyzer] Add C++ array delete checker.

By the way, what would you think about putting the implementation of the two checkers (DeleteWithNonVirtualDtor, ArrayDelete) into two separate checker classes and placing the shared code into a common base class of these classes? I think this could be a significant simplification: we could avoid the manual management of the two checker names and bug types and register two regular checker objects instead of the one dummy prerequisite checker + two "fake" checker registrations that just toggle boolean flags.

Aug 21 2023, 2:39 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D157104: [analyzer] Improve underflow handling in ArrayBoundV2.

A (somewhat delayed) ping to @steakhal because you asked me to tell you when I have the results. (I'm writing this only because I don't know whether you have seen the table that I uploaded on Friday.)

Aug 21 2023, 2:08 AM · Restricted Project, Restricted Project

Aug 18 2023

donat.nagy added a comment to D157104: [analyzer] Improve underflow handling in ArrayBoundV2.

The results on open-source projects are depressing, but acceptable. This checker is looking for a serious defect, so it doesn't find any true positives on stable releases; however it produces a steady trickle of false positives because the Clang SA engine regularly misinterprets complicated code. As the only effect of this patch is that it allows this checker to analyze more situations, it introduces no true positives and a manageable amount of false positives (on average ~1/project).

Aug 18 2023, 6:34 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D158156: [analyzer] Add C++ array delete checker.

This checker is a straightforward, clean implementation of the SEI-CERT rule EXP51-CPP, it'll be a good addition to Static Analyzer. I'm satisfied with the implementation, although there might be others who have a more refined taste in C++ coding style. The only issue that I found is the misleading name of the "common ancestor" hidden checker that I mentioned in the inline comment.

Aug 18 2023, 5:26 AM · Restricted Project, Restricted Project
donat.nagy committed rG25b9696b61e5: [analyzer] Upstream BitwiseShiftChecker (authored by donat.nagy).
[analyzer] Upstream BitwiseShiftChecker
Aug 18 2023, 1:48 AM · Restricted Project, Restricted Project
donat.nagy closed D156312: [analyzer] Upstream BitwiseShiftChecker.
Aug 18 2023, 1:48 AM · Restricted Project, Restricted Project

Aug 16 2023

donat.nagy updated the diff for D156312: [analyzer] Upstream BitwiseShiftChecker.

A few minutes ago I accidentally re-uploaded the previous version of the patch (diffs #550318 and #550703 are identical); now I'm fixing this by uploading the actually updated variant.

Aug 16 2023, 5:07 AM · Restricted Project, Restricted Project
donat.nagy added inline comments to D156312: [analyzer] Upstream BitwiseShiftChecker.
Aug 16 2023, 5:03 AM · Restricted Project, Restricted Project
donat.nagy updated the diff for D156312: [analyzer] Upstream BitwiseShiftChecker.

Updated the release notes. This could be the final version of this commit if you agree that it's good enough.

Aug 16 2023, 5:01 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D156312: [analyzer] Upstream BitwiseShiftChecker.

I don't think we should do anything about it unless it's frequent enough.
Try to come up with a heuristic to be able to measure how often this happens, if you really care.
Once you have a semi-working heuristic for determining if that bugpath suffers from this, you can as well use it for marking the bugreport invalid if that's the case to lean towards suppressing those FPs.

Minor clarification: these are not FPs, these are true positives with a bad error message. I was annoyed when I found this surprising bug on the almost-ready checker that I was working on; but I wouldn't say that this is an especially severe issue.

Aug 16 2023, 1:59 AM · Restricted Project, Restricted Project

Aug 15 2023

donat.nagy added a comment to D156312: [analyzer] Upstream BitwiseShiftChecker.

(The release notes entry is still missing, I'll try to add it tomorrow.)

Aug 15 2023, 7:10 AM · Restricted Project, Restricted Project
donat.nagy updated the diff for D156312: [analyzer] Upstream BitwiseShiftChecker.

I verified that the checker handles the examples in the documentation correctly (and added them to the test suite). However, as I was tweaking the examples in the documentation, I accidentally found a situation where the checker produces a very surprising and arguably incorrect error message.

Aug 15 2023, 7:09 AM · Restricted Project, Restricted Project

Aug 14 2023

donat.nagy updated the diff for D156312: [analyzer] Upstream BitwiseShiftChecker.
Aug 14 2023, 8:55 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D156312: [analyzer] Upstream BitwiseShiftChecker.

I handled the inline comments, I'll check the example code and edit the release notes tomorrow.

Aug 14 2023, 8:55 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D157104: [analyzer] Improve underflow handling in ArrayBoundV2.

I didn't upload the test results yet because right now there is some incompatibility between our local fork and the upstream. There is ongoing work to fix this (by rebasing our fork), but until it's done I can't use our automated tools to run the analysis with revisions that are close to the upstream main branch.

Aug 14 2023, 4:59 AM · Restricted Project, Restricted Project
donat.nagy updated the diff for D157104: [analyzer] Improve underflow handling in ArrayBoundV2.
Aug 14 2023, 4:53 AM · Restricted Project, Restricted Project

Aug 10 2023

donat.nagy updated the diff for D156312: [analyzer] Upstream BitwiseShiftChecker.

Uploading a new version based on the reviews.

Aug 10 2023, 10:16 AM · Restricted Project, Restricted Project
donat.nagy added inline comments to D156312: [analyzer] Upstream BitwiseShiftChecker.
Aug 10 2023, 10:15 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D156312: [analyzer] Upstream BitwiseShiftChecker.

Thanks for the suggestions, I answered those where I had something to say and I'll implement the rest of them.

Aug 10 2023, 4:57 AM · Restricted Project, Restricted Project

Aug 9 2023

donat.nagy added inline comments to D156312: [analyzer] Upstream BitwiseShiftChecker.
Aug 9 2023, 8:08 AM · Restricted Project, Restricted Project
donat.nagy updated the diff for D156312: [analyzer] Upstream BitwiseShiftChecker.
Aug 9 2023, 8:03 AM · Restricted Project, Restricted Project
donat.nagy added inline comments to D156312: [analyzer] Upstream BitwiseShiftChecker.
Aug 9 2023, 6:28 AM · Restricted Project, Restricted Project
donat.nagy planned changes to D156312: [analyzer] Upstream BitwiseShiftChecker.

I'll soon upload a refactored version.

Aug 9 2023, 4:39 AM · Restricted Project, Restricted Project

Aug 8 2023

donat.nagy added inline comments to D156787: [analyzer][attr] Add docs to ownership_* attributes.
Aug 8 2023, 4:07 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha..

Why is this checker placed in the "unix" group (instead of e.g. "core")? As far as I understand it can check some POSIX-specific functions with a flag (that's off by default) but the core functionality checks platform-independent standard library functions.

Aug 8 2023, 12:49 AM · Restricted Project, Restricted Project

Aug 4 2023

donat.nagy added reviewers for D157104: [analyzer] Improve underflow handling in ArrayBoundV2: dkrupp, steakhal, Szelethus, gamesh411.

I'll try to upload results on open source projects, probably on Tuesday (I'm on vacation on Monday).

Aug 4 2023, 8:47 AM · Restricted Project, Restricted Project
donat.nagy abandoned D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2.

I'm abandoning this commit because it amalgamates several unrelated changes and I think it'd be better to handle them separately:

Aug 4 2023, 8:41 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D157104: [analyzer] Improve underflow handling in ArrayBoundV2.

Note: this commit is split off from the larger commit https://reviews.llvm.org/D150446, which combined this simple improvement with other, more complex changes that had problematic side-effects.

Aug 4 2023, 8:07 AM · Restricted Project, Restricted Project
donat.nagy requested review of D157104: [analyzer] Improve underflow handling in ArrayBoundV2.
Aug 4 2023, 8:05 AM · Restricted Project, Restricted Project

Aug 3 2023

donat.nagy added a comment to D156312: [analyzer] Upstream BitwiseShiftChecker.

@steakhal Thanks for the review!

Aug 3 2023, 6:05 AM · Restricted Project, Restricted Project
donat.nagy added inline comments to D153954: [clang][analyzer] Fix empty enum handling in EnumCastOutOfRange checker.
Aug 3 2023, 5:54 AM · Restricted Project, Restricted Project
donat.nagy accepted D153954: [clang][analyzer] Fix empty enum handling in EnumCastOutOfRange checker.

LGTM, with a little bikeshedding ;-)

Aug 3 2023, 5:13 AM · Restricted Project, Restricted Project

Aug 2 2023

donat.nagy accepted D155715: [clang][analyzer] Improve StdCLibraryFunctions socket send/recv functions..

LGTM, I don't have anything significant to add.

Aug 2 2023, 2:48 AM · Restricted Project, Restricted Project

Aug 1 2023

donat.nagy added inline comments to D156312: [analyzer] Upstream BitwiseShiftChecker.
Aug 1 2023, 1:32 AM · Restricted Project, Restricted Project

Jul 27 2023

donat.nagy added a comment to D156312: [analyzer] Upstream BitwiseShiftChecker.

(4) UBOR exhibits buggy behavior in code that involves cast expressions

Hmm, what makes your check more resilient to our overall type-incorrectness?

Jul 27 2023, 7:17 AM · Restricted Project, Restricted Project

Jul 26 2023

donat.nagy updated the diff for D156312: [analyzer] Upstream BitwiseShiftChecker.

(Fix incorrect filename mark in the license header.)

Jul 26 2023, 5:11 AM · Restricted Project, Restricted Project
donat.nagy added reviewers for D156312: [analyzer] Upstream BitwiseShiftChecker: gamesh411, Szelethus, balazske, steakhal, dkrupp.
Jul 26 2023, 5:06 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D156312: [analyzer] Upstream BitwiseShiftChecker.

I tested this commit on several open source projects (in the default mode Pedantic=false), the following table summarizes the results:

Jul 26 2023, 5:05 AM · Restricted Project, Restricted Project
donat.nagy requested review of D156312: [analyzer] Upstream BitwiseShiftChecker.
Jul 26 2023, 4:22 AM · Restricted Project, Restricted Project

Jul 25 2023

donat.nagy accepted D145229: [analyzer] Improve the documentation of the alpha.security.taint.TaintPropagation checker.
Jul 25 2023, 1:15 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D145229: [analyzer] Improve the documentation of the alpha.security.taint.TaintPropagation checker.

Mostly LGTM, but I'd like to ask you to ensure consistent line wrapping (to e.g. 72 or 80 characters) in the free-flowing text that was added or rewritten by this commit. (I know that this is a minor question because the linebreaks won't be visible in the final output, but the raw markdown is itself a human-readable format and we should keep it reasonably clean.)

Jul 25 2023, 12:37 AM · Restricted Project, Restricted Project

Jul 13 2023

donat.nagy added a comment to D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker..

(Just added some side remarks about string handling annoyances.)

Jul 13 2023, 1:44 AM · Restricted Project, Restricted Project

Jul 10 2023

donat.nagy added a comment to D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker..

It would be more simple to handle the standard streams in StreamChecker only. [...]

Jul 10 2023, 12:07 PM · Restricted Project, Restricted Project
donat.nagy accepted D153776: [clang][analyzer] Display notes in StdLibraryFunctionsChecker only if interesting.

LGTM, thanks!

Jul 10 2023, 2:11 AM · Restricted Project, Restricted Project
donat.nagy accepted D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker..

Ok, then I think you can finally merge this commit and its two predecessors. I'm happy to see that this improvement is complete.

Jul 10 2023, 2:10 AM · Restricted Project, Restricted Project

Jul 7 2023

donat.nagy added a comment to D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker..

I looked through most of the open source results, and they look promising.

Jul 7 2023, 6:09 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D154509: [clang][analyzer] StdLibraryFunctionsChecker: Allow NULL buffer in `fread` and `fwrite` if size is zero..

By the way here is a concrete example where this patch is needed to squash a false positive: https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclf_notetag_interesting_test_3&is-unique=on&report-hash=49939ae1896dff1ad782092b8534bd68&report-filepath=%2arelcache.c&report-id=1942889

Jul 7 2023, 5:54 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D153954: [clang][analyzer] Fix empty enum handling in EnumCastOutOfRange checker.

[...] I'd not recommend moving the actual implementation anywhere.

Jul 7 2023, 5:19 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D153954: [clang][analyzer] Fix empty enum handling in EnumCastOutOfRange checker.

I think the old "report when the value stored in an enum type is not equal to one of the enumerators" behavior would be useful as an opt-in checker that could be adopted by some projects; while the new "relaxed" version is too bland to be really useful. I'd suggest keeping the old behavior in the general case, eliminating the "obvious" false positives like std::byte (don't report types that have no enumerators) and moving this checker towards the optin group.

Jul 7 2023, 3:04 AM · Restricted Project, Restricted Project
donat.nagy accepted D154509: [clang][analyzer] StdLibraryFunctionsChecker: Allow NULL buffer in `fread` and `fwrite` if size is zero..

LGTM.

Jul 7 2023, 1:58 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker.

The commit looks good in general, I have a few minor suggestions and a serious question about the state transition bureaucracy.

Jul 7 2023, 1:40 AM · Restricted Project, Restricted Project

Jul 5 2023

donat.nagy added a comment to D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker..

These code changes seem to be promising. Please upload results on the open source projects (like the ones that you uploaded previously), and I hope that after verifying those I we can finally merge this set of commits.

Jul 5 2023, 1:55 AM · Restricted Project, Restricted Project

Jul 4 2023

donat.nagy added a comment to D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer.

I don't have concrete remarks, but I would like to note that I'm happy to see this cleanup :)

Jul 4 2023, 5:04 AM · Restricted Project, Restricted Project

Jul 3 2023

donat.nagy added a comment to D153776: [clang][analyzer] Display notes in StdLibraryFunctionsChecker only if interesting.

The success/failure note tags are not added to all functions, dup is one of these, this means at dup no note tag is shown. A next patch can be made to add these messages to all functions. The other places look good, but CodeChecker is a bit tricky, you must select *_stdclf_notetag_interesting_test_2 at the small arrow after the "found in:" text (upper right corner). The link is good but not that instance of the bug is displayed because only the note tags are different.

Jul 3 2023, 2:56 AM · Restricted Project, Restricted Project

Jun 30 2023

donat.nagy added a comment to D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker..

See results and their discussion on the review of the tightly connected follow-up commit D153776.

Jun 30 2023, 8:46 AM · Restricted Project, Restricted Project
donat.nagy requested changes to D153776: [clang][analyzer] Display notes in StdLibraryFunctionsChecker only if interesting.

Thanks for attaching the test results! Unfortunately it seems that it was important to look at them, because I noticed that in the posgresql codebase there are many bug reports on calls like func1(func2(...), ...) where the checker does not print the expected "Assuming that 'func2' fails" note: fdopen/dup, fstat/fileno, isatty/fileno, another isatty/fileno, a third isatty/fileno, a second dup/fileno (this is not an exhaustive list, but I think I listed the majority of them).

Jun 30 2023, 8:44 AM · Restricted Project, Restricted Project
donat.nagy committed rG1d75b18843fb: [analyzer][NFC] Fix dangling StringRef in barely used code (authored by donat.nagy).
[analyzer][NFC] Fix dangling StringRef in barely used code
Jun 30 2023, 8:18 AM · Restricted Project, Restricted Project
donat.nagy closed D153889: [analyzer][NFC] Fix dangling StringRef in barely used code.
Jun 30 2023, 8:18 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D153674: [dataflow] Disallow implicit copy of Environment, use fork() instead.

When I tried to compile with gcc 7.5 after your fix commit (2f7d30dee826), I got another very similar compilation error from a different location and I pushed commit cec30e2b190b to fix that. After this second correction I can successfully run ninja check-clang-analysis with gcc7.5.

Jun 30 2023, 7:37 AM · Restricted Project, Restricted Project
donat.nagy committed rGcec30e2b190b: [dataflow] Fix complie on gcc7 part 2 (authored by donat.nagy).
[dataflow] Fix complie on gcc7 part 2
Jun 30 2023, 7:32 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D153674: [dataflow] Disallow implicit copy of Environment, use fork() instead.

Thank you for the fix :) !

Jun 30 2023, 1:33 AM · Restricted Project, Restricted Project

Jun 29 2023

donat.nagy accepted D153776: [clang][analyzer] Display notes in StdLibraryFunctionsChecker only if interesting.

LGTM if you rebase these changes onto the most recent version of D153612. Thanks for adding this interestingness check!

Jun 29 2023, 8:14 AM · Restricted Project, Restricted Project
donat.nagy accepted D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker..

LGTM, but please wait until you can merge this together with D153776.

Jun 29 2023, 8:09 AM · Restricted Project, Restricted Project
donat.nagy added a comment to D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker..

Thanks for the update! I have two minor remarks (string handling is complicated in C++...) but the code looks good otherwise.

Jun 29 2023, 6:27 AM · Restricted Project, Restricted Project
donat.nagy accepted D153363: [clang][analyzer] No end-of-file when seek to file begin..

LGTM.

Jun 29 2023, 6:10 AM · Restricted Project, Restricted Project