segfault at method invocation

Pages: 12
nuderobmonkey (640)
I get at line 55 a segfault when the fly-method will get invoked.
If I exchange the raw pointers by smart pointers the error disappears.

Any idea why this happens?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
 
/**
 * Testing the strategy pattern.
 *  _______________             _________________
 * |FlyBehavior   |            | Duck            |
 * | virtual fly()|<-----------|  *m_flyBehavior |
 * '---^----^-----'            '-----------------'
 *     |     \
 *  ___|___   \_______
 * |NoFly  |  |FastFly|
 * | fly() |  |  fly()|
 * '-------'  '-------'
 *
 */

#include <iostream>

class FlyBehavior 
{ 
public:
    virtual void fly() = 0;
    virtual ~FlyBehavior(){}
};

class NoFly : public FlyBehavior 
{
public: 
    void fly() override { std::cout << "I cannot fly!  "; }
};

class FastFly : public FlyBehavior 
{
public:
    void fly() override { std::cout << "I fly very fast. "; }
};


class Duck
{
public:
    void fly();
    Duck( FlyBehavior * fb);
    ~Duck();
private:
    FlyBehavior * m_flyBehavior;
};
Duck::Duck( FlyBehavior * fb )
    : m_flyBehavior{fb}
{}
Duck::~Duck()
{
    delete m_flyBehavior;
}
void Duck::fly()
{
    m_flyBehavior -> fly ();
}

#include <random>
#include <vector>

int main()
{
    auto rng = std::default_random_engine( std::random_device{}() );
    auto d = std::uniform_int_distribution<>( 0, 1 );
    
    std::vector<Duck> v;
    v.reserve(64);
    
    for( int i = 0; i < 64; ++i)
    {
        FlyBehavior * fb;
        if( d(rng) ) fb = new NoFly;
        else fb = new FastFly;

        v.push_back( Duck( fb ) );
    }
    
    for( Duck & duck : v)
    {
        duck.fly();
        std::cout<<std::endl;
    }
}
againtry (2313)
1
2
3
4
 
Duck::~Duck()
{
   // delete m_flyBehavior; // <--
}
nuderobmonkey (640)
Thanks.

But I don't understand why the error comes from the destructor. I thought it would get invoked not before the lifetime of the vector ends (what would be at the end of main() )
againtry (2313)
The problem arises because the pointer you are 'delete'-ing in the destructor wasn't created with a 'new'. Deleting the pointer in main is also problematic - try it. Tricky - now for somebody to impress smart pointers.

As an aside, FlyBehavior is probably, intuitively, a method rather than a separate class. SlowDucks and FastDucks would inherit their flying behavior from Ducks. Up to you because I don't really give a "flying (d)uck" :)

nuderobmonkey (640)
The problem arises because the pointer you are 'delete'-ing in the destructor wasn't created with a 'new'.
Can you further explain that issue? I thought that deleting would be referred to addresses in the memory so that it wouldn't matter from where the deletion would be invoked.

And you are right, fly() would be better when it would be implemented by inheritance. But my code example is still a snipped of some greater part where I have some more traits so I cannot use anymore inheritance if I would being able freely combining these traits together.
againtry (2313)
Wait for a second and I'll show you the message I got when I ran your program using XCode where the errors are a bit more explanatory - in essence your destructor is deleteing something that doesn't exist, ie it wasn't created using new. If it had then the delete would be OK.

I cannot use anymore inheritance if I would being able freely combining these traits together.
I'm not sure what you mean and while I don't want to belabor the point about inheritance, it seems a bit strange that inheritance is limited. But I'm not aware of the greater part so last said.
againtry (2313)
So, running your code as-is under XCode clang ... the output is:

I cannot fly!  

... lots of the same meassage 

I cannot fly!  
I cannot fly!  
I cannot fly!  
General(5725,0x1000dfdc0) malloc: *** error for object 0x1006ab990: pointer being freed was not allocated
General(5725,0x1000dfdc0) malloc: *** set a breakpoint in malloc_error_break to debug
(lldb)


Hence I deleted the line in the destructor simply because it doesn't reference any new memory ie that created by a new.

Keep in mind you can declare a pointer to an object-type without creating the object. eg Object* obj = nullptr; doesn't point to 'anything'

'delete' that and see what happens :)
Last edited on
mbozzi (3932)
The problem arises from this line:
v.push_back( Duck( fb ) );

The subexpression Duck(fb) constructs a temporary object of type Duck whose lifetime ends at the semicolon;

