[clang-tidy] Create check for "MEM56-CPP. Do not store an already-owned pointer value in an unrelated smart pointer"

[PiotrZSL]

Background: https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM56-CPP.+Do+not+store+an+already-owned+pointer+value+in+an+unrelated+smart+pointer

   A& getA();
   void foo()
   {
        std::shared_ptr<A> a(&getA());
   }

AC:

• should support unique_ptr & shared_ptr
• should flag any pointers being passed to smart pointer constructor or reset method
• should ignore when result of new operator, release method call is passed to smart_ptr
• should ignore smart_ptr with custom deleter

Proposed name: bugprone-smart-ptr-initialization, or something else like: suspicious, or something that would point out that object is stolen.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Piotr Zegar (PiotrZSL)

Background: https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM56-CPP.+Do+not+store+an+already-owned+pointer+value+in+an+unrelated+smart+pointer ``` A& getA(); void foo() { std::shared_ptr<A> a(&getA()); } ``` AC: * should support unique_ptr & shared_ptr * should flag any pointers being passed to smart pointer constructor or reset method * should ignore when result of new operator, release method call is passed to smart_ptr * should ignore smart_ptr with custom deleter Proposed name: bugprone-smart-ptr-initialization, or something else like: suspicious, or something that would point out that object is stolen.

[carlosgalvezp]

This is a CERT rule, shouldn't it belong in the cert module?

[PiotrZSL]

Personally I think that in cert there should be only an alias.

[carlosgalvezp]

Why?

The problem I see with aliases is that check developers do not immediately see that the check is used for a coding guideline. Thus the check can deviate from what the coding guideline actually requires, potentially making it less strict.

If this is the pattern we want to have (not have any check implemented as part of a coding guideline, but as alias instead) then I believe there needs to be some proper RFC about it, and document it.

[PiotrZSL]

Problem I see is that some rules are part of multiple codding guidelines. And then when enabling/disabling checks user need to look into documentation to find out what check actually does, because name of the check "cert-mem56-cpp" tells nothing. We already got lot of checks in bugprone namespace that got cert aliases. Personally I do not care if this will be cert check with alias in bugprone or bugprone check with alias in cert.

I run into this problem in production code, and then I started searching internet and found that it's mention in cert.

[carlosgalvezp]

Fair enough, I agree the naming "cert-mem56-cpp" is quite bad. It's unfortunate this convention was chosen, otherwise it should have been like the checks in other guidelines.

In principle I like the idea of aliasing from core check to guideline check. That's what they do for example in Helix QAC (in the link you posted), they have 3 different checks/diagnostics and they cherry-pick those to fulfill the requirements of the CERT check. But those are very granular and cannot be extended with new functionality, which makes sure the functionality of the CERT check doesn't change when one of the sub-checks changes slightly.

Anyhow, I don't think we consider clang-tidy to be a professional tool for complying with guidelines and so perhaps this level of assurance is out of scope. In that case I'd be OK with your original proposal, i.e. have the check in bugprone and alias to cert.