Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash due to undefined behaviours #308

Closed
wants to merge 5 commits into from

Conversation

sgn
Copy link

@sgn sgn commented Jul 7, 2021

I discovered this problem when running fcitx5-config-qt, fcitx5 crashes as soon as the config tool is launched.

I have gcc 10.2.1 (pre-version), fcitx5 (5.0.8), dbus 1.12.20.


As of it's now, we're downcasting from SignalBase to
Signal (which is the same type with
Signal<SignalType, LastValue<...>>) in its member functions.

The downcast is incorrect if its Combiner is NOT LastValue, fcitx5 will
run into undefined behaviours with this kind of casting.

In my local machine, this result in indeterminated value in
CheckUpdateResult, then fcitx5 will crash because of DBus' assertion.

When compiling with: -fsanitize=undefined, this problem is being
reported:

../src/lib/fcitx/../fcitx-utils/connectableobject.h:115:18: runtime error: downcast of address 0x559bd4a2a5b0 which does not point to an object of type 'Signal'
0x559bd4a2a5b0: note: object is of type '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
 00 00 00 00  40 25 21 2e f7 7f 00 00  e0 f5 a2 d4 9b 55 00 00  20 70 61 67 65 00 00 00  41 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
../src/lib/fcitx/../fcitx-utils/connectableobject.h:116:21: runtime error: member call on address 0x559bd4a2a5b0 which does not point to an object of type 'Signal'
0x559bd4a2a5b0: note: object is of type '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
 00 00 00 00  40 25 21 2e f7 7f 00 00  e0 f5 a2 d4 9b 55 00 00  20 70 61 67 65 00 00 00  41 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'

@sgn sgn changed the title Fix undefined behaviours Fix crash due to undefined behaviours Jul 7, 2021
sgn added 5 commits July 9, 2021 08:42
As of it's now, we're downcasting from SignalBase to
Signal<SignalType> (which is the same type with
Signal<SignalType, LastValue<...>>) in its member functions.

The downcast is incorrect if its Combiner is NOT LastValue, fcitx5 will
run into undefined behaviours with this kind of casting.

In my local machine, this result in indeterminated value in
CheckUpdateResult, then fcitx5 will crash because of DBus' assertion.

When compiling with: -fsanitize=undefined, this problem is being
reported:

```
../src/lib/fcitx/../fcitx-utils/connectableobject.h:115:18: runtime error: downcast of address 0x559bd4a2a5b0 which does not point to an object of type 'Signal'
0x559bd4a2a5b0: note: object is of type '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
 00 00 00 00  40 25 21 2e f7 7f 00 00  e0 f5 a2 d4 9b 55 00 00  20 70 61 67 65 00 00 00  41 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
../src/lib/fcitx/../fcitx-utils/connectableobject.h:116:21: runtime error: member call on address 0x559bd4a2a5b0 which does not point to an object of type 'Signal'
0x559bd4a2a5b0: note: object is of type '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
 00 00 00 00  40 25 21 2e f7 7f 00 00  e0 f5 a2 d4 9b 55 00 00  20 70 61 67 65 00 00 00  41 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
```

As a preparatory step, let's add combinerType into data structure for
Signal, and uses it as default combiner.
In order to correctly downcast SignalBase, in the next step,
we will switch all initialisation of SignalAdaptor to use
SignalType::combinerType.

Let's introduce a new macro to ease to initialisation of those
signals which employs custom combiner.

Arguably, the custom combiner is an implementation detail,
thus it'll be put in source files. Hence, the macro is intended
to be used outside of class declaration.
…Result

As of it's now, we're downcasting Signal data structure for CheckUpdate
from SignalBase to Signal<bool()> (which is the same type with
Signal<bool(), LastValue<bool>>) in SignalAdaptor member functions.

The downcast is incorrect because it's Signal<bool(), CheckUpdateResult>
instead, thus fcitx5 will run into undefined behaviours with this downcasting.

In my local machine, this result in indeterminated value in
CheckUpdateResult, then fcitx5 will crash because of DBus' assertion.

When compiling with: -fsanitize=undefined, this problem is being
reported:

```
../src/lib/fcitx/../fcitx-utils/connectableobject.h:115:18: runtime error: downcast of address 0x559bd4a2a5b0 which does not point to an object of type 'Signal'
0x559bd4a2a5b0: note: object is of type '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
 00 00 00 00  40 25 21 2e f7 7f 00 00  e0 f5 a2 d4 9b 55 00 00  20 70 61 67 65 00 00 00  41 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
../src/lib/fcitx/../fcitx-utils/connectableobject.h:116:21: runtime error: member call on address 0x559bd4a2a5b0 which does not point to an object of type 'Signal'
0x559bd4a2a5b0: note: object is of type '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
 00 00 00 00  40 25 21 2e f7 7f 00 00  e0 f5 a2 d4 9b 55 00 00  20 70 61 67 65 00 00 00  41 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for '*N5fcitx6SignalIFbvENS_12_GLOBAL__N_117CheckUpdateResultEEE'
```

Let's downcast it into correct type by setting CheckUpdateResult as
combinerType.
Now, all of our code paths use the correct downcasting, let's enforce
the correct downcasting by doing the static_assert.
With the introduction of FCITX_DECLARE_SIGNAL_WITH_COMBINER and the
previous enforcement. The combiner should always pull from signals'
data structure now.

Let's remove those unused macros to prevent mis-usage.
@sgn sgn force-pushed the fix-undefined-behaviours branch from 54e0fac to c1e676f Compare July 9, 2021 01:44
@wengxt wengxt closed this in 6393480 Aug 5, 2021
@wengxt
Copy link
Member

wengxt commented Aug 5, 2021

Thanks, but I'd just avoid use this for now until we bump next abi version.

@sgn
Copy link
Author

sgn commented Aug 5, 2021

Sorry, which ABI breakage did you mean?
In my naive check:

  • all data structure layout aren't changed.

  • the symbol table for old and new fcitx-utils only differs in _ZN5fcitx12_GLOBAL__N_115checkBoolEnvVarEPKc which is function in anonymous namespace, so nothing to worry about
    libfcitx-utils-abi.old.txt
    libfcitx-utils-abi.new.txt

  • new libFcitxCore.so's only misses fcitx::Signal<bool (), fcitx::LastValue<bool> >::operator()() (which is a symbol from fcitx-utils) and those typeinfo for some std::function, which could be added in a dummy functions
    libfcitx-abi.old.txt
    libfcitx-abi.new.txt


Anyway, this is not a deal breaker since I think current CheckUpdateResult isn't used anyway.

@wengxt
Copy link
Member

wengxt commented Aug 5, 2021

Here's the thing, with your change, you can't do

instance->connect<CheckUpdate>(...)

That defeat the purpose of this signal connect API.

The intention of this "combiner" should be able to erase and not expose the combiner type in the symbol, which means it would require change to ConnectableObject's template function compiled within each addon library.

Ideally, there should be a type erasured version of Signal, but then you introduce the change to SignalBase -> Signal type hierarchy.

@wengxt
Copy link
Member

wengxt commented Nov 1, 2021

Same feature was revived via a different mechanism in 5ddc4ea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants