V39/analyzer extra stuff #4730
Draft
V39/analyzer extra stuff #4730
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This used a junk wrapper struct to allocate two structs in a single malloc, which is confusing to the gcc static analyzer. Instead, just use two allocations... Nothing cleans these up, so they would still be leaked in practice, except that Cyrus never calls this function anyway.
it doesn't understand that we only won't have a *comp if there was an error already, so just check for *comp too
it doesn't understand that we only won't have a *param if there was an error already, so just check for *param too
not sure if this helps or is complete
GCC static analyser complains that the year field of due and created are uninitialized, but only when memcpying them over period.end, even though it might have just successfully memcpy'd it over period.start without complaint. Even weirder: replacing the memcpy's with assignments stops it complaining, even though it should not have made any difference. Might be a GCC bug? But I need to shut it up to get on with fixing the real complaints... gcc (Debian 12.2.0-14) 12.2.0 imap/ical_support.c: In function 'icalcomponent_get_utc_timespan': imap/ical_support.c:1223:25: error: use of uninitialized value '*(unsigned char (*)[40])((char *)&due + offsetof(struct icaltimetype, year))' [CWE-457] [-Werror=analyzer-use-of-uninitialized-value] 1223 | memcpy(&period.end, &due, sizeof(struct icaltimetype)); ... imap/ical_support.c:1251:25: error: use of uninitialized value '*(unsigned char (*)[40])((char *)&created + offsetof(struct icaltimetype, year))' [CWE-457] [-Werror=analyzer-use-of-uninitialized-value] 1251 | memcpy(&period.end, &created, sizeof(struct icaltimetype));
Not sure if this is necessary, but it shuts up the static analyzer...
what does this case really mean?
analyzer doesn't understand that msgdata is only NULL when nmsgs is zero, but if nmsgs is zero we weren't doing anything anyway. need to shut it up so i can continue...
analyzer doesn't understand that last is only null when we haven't found a list head yet... but surely we need to reset it to null when starting a new list!
assuming these only fail on out-of-memory...
free(NULL) is a no-op, but it looks like the static analyzer's free wrapper doesn't know that, and treats two calls to free(NULL) in the same function as if NULL were being freed twice, and then complains about a double-free!
ckind may be null and _jsunknown_to_vcard does not change that, but we try to deref it anyway
if no scriptid, we exited above from the parser.invalid check
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This sits on top of #4724, will need rebasing once that lands. The real contents here are the commits starting with "WIP".
This contains all the stuff where there's either a real problem that I haven't worked out how to solve properly, or where I needed to add stuff just to shut up a false positive from the static analyzer.
Even with all these commits, a few source files still don't build cleanly with
-fanalyzeyet. In particular, all the code generated by flex/bison. Also, jmap_contact.c, httpd.c, and http_jwt.testc are others I remember at the time of writing. For these files, I had to compile them individually without-fanalyzedue to things I couldn't fix nor shut up.With these commits, and a lot of CPU time and RAM, you can build all of Cyrus (with exceptions as above) under the analyzer, and successfully run the cunit and cassandane tests.