WIP: some coverity & compiler warning fixes#4086
Conversation
| /* remove existing entry */ | ||
| if (file) { | ||
| attr_cache_remove(cache, file); | ||
| git_attr_file__free(file); /* offset incref from lookup */ |
There was a problem hiding this comment.
This is a line I'm always stumbling upon every few weeks and I'm still not really sure what is the right thing to do here. As-is, this creates a memory leak with attr files, so this is not the correct thing to do. Locally, I'm running with an assert here that file still has a positive reference-count, and until now it did not trigger. And I think this is also expected, as the comment also states that this is to offset the lookup-incref.
So I'm not really sure what to do here. I'd guess this is simply a false positive of Coverity.
There was a problem hiding this comment.
Hmm. Yeah, you're right, this is not correct. But I think Coverity is correct here and there's a code path that will double free file, when attr_cache_lookup_entry (in attr_cache_remove) fails... I'll look at this more carefully when I have a few minutes.
There was a problem hiding this comment.
Might actually be, yeah. I think I came up with similar reasoning once when I touched the threading issues with attr-file caches in the threadmania-tests. But I discarded the complete branch as it was leading nowhere. Might want to revisit this again soonish.
There was a problem hiding this comment.
Yeah, I updated this - when attr_cache_lookup_entry fails to find the entry, it frees the given entry. (Which would lead to the double-free that coverity complains about.)
When we fail to load submodules, don't free the list; it is later freed unconditionally.
If `attr_cache_lookup_entry` fails to find the given file, make sure that we do not try to free the given file.
e0fff2f to
7f66a70
Compare
| @@ -188,8 +188,7 @@ static int load_submodule_names(git_strmap *out, git_config *cfg) | |||
| git_buf_put(&buf, fdot + 1, ldot - fdot - 1); | |||
| git_strmap_insert(out, entry->value, git_buf_detach(&buf), rval); | |||
| if (rval < 0) { | |||
There was a problem hiding this comment.
Not related to your change, which looks good to me, but is there any reason why git_strmap_insert is implemented as a macro? rval being used as a return value when it is at first sight passed in as an int only looks really horrible.
I'd at a minimum like to change it so the macro expects a pointer to an int so that the call here at least behaves somewhat like a normal function call.
There was a problem hiding this comment.
I share your dismay and would be happy to see anything that makes things more readable and less magical.
|
Looks good to me 👍 |
No description provided.