tdlib / td

Cross-platform library for building Telegram clients
https://core.telegram.org/tdlib
Boost Software License 1.0
7.11k stars 1.44k forks source link

Fix DialogActionManager::send_dialog_action in secret chats #2756

Closed LiruMouse closed 9 months ago

LiruMouse commented 9 months ago

The code recently introduced in DialogActionManager::send_dialog_action makes the assumption that, since we'd either have early returned or DialogManager::have_input_peer would return true; that the else block of the conditional, which intended to set input_peer, should have done so successfully.

However, DialogManager::have_input_peer will return true for secret chats when contacts_manager->have_input_encrypted_peer And DialogManager::get_input_peer will return a nullptr for secret chats by design (I believe this is because they operate using structures that are currently incompatible)

There is a block in DialogActionManager::send_dialog_action meant for handling Secret Chats that will return early and avoid using the locally scoped data, like input_peer, being prepared at the beginning of the function.

This should fix the root of a crash that happens when typing in or sending messages (like ones with attachments) to secret chats on: Telegram X 0.26.4.1678-arm64-v8a (99b10675) TDLib: 1.8.23 (tdlib/td@4bafdc2)

My proposed change moves the Secret Chat block up to before anything is done to prepare locally scoped data objects that aren't used in the case of secret chats. Note that it doesn't include the return when is_dialog_action_unneeded, and I do not have the familiarity to know if that is still useful to check first. If this is the case, it may be better to simply move the CHECK(input_peer != nullptr); to after the block meant to handle secret chats, because it is after point where we will access that pointer, and would therefore want it set.

levlam commented 9 months ago

Thank you!

You have analyzed the code and source of the crash correctly.

If this is the case, it may be better to simply move the CHECK(input_peer != nullptr); to after the block meant to handle secret chats, because it is after point where we will access that pointer, and would therefore want it set.

Yes, this would be the correct fix. The code was moved from MessagesManager mostly intact, but the CHECK was added in the wrong place. It should be moved below the secret chat case to fix the issue.

levlam commented 9 months ago

Thank you, @LiruMouse!