A shallow copy of that object is pushed into the vector, and the temporary is destroyed at the semicolon, which runs the destructor and deletes fb. The Duck object stored in the vector contains a copy of a pointer which has already been deleted.

What could have been done about this?
Follow the Rule of Three: https://en.cppreference.com/w/cpp/language/rule_of_three
Last edited on
againtry (2313)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
 
#include <iostream>

int main()
{
    int* p;
   // delete p; // Error
    
    int* q = new int{89};
    std::cout << *q << '\n';
    
    delete q;
    q = nullptr;
    
    std::cout << q << '\n';
    
    return 0;
}
mbozzi (3932)
The problem is similar to the one exhibited here:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
// Red flag: a assumes ownership of a resource but does not
// implement the Rule of Three
struct a { 
    int* p = nullptr;
    ~a() { delete p; }
};
// note: by default, the expression 'delete nullptr' has no effect.

int main()
{
    // assuming no exception is thrown during allocation:
    a x { new int }; 
     
    { 
        a y = x; 
        *y.p = 42;
    } // y.p is destroyed here

    // since x.p has the same value, x.p no longer points to an
    // object within its lifetime
    
    *x.p = 24; // wrong: access to an object outside of its lifetime
} // If the program did not crash by now, delete x.p again.
// Many systems will catch this error, even if the prior errors go unnoticed.
Last edited on
nuderobmonkey (640)
Thanks!

Now I pass the Duck object with v.push_back( std::move(Duck( fb )) ); to the vector and it runs fine.
againtry (2313)
Now I pass the Duck object

That's another way - very tricky eh! :)
mbozzi (3932)
Now I pass the Duck object with v.push_back( std::move(Duck( fb )) );

Unless you implemented a move constructor (abiding by the rule of three), this does not solve the problem.

Further, since Duck(fb) is a prvalue expression, the call to std::move will never have any effect, and it can be removed.

Note that the subexpression Duck(fb) still has the same effects, and therefore the program will still exhibit the same issue. To avoid creating that temporary object at all, you could write v.emplace_back( fb ) instead. This would work fine until the vector resizes itself!

An option is to implement proper value semantics for Duck via the Rule of Three, so that the identity of individual Duck objects becomes unimportant.

The ideal choice is to store smart pointers instead. This places the burden of resource management onto a standard library class, obeys the Single Responsibility Principle, and saves you a significant amount of detailed work.
Last edited on
againtry (2313)
Rule of (thumb) Three refers to copy constructor among the other two - which is necessary for OP's problem at least.

One way of looking at it is, sooner or later, a 'permanent' object has to be created, even if it is going to be shuffled round by pointers. Creating an object as a pointer to itself is obscure to say the least. C++ is a bit of a camel.

Probably repeating but the constructor itself could be another place for creating the object from the pointer.

Rule of Five adds the move constructor making the rule of 3 broader. Who knows whether that adds value?

nuderobmonkey (640)
Ok ok, I cheated :)
I had still forgotten to remove the comment-out of the delete directive. Now I have added a copy constructor and a assignment operator method, but I get still a segfault. Could you check if I have them correctly implemented?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
 
class Duck
{
public:
    void fly();
    Duck( FlyBehavior * fb);
    Duck( const Duck& other);
    Duck & operator= (const Duck& other);
    ~Duck();
private:
    FlyBehavior * m_flyBehavior;
};
Duck::Duck( FlyBehavior * fb )
    : m_flyBehavior{fb}
{}
Duck::Duck( const Duck& other)
{
    if(this != &other)
        m_flyBehavior = other.m_flyBehavior;
}
Duck & Duck::operator= ( const Duck& other)
{
    if( this != &other) {
        delete m_flyBehavior;
        m_flyBehavior = other.m_flyBehavior;
    }
    return *this;
}
Duck::~Duck()
{
    delete m_flyBehavior;
}
void Duck::fly()
{
    m_flyBehavior -> fly ();
}
againtry (2313)
ok
againtry (2313)
Your code runs by commenting out the delete in the constructor but it all looks a bit obscure with the rest.
nuderobmonkey (640)
I've now added a move assignment operator and a move constructor. Then it works all good, no idea, why :)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
 
/**
 * Testing the strategy pattern.
 *  _______________             _________________
 * |FlyBehavior   |            | Duck            |
 * | virtual fly()|<-----------|  *m_flyBehavior |
 * '---^----^-----'            |  fly()          |
 *     |     \                 +-----------------+
 *  ___|___   \_______
 * |NoFly  |  |FastFly|
 * | fly() |  |  fly()|
 * '-------'  '-------'
 */

