getting a segfault when I try to deep copy `unique_ptr`s

Question

Why can't I return a unique_ptr from a clone function? I thought I could do this.

I have a base class for different mathematical functions called transform. I have a container of pointers to this type because I am using polymorphism. For example, all these derived classes have a different implementation of log_jacobian that is useful for statistical algorithms.

I am using unique_ptrs to this transform class, and so I have made a (pure virtual) clone function that makes new unique_ptr pointing to a deep copy of the same mathematical transform object. This new object is of the same type that derives from transform<float_t>, but it's a separate because you can't have two unique_ptrs pointing to the same thing.

template<typename float_t>
class transform{
...
virtual std::unique_ptr<transform<float_t>> clone() const = 0;
...
};

My transform_container class holds a few of these at a time. After all, most statistical models have more than one parameter.

template<typename float_t, size_t numelem>
class transform_container{
private:

    using array_ptrs = std::array<std::unique_ptr<transform<float_t>>, numelem>;
    array_ptrs m_ts;
    unsigned m_add_idx;
...
    auto get_transforms() const -> array_ptrs;
};

I'm not sure why the deep copy function get_transforms isn't working, though. It is used to make copies, and to access individual transformations from the container. I get a segfault when I run some tests. If I run it in gdb, it explicitly tells me the line with a comment after it is bad.

template<typename float_t, size_t numelem>
auto transform_container<float_t,numelem>::get_transforms() const -> array_ptrs
{
    array_ptrs deep_cpy;
    for(size_t i = 0; i < numelem; ++i){
        deep_cpy[i] = m_ts[i]->clone(); // this line
    }
    return deep_cpy;
}

I've also tried std::moveing it into deep_cpy[i] and using unique_ptr::reset, but to no avail.

Edit:

here are some other relevant methods: a method that adds transforms to transform_container, and the factory method for the individual transform:

template<typename float_t>
std::unique_ptr<transform<float_t> > transform<float_t>::create(trans_type tt)
{
    if(tt == trans_type::TT_null){
        
        return std::unique_ptr<transform<float_t> >(new null_trans<float_t> );
    
    }else if(tt == trans_type::TT_twice_fisher){
        
        return std::unique_ptr<transform<float_t> >(new twice_fisher_trans<float_t> );
    
    }else if(tt == trans_type::TT_logit){
        
        return std::unique_ptr<transform<float_t> >(new logit_trans<float_t> );
    
    }else if(tt == trans_type::TT_log){

        return std::unique_ptr<transform<float_t> >(new log_trans<float_t> );
    
    }else{

        throw std::invalid_argument("that transform type was not accounted for");
    
    }
}

template<typename float_t, size_t numelem>
void transform_container<float_t, numelem>::add_transform(trans_type tt)
{
    m_ts[m_add_idx] = transform<float_t>::create(tt);
    m_add_idx++;
}

Are all `numelem` elements of the `m_ts` array valid? I.e. are all pointers initialized and pointing to an actual `transform` object? — Some programmer dude, Mar 31 '21 at 22:40
A [mcve] would be helpful. I wrote up a quick toy program, and it works without a problem. — Eljay, Mar 31 '21 at 22:41
Then all unique pointer objects in the array will be default constructed which means they will be null pointers. Do you initialize *all* `numelem` pointers somewhere else? Actually make them point somewhere? — Some programmer dude, Mar 31 '21 at 22:43
Add `if (!m_ts[i]) throw "oopsies";` just before the line that has the problem. Out of paranoia. — Eljay, Mar 31 '21 at 22:55
@Taylor: Yes, you can't tell a thing that doesn't exist to make a clone of itself — Mooing Duck, Mar 31 '21 at 23:12
Also, learn how to use a debugger; you'll be able to halt at the moment of crash and examine variables. — Yakk - Adam Nevraumont, Mar 31 '21 at 23:15

Remy Lebeau · Accepted Answer · 2021-04-01T01:53:09.237

In get_transforms(), you are looping over the entire m_ts[] array, calling clone() on all elements - even ones that have not been assigned by add_transform() yet! Unassigned unique_ptrs will be holding a nullptr pointer, and it is undefined behavior to call a non-static class method through a nullptr.

The easiest fix is to change the loop in get_transforms() to use m_add_idx instead of numelem:

template<typename float_t, size_t numelem>
auto transform_container<float_t,numelem>::get_transforms() const -> array_ptrs
{
    array_ptrs deep_cpy;
    for(size_t i = 0; i < m_add_idx; ++i){ // <-- here
        deep_cpy[i] = m_ts[i]->clone();
    }
    return deep_cpy;
}

Otherwise, you would have to manually ignore any nullptr elements, eg:

template<typename float_t, size_t numelem>
auto transform_container<float_t,numelem>::get_transforms() const -> array_ptrs
{
    array_ptrs deep_cpy;
    for(size_t i = 0, j = 0; i < numelem; ++i){
        if (m_ts[i]) {
            deep_cpy[j++] = m_ts[i]->clone();
        }
    }
    return deep_cpy;
}

Either way, you should update add_transform() to validate that m_add_idx will not exceed numelem:

template<typename float_t, size_t numelem>
void transform_container<float_t, numelem>::add_transform(trans_type tt)
{
    if (m_add_idx >= numelem) throw std::length_error("cant add any more transforms"); // <-- here
    m_ts[m_add_idx] = transform<float_t>::create(tt);
    ++m_add_idx;
}

That being said, since transform_container can have a variable number of transforms assigned to it, I would suggest changing transform_container to use a std::vector instead of a std::array, eg:

template<typename float_t>
class transform_container{
private:

    using vector_ptrs = std::vector<std::unique_ptr<transform<float_t>>>;
    vector_ptrs m_ts;
...
    auto get_transforms() const -> vector_ptrs;
};

template<typename float_t>
auto transform_container<float_t>::get_transforms() const -> vector_ptrs
{
    vector_ptrs deep_cpy;
    deep_cpy.reserve(m_ts.size());
    for(const auto &elem : m_ts){
        deep_cpy.push_back(elem->clone());
    }
    return deep_cpy;
}

template<typename float_t>
std::unique_ptr<transform<float_t>> transform<float_t>::create(trans_type tt)
{
    switch (tt) {
        case trans_type::TT_null:
            return std::make_unique<null_trans<float_t>>();

        case trans_type::TT_twice_fisher:
            return std::make_unique<twice_fisher_trans<float_t>>();
    
        case trans_type::TT_logit:
            return std::make_unique<logit_trans<float_t>>();
    
        case trans_type::TT_log:
            return std::make_unique<log_trans<float_t>>();
    }    

    throw std::invalid_argument("that transform type was not accounted for");
}

template<typename float_t>
void transform_container<float_t>::add_transform(trans_type tt)
{
    m_ts.push_back(transform<float_t>::create(tt));
}

getting a segfault when I try to deep copy `unique_ptr`s

Edit:

1 Answers1