Opened 8 months ago

Closed 5 months ago

#15884 closed defect (fixed)

ODR violation in wxWeakRef

Reported by: minoki Owned by: VZ
Priority: normal Milestone: 3.1.0
Component: base Version:
Keywords: wxWeakRef Cc:
Blocked By: Blocking:
Patch: yes

Description

Currently, wxWeakRef is implemented differently for complete types and incomplete types. So, the same wxWeakRef<SomeClass> may have different implementation between compilation units (i.e. ODR violation). This may lead to program crash.

wxWeakRef must use the same implementation (or, at least, must have the same data members) whether the type is incomplete or not.

The attached files (mytrackable.h, foo.h, foo.cpp, test.cpp) reproduce the problem.

Attachments (5)

mytrackable.h download (160 bytes) - added by minoki 8 months ago.
Reproducing code (1)
foo.h download (132 bytes) - added by minoki 8 months ago.
Reproducing code (2)
foo.cpp download (303 bytes) - added by minoki 8 months ago.
Reproducing code (3)
test.cpp download (417 bytes) - added by minoki 8 months ago.
Reproducing code (4)
Unify-wxWeakRef-implementation.patch download (7.4 KB) - added by minoki 5 months ago.

Download all attachments as: .zip

Change History (12)

Changed 8 months ago by minoki

Reproducing code (1)

Changed 8 months ago by minoki

Reproducing code (2)

Changed 8 months ago by minoki

Reproducing code (3)

Changed 8 months ago by minoki

Reproducing code (4)

comment:1 follow-up: Changed 8 months ago by vadz

  • Milestone set to 3.1.0

We definitely need to fix this, thanks.

I didn't have time to look at the code in details yet, but would defining USE_ONLY_STATIC_WEAKREF fix this? This would give us at least some solution, even though not the most efficient one...

comment:2 in reply to: ↑ 1 Changed 8 months ago by minoki

Replying to vadz:

We definitely need to fix this, thanks.

I didn't have time to look at the code in details yet, but would defining USE_ONLY_STATIC_WEAKREF fix this? This would give us at least some solution, even though not the most efficient one...

Defining USE_ONLY_STATIC_WEAKREF does fix ODR violation, but it also breaks a user code like this:

#include <wx/wx.h>

class MyTrackableClass;

int main() {
    wxWeakRef<MyTrackableClass> foo;
}

If we are going to support user code like above, we should use the implementation of current wxWeakRefImpl<T, false>.

comment:3 follow-up: Changed 7 months ago by vadz

  • Status changed from new to confirmed

I wonder if not allowing wxWeakRefs of incomplete classes might be a better solution than never using wxWeakRefStatic at all. It does look like a pity to to have to use twice as much space and dynamic casts unnecessarily... Maybe we could have wxWeakForwardRef which could be used for incomplete classes to still allow this if it's really useful (is it?), but forbid their use with wxWeakRef itself, what do you think?

comment:4 in reply to: ↑ 3 Changed 5 months ago by minoki

Sorry, I've missed your comment somehow.

I'd rather have one wxWeakRef class even if it used twice as much space. It would be confusing if there were multiple wxWeakRefs (although we have wxWeakRef and wxWeakRefDynamic already).

As for dynamic cast, IIRC, there will be no runtime cost if the class is statically known to be derived from wxTrackable (i.e. upcast). Or, we might require the class to be complete when assigning the pointer, and allowing the class to be incomplete when copying and destructuring. The latter would not use dynamic casts and allows comment:2 to compile.

comment:5 Changed 5 months ago by vadz

Using twice as much space is not a problem, so if you see a solution which allows to fix the ODR violation while still remaining backwards compatible, I'm all for it. I'm afraid I just don't see how exactly would it work... But, again, if you do, please just submit a patch with it (if possible, together with a change to the unit tests checking for the situation of comment:2). TIA!

Changed 5 months ago by minoki

comment:6 Changed 5 months ago by minoki

  • Patch set

I've attached a patch to unify wxWeakRef implementation.

As in tests/weakref/weakref.cpp, the following code is valid:

{
    // Construction of a wxWeakRef to an incomplete class should be fine
    wxWeakRef<IncompleteClass> p;

    // Copying should be also OK
    p = p;

    // Assigning a raw pointer should cause compile error
    //p = static_cast<IncompleteClass*>(0);

    // Releasing should be OK
}

(This code is valid in the old implementation, but not valid with -DUSE_ONLY_STATIC_WEAKREF)

comment:7 Changed 5 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from confirmed to closed

In 76316:

Use the same wxWeakRef implementation for complete and incomplete classes.

This fixes an ODR violation which could arise if wxWeakRef<T> was seen both
when T was an incomplete (e.g. just forward-defined) class and when it was
complete. As different implementations, with different binary layouts, were
used in these two cases, this resulted in fatal run-time problems.

Fix this by always using the slightly less efficient (because storing an extra
pointer) but simpler and safe "dynamic" wxWeakRef implementation.

Also get rid of checks for the ancient compilers such as VC6 and g++ < 3.3,
they are not supported any longer.

Closes #15884.

Note: See TracTickets for help on using tickets.