#include <iostream>
#include <utility>

class FlyBehavior 
{ 
public:
    virtual void fly() = 0;
    virtual ~FlyBehavior(){}
};

class NoFly : public FlyBehavior 
{
public: 
    void fly() override { std::cout << "I cannot fly!  "; }
};

class FastFly : public FlyBehavior 
{
public:
    void fly() override { std::cout << "I fly very fast. "; }
};


class Duck
{
public:
    void fly();
    Duck( const std::string & s_flyBehavior = "NoFly" );
    Duck( FlyBehavior * fb = new NoFly);
    Duck( const Duck& other );
    Duck( Duck && other);
    Duck & operator= ( const Duck & other );
    Duck & operator= ( Duck && other);
    ~Duck();
private:
    FlyBehavior * m_flyBehavior;
};
Duck::Duck( const std::string & s_flyBehavior)
{
    if( s_flyBehavior == "FastFly" ) m_flyBehavior = new FastFly;
    else m_flyBehavior = new NoFly;
}
Duck::Duck( FlyBehavior * fb)
    : m_flyBehavior{fb}
{}
Duck::Duck( const Duck& other)
{
    if(this != &other)
        m_flyBehavior = other.m_flyBehavior;
}
Duck & Duck::operator= ( const Duck& other)
{
    if( this != &other) {
        delete m_flyBehavior;
        m_flyBehavior = other.m_flyBehavior;
    }
    return *this;
}
Duck::Duck( Duck && other)
{
    std::swap(other.m_flyBehavior, m_flyBehavior);
}
Duck & Duck::operator= ( Duck && other)
{
    std::swap( m_flyBehavior, other.m_flyBehavior );
    return *this;
}
Duck::~Duck()
{
    delete m_flyBehavior;
}
void Duck::fly()
{
    m_flyBehavior -> fly ();
}

#include <random>
#include <vector>

int main()
{
    auto rng = std::default_random_engine( std::random_device{}() );
    auto d = std::uniform_int_distribution<>( 0, 1 );
    
    std::vector<Duck> v;
    v.reserve(64);
    
    for( int i = 0; i < 64; ++i)
    {
        FlyBehavior * fb;
        if( d(rng) ) fb = new NoFly;
        else fb = new FastFly;

        v.push_back( Duck( fb ) );
    }
    
    for( Duck & duck : v)
    {
        duck.fly();
        std::cout<<std::endl;
    }
}


But anyway I will stick to unique_ptr for my final code.
againtry (2313)
Or this maybe? Anyway, this was what I was thinking about :]

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
 
#include <iostream>

class FlyBehavior
{
protected:
    std::string m_flyBehavior;
    
public:
    FlyBehavior(){};
    ~FlyBehavior(){};
    
    void fly()
    {
        std::cout << m_flyBehavior << '\n';
    }
    
    std::string getBehavior()
    {
        return m_flyBehavior;
    }
    
    void setBehavior(std::string aBehavoir)
    {
        m_flyBehavior = aBehavoir;
    }
    
};


class NoFly : public FlyBehavior
{
public:
    NoFly()
    {
        m_flyBehavior = "I cannot fly!  ";
    }
};


class FastFly : public FlyBehavior
{
public:
    FastFly()
    {
        m_flyBehavior = "I fly very fast. ";
    }
};


class Duck : public FlyBehavior
{
public:
    Duck(FlyBehavior* fb)
    {
        m_flyBehavior = fb -> getBehavior();
    };
    
    ~Duck()
    {
        std::cout << "Duck deleted ... :] ???\n";
    }
    
};

#include <random>
#include <vector>

int main()
{
    auto rng = std::default_random_engine( std::random_device{}() );
    auto d = std::uniform_int_distribution<>( 0, 1 );
    
    std::vector<Duck> v;
    v.reserve(64);
    
    for( int i = 0; i < 64; ++i)
    {
        FlyBehavior* fb;
        if( d(rng) )
        {
            fb = new NoFly;
        }
        else
        {
            fb = new FastFly;
        }
        
        v.push_back( Duck( fb ) );

        delete fb;
        fb = nullptr;

    }
    
    for( Duck & duck : v)
    {
        duck.fly();
    }
}
nuderobmonkey (640)
Yea, that's plain inheritance. But assume, you have a lot of different behaviors...

I have here only fly(), eat(), quack().

Still with these three behaviors, there are eight combinations possible.
And how would you order them into an inheritance tree?
Pages: 12