libkmod: Simplify lookup_builtin_file()

[lucasdemarchi]

Use cleanup attribute.

[evelikov]

Tl:Dr: _cleanup_ coverage is at ~1%, with 2 instance added in ~450 patches since 2024/01/01 yet it contributes to 60% of false reports.

Longer version:
I guess we can step back and look at the bigger picture: how much do we use _cleanup_ and does it outweigh the benefits of adding analyser like scan-build (aka clang analyze)?

• looking around - we had 3 new instances over the last 1.5 years (2 of those my me 😛) and third one back in 2018. In terms of count there are only 19 instances in the whole codebase, while they could easily be 10-20x as much.

• glance at recent~ish scan-build report shows 38 issues, 23 of which from our use of _cleanup_ - the rest, at least in part, are genuine - writing to a malloc(0) block, passing NULL to nonnull` annotated API, etc

Objectively looking our _cleanup_ use is at 1%, while it contributes ~60% of noise to a scan-build run. This doesn't seems worth it.

On the subjective side - it's sad and annoying to remove a really cool feature. Personally, if scan-build properly supported the attribute, I would have used a lot more than in 2 out of 200+ patches. Alas that's not the case ...

Plus there is nothing blocking us from re-introducing it later on when scan-build gets wiser.

[lucasdemarchi]

I'm still of the opinion that scan-build needs to get wiser or we need to convince it of a proper way to understand it. Because it seems they are already even checking for improper use of the cleanup attribute. How the heck they don't understand what the attribute does?

You are creating a chicken and egg situation: it doesn't get used more because of X and X doesn't improve because it's not used enough. When something is that dumb, I prefer to simply disable it and use something smarter to cover the same thing. We can simply disable the unix.Malloc check and rely on valgrind for that.

Another alternative: try to convince the analyzer to stop bothering us with those false positives

[lucasdemarchi]

I added a commit to shut up clang-analyzer on those specific case. I didn't finish moving the assignments to the declarations because I think I didn't try hard to convince clang-analyzer to shut up. Anyway, this seems better than "let's not use this and hope some day clang-analyzer will understand a 10+ year old feature".

[lucasdemarchi]

@evelikov not sure how useful those checks for leak are with clang-analyzer. Example of one place not using the attribute:

../../../libkmod/libkmod-module.c:2047:12: warning: Potential leak of memory pointed to by 'mv' [unix.Malloc]
 2047 |                         *list = n;
      |                                 ^
../../../libkmod/libkmod-module.c:2151:12: warning: Potential leak of memory pointed to by 'mv' [unix.Malloc]
 2151 |                         *list = n;
      |                                 ^
../../../libkmod/libkmod-module.c:2261:12: warning: Potential leak of memory pointed to by 'mv' [unix.Malloc]
 2261 |                         *list = n;
      |                                 ^

Basically it doesn't seem clang-analyzer understands the ownership transfer when we append to a list.

[evelikov]

Fully agree that clang-analyze should get wiser - yet again I'm just the messenger here. The chicken and egg seems valid if clang devs see kmod (plus other users like systemd) as moderate-high interest/priority project. Hope I don't sound like a cold-hearted a** here - trying to be realistic.

Thanks for the ::suppress trick - didn't knew it existed 🙇

At a glance it seems that the allocation needs to be annotated, not the variable declaration. While moving the assignment/allocation on declaration works, we cannot reliably use it where we don't know the size early "enough" since we explicitly ban declaration after statements.

If we remove -Wdeclaration-after-statement (I'm in favour btw) this would help us move things as needed.

[lucasdemarchi]

See the last commit with the fixup where I return it to where it was. The problem is, like I said, even with those it seems like the leak check in clang-analyzer is not smart enough and has other problems. To the point I think we'd be better off turning off that specific check.

[evelikov]

Apologies - had the reply written on the day before and didn't refresh/check before posting 🙇

Looking at the 23 mem leak reports we have:

• attribute cleanup related 10
• kmod_list ownership transfer 7
• legitimate 6 - some of which in the tests

Suspect we could "fix" the kmod_list ones although it'll be alot of churn. Plus there is a 10+ year old issue about the cleanup attribute with limited activity.

So despite the legitimate catches, the noise is too much to have unix.Malloc enabled by default 😭

[evelikov]

Thumbs up for reintroducing the _cleanup_free_ instance. The clang/suppress patch can be deferred for a day when the false-positives are fewer.

[lucasdemarchi]

@evelikov I added a few more compat to attribute handling. I gave it a quick check on list of reported leaks with the clang-analyzer in my machine and I think this clears/hides all the cleanup-attribute-related ones.

[evelikov]

@evelikov I added a few more compat to attribute handling. I gave it a quick check on list of reported leaks with the clang-analyzer in my machine and I think this clears/hides all the cleanup-attribute-related ones.

There are a few more, but I wouldn't worry about handling them all atm. As long as you mind a way to appease clang (ironically it clang 14 chokes on clang::anything) - feel free to merge.

[lucasdemarchi]

Yeah, I think I will just check for running with clang_analyzer and remove the attribute check.

[lucasdemarchi]

Applied, thanks