C++ access violation in msvc but not gcc for multiple inheritance and cast

Question

I have a fairly large project with multiple interfaces and implementations. The code was implemented on a linux environment using g++ (5.4 I think). After porting the code to Windows and running it with VS15 (MSVC v140) I got an access violation after trying to access a casted pointer.

This is the inheritance hierarchy in the code below:

                 A    
                / \  
     virtual   /   \   
              /     |
             B      |
             |      | virtual
             C      |
             |      |
             \      / 
              \    /
                D

In the real code the inheritance design includes more classes so please don't pick on why this is the way I inherit from A. I've narrowed down the code to present only what is necessary.

The following runs with gcc and prints foo called twice (Live demo on rextester), but with msvc on the second call to foo crashes with access violation (Live demo on rextester)

#include <iostream>

class A{};

class B : public virtual A{};

class C : public B
{
public:
    virtual void foo() = 0;
};

class D : public virtual A, public C
{
public:
    bool convert(int id, B** ext)
    {
        if (id == 1)
        {
            *ext = (C*)this;
            return true;
        }

        if (id == 42)
        {
            C** pp_ext = (C**)(ext);
            *pp_ext = (C*)this;
            return true;
        }

        return false;
    }
    void foo() override
    {
        std::cout << "foo called" << std::endl;
    }
};


int main()
{
    D s;
    C* foo_ext = nullptr;
    s.convert(42, (B**)&foo_ext);
    foo_ext->foo();

    foo_ext = nullptr;
    s.convert(1, (B**)&foo_ext);
    foo_ext->foo();

    return 0;
}

First - Am I missing a fundamental error in the *ext = (C*)this; conversion?

Second - Why is this code different in the two compilers?

EDIT:

1. This code uses pointers, pointers to pointers and is built with this inheritance for good reasons (one of which is ABI compliant interface).

2. dynamic_cast doesn't change the behavior in this case.

3. If I call static_cast<C*>(*ext)->foo(); after *ext = (C*)this; it will call foo, but fail after returning from convert. This is something I have already understood and this is what made me understand that the solution for 42 is a (good?) solution.

Answer 1

When dealing with inheritance, and especially multiple inheritance, you should really try to avoid casts entirely. But if you have to cast, then use either static_cast or dynamic_cast. That way the compiler will help you avoid invalid conversions. If you do anything else, then you have to understand all the details of C++ as well as or better than the compiler itself! Otherwise you risk easily making mistakes. Like you did here.

Try modifying your main to something like:

int main()
{
    D s;

    A* a = &s;
    B* b = &s;
    C* c = &s;
    std::cout << "A address = " << a << std::endl;
    std::cout << "B address = " << b << std::endl;
    std::cout << "C address = " << c << std::endl;
    std::cout << "D address = " << &s << std::endl;

    C* foo_ext = nullptr;
    s.convert(42, (B**)&foo_ext);
    std::cout << "foo_ext = " << foo_ext << std::endl;
    foo_ext->foo();

    foo_ext = nullptr;
    s.convert(1, (B**)&foo_ext);
    std::cout << "foo_ext = " << foo_ext << std::endl;
    foo_ext->foo();

    return 0;
}

Running that up until the crash, I get:

A address = 0037FEA0
B address = 0037FE9C
C address = 0037FE98
D address = 0037FE98
foo_ext = 0037FE98
foo called
foo_ext = 0037FE9C

Obviously in the second case foo_ext is not being set to the proper C address of the object but instead to the B part. In practice then, the call to foo() might be going through an incorrect or non-existent virtual table pointer, thus the crash.

Now, why does the first case work? Well, cutting it down to the minimum of what's happening, you've effectively done:

C* foo_ext = nullptr;
C** ppc = &foo_ext;
B** ppb = (B**)ppc;
C** pp_ext = (C**)ppb;
*pp_ext = &s;

