CRTP copy method warns of potential memory leak

Question

I have an object hierarchy and need to be able to clone objects from the base class. I've followed the typical CRTP pattern, except that I also want to be able to return the child class if copy is called on a child directly. To do that, I've followed the suggestion here: https://stackoverflow.com/a/30252692/1180785

It seems to work fine, but Clang warns me that I have a potential memory leak. I've reduced the code down to this MCVE:

template <typename T>
class CRTP {
protected:
    virtual CRTP<T> *internal_copy(void) const {
        return new T(static_cast<const T&>(*this));
    }

public:
    T *copy(void) const {
        return static_cast<T*>(internal_copy());
    }

    virtual ~CRTP(void) = default;
};

class Impl : public CRTP<Impl> {
};

int main(void) {
    Impl a;
    Impl *b = a.copy();
    delete b;
}

As far as I can tell, there's no possible memory leak there, but running Clang through XCode shows this:

Is there a memory leak here? If not, what's causing the false positive and how can I work around it? (I'd rather not turn off static analysis)

The program you show [doesn't call `CRTP::copy`](http://rextester.com/UBB92957) at all. I suspect the code you run may be different from one you show. — Igor Tandetnik, Dec 11 '16 at 14:16
@IgorTandetnik good point; I missed that while I was reducing it. However, the warning I posted is taken directly from the code I posted, so somehow it is indeed running CRTP::copy. That makes me think this could actually be a bug in the analyser related to virtual methods. — Dave, Dec 11 '16 at 14:19
@IgorTandetnik I have updated the code with an improved demonstration of the issue, which removes the unnecessary "virtual" on the copy and removes the array. This one does indeed call the CRTP::copy method, and the clang analysis is the same. — Dave, Dec 11 '16 at 14:25
For what it's worth, I don't see a memory leak. Not sure why the analyzer complains. Maybe it doesn't like any function returning a raw pointer to allocated memory; or maybe the template dance confuses it enough that it can't prove that the caller does free the memory. Consider returning `std::unique_ptr` in place of raw pointer. — Igor Tandetnik, Dec 11 '16 at 14:47
@IgorTandetnik even using `unique_ptr` (`std::unique_ptr copy(void) const { return std::unique_ptr(static_cast(internal_copy())); }`) produces the warning. Interestingly, I've found that it disappears if I reverse the way copy & internal_copy are linked (i.e. if all logic is in copy, with no call to internal_copy, it's fine) — Dave, Dec 11 '16 at 15:00
Why does internal_copy need to return `CRTP*` instead of just `T*`? Also, since `T` is the base of `CRTP`, why does copy need to `static_cast` it? I wonder if the analyzer is thinking that you're somehow losing information with all the changing of the pointer type. Just a wild guess... — cyberbisson, Dec 12 '16 at 16:23
@cyberbisson see the linked answer for the full context of why it needs to be that way. It's basically because it can't override an existing method to return a type which it doesn't yet know is a sub-class of the original (the base class has been removed from my reduced example here). Also `T` is not the base of `CRTP`; `CRTP` is the base of `T` (which is what makes it curiously recursive!) — Dave, Dec 12 '16 at 18:37
@Dave Oh duh, of course. I skimmed the link, but didn't fully grok the covariance issue only presenting itself when it was overridden. Ignore ignore ignore. :) — cyberbisson, Dec 12 '16 at 19:17

score 1 · Answer 1 · edited May 23 '17 at 12:33

I have found a workaround which makes the analyser happy while still allowing the use of this pattern. Simply reverse the link between copy and internal_copy:

template <typename T>
class CRTP : public Base {
protected:
    virtual CRTP<T> *internal_copy(void) const {
        return copy();
    }

public:
    T *copy(void) const {
        return new T(static_cast<const T&>(*this));
    }
};

This still works in the context of the original suggestion here, because when resolving copy inside CRTP, it will prefer CRTP's override (even though it isn't a virtual method), so there is no infinite loop.

As for why the analyser is happy with this order but unhappy with the original order, I have no idea.

score 1 · Answer 2 · answered Dec 12 '16 at 16:59

I think the analyzer is getting confused by the static_cast in your copy method. If you simply change it to dynamic_cast, it ceases to flag the line as a memory leak. This leads me to believe that it thinks you are possibly casting a base type instance (CRTP<T>) to its derived type (T), which would of course be invalid when dereferenced. You are obviously not doing this, so this may be a bug in the leak detector, or it may be onto something more subtle than what I'm thinking about right now. It may not be an error now, but if Impl becomes a more complicated type (e.g., one with multiple inheritance), it may become an issue (as will your static_cast in the copy-CTOR invocation).

This implementation (with fewer casts) had zero leaks for me as well:

template <typename T>
class CRTP {
protected:
    virtual T *internal_copy() const {
        return new T(static_cast<const T&>(*this));
    }

public:
    T *copy() const {
        return internal_copy();
    }

    virtual ~CRTP() = default;
};

See my answer to your comment for an explanation of why `internal_copy` can't return `T*` in this use-case. Interesting that `dynamic_cast` makes it happy; it isn't something I tested because I'm using `-no-rtti`, but as you say; perhaps it just thinks the static_cast might cast to the wrong type. — Dave, Dec 12 '16 at 18:43

CRTP copy method warns of potential memory leak

2 Answers2