So it starts with a C pointer and ends with a C pointers. And the compiler knows how to properly shift a D pointer to be C pointer. (Derived to base conversion. The compiler doesn't need a cast to do that.)

However in the second case you effectively have:

C* foo_ext = nullptr;
C** ppc = &foo_ext;
B** ext = (B**)ppc;
*ext = &s;

So at the last line the compiler is shifting a D pointer to become a B pointer. But really that address is ending up back in a C pointer. So it has been shifted too far back up the inheritance hierarchy!

---

Now, how to fix you program... Well, the basic idea of course is to get rid of most of the casting and replace the remaining couple spots with static or dynamic casts. Try the following:

#include <iostream>

class A {};

class B : public virtual A {};

class C : public B
{
public:
    virtual void foo() = 0;
};

class D : public virtual A, public C
{
public:
    bool convert(int id, B** ext)
    {
        if (id == 1)
        {
            *ext = this;
            return true;
        }

        if (id == 42)
        {
            *ext = this;
            return true;
        }

        return false;
    }
    void foo() override
    {
        std::cout << "foo called" << std::endl;
    }
};


int main()
{
    D s;

    A* a = &s;
    B* b = &s;
    C* c = &s;
    std::cout << "A address = " << a << std::endl;
    std::cout << "B address = " << b << std::endl;
    std::cout << "C address = " << c << std::endl;
    std::cout << "D address = " << &s << std::endl;

    B* b_pointer = nullptr;
    C* foo_ext = nullptr;
    s.convert(42, &b_pointer);
    foo_ext = static_cast<C*>(b_pointer);
    std::cout << "foo_ext = " << foo_ext << std::endl;
    foo_ext->foo();

    b_pointer = nullptr;
    foo_ext = nullptr;
    s.convert(1, &b_pointer);
    foo_ext = static_cast<C*>(b_pointer);
    std::cout << "foo_ext = " << foo_ext << std::endl;
    foo_ext->foo();

    return 0;
}

This should run fine with all the expected output. The static casts allow the compiler to properly situate the pointers as per their types. (And in this limited example we of course know what all the types really are, so we don't have to bother with dynamic cast.)

I assume this example was based on a real world problem. I can't say whether this exact solution is appropriate for that (since obviously I don't know the real situation). But the principle is sound: don't lie to the compiler by forcing questionable casts. Let it automatically convert whenever possible, and resort only to static_cast or dynamic_cast when necessary.

Answer 2

The first runtime problem comes at:

s.convert(42, (B**)&foo_ext);

foo_ext has type C *. So the use of *ext inside convert commits a strict aliasing violation, by accessing the memory of a C * as if it were a B * . In general, pointers to different types may have different size and representation; but even if they don't, it is still not permitted to alias them.

Although the strict aliasing rule has an exception for accessing base classes, that doesn't extend to base class pointers.

MSVC probably doesn't enforce strict aliasing in that situation (in fact some things in the Windows API rely on that behaviour). But if you want to write portable code, it would be a good idea to not rely on strict aliasing violations.

---

The convert function uses pass-by-pointer. There is never a need for this in C++. You can use pass-by-reference as a language feature instead. This reduces the chance of making mistakes -- some of the mistakes in your code actually can't even be expressed in reference notation.

Here is a modified version:

#include <iostream>

class A{};

class B : public virtual A{};

class C : public B
{
public:
    virtual void foo() = 0;
};

class D : public virtual A, public C
{
public:
    bool convert(int id, B*& ext)
    {
        if (id == 1)
        {
            ext = static_cast<C *>(this);  // note: redundant cast
            return true;
        }

        if (id == 42)
        {
            ext = this;
            return true;
        }

        return false;
    }
    void foo() override
    {
        std::cout << "foo called" << std::endl;
    }
};

int main()
{
    D s;
    B* foo_ext_b;
    C* foo_ext;

    foo_ext_b = nullptr;

    if ( !s.convert(42, foo_ext_b) )
        throw std::runtime_error("convert42 failed");

    foo_ext = static_cast<C *>(foo_ext_b);
    foo_ext->foo();

    foo_ext_b  = nullptr;

    if ( !s.convert(1, foo_ext_b) )
        throw std::runtime_error("convert1 failed");

    foo_ext = static_cast<C *>(foo_ext_b);
    foo_ext->foo();

    return 0;
}

Note that using foo_ext = static_cast<C *>(foo_ext_b); , in general, is error-prone. There will be silent undefined behaviour if convert "returned" a B * that did not happen to point to a B which is a base class of a C instance.

For safety you would use dynamic_cast instead. But to allow dynamic_cast to work, the base class must have at least one virtual function. You could add a virtual destructor to B or A.