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

TDLib crashes when compiled with OpenSSL 3.4.0 #3171

Closed
vincentneo opened this issue Dec 6, 2024 · 15 comments
Closed

TDLib crashes when compiled with OpenSSL 3.4.0 #3171

vincentneo opened this issue Dec 6, 2024 · 15 comments

Comments

@vincentneo
Copy link
Contributor

Crashes after first few updates are received.

Crash at line 954 of crypto.cpp (int err = PKCS5_PBKDF2_HMAC(password.data(), narrow_cast<int>(password.size()), salt.ubegin(),...)

Looking at crash location, I suspect it's an incompatibility with OpenSSL 3.4.0. Will confirm later by recompiling using 3.2.2 which is a known good version.

Information:

  • TDLib 1.8.41, c85b20a
  • OpenSSL 3.4.0
  • Tested on watchOS 11.0
@vincentneo
Copy link
Contributor Author

When compiled with OpenSSL 3.2.2, as expected TDLib 1.8.41 works fine.

@levlam
Copy link
Contributor

levlam commented Dec 6, 2024

The trace point to internals of the OpenSSL, which highly likely makes the bug an OpenSSL 3.4.0 bug. To confirm this and remove other possible causes a minimal example is required. Could you build the code

#include <openssl/evp.h>

void test_pbkdf2() {
  unsigned char buf[32];
  unsigned char salt[4];
  salt[0] = salt[1] = salt[2] = salt[3] = 's';
  PKCS5_PBKDF2_HMAC("password", 8, salt, 4, 2, EVP_sha256(), 32, buf);
}

, call the function from your code without loading TDLib and check whether it crashes?

@vincentneo
Copy link
Contributor Author

Indeed it crashes too.

Screenshot 2024-12-07 at 12 59 59 AM Project used to run can be found in this repo: https://github.com/vincentneo/OpenSSL340Crash

@levlam
Copy link
Contributor

levlam commented Dec 6, 2024

Does it crash only for some specific architecture?

@vincentneo
Copy link
Contributor Author

I think so as it didn't crash on the watchOS simulator (arm64). It only crashed on the watch device (arm64_32)

@levlam
Copy link
Contributor

levlam commented Dec 7, 2024

Could you show the exact place of the crash in ossl_ht_get function?

@vincentneo
Copy link
Contributor Author

Screenshot 2024-12-07 at 11 35 48 PM

Perhaps I should rebuild this with dSYMs or something

@levlam
Copy link
Contributor

levlam commented Dec 7, 2024

Could you do the last test before this can be properly reported to the OpenSSL team? Could you rebuild OpenSSL 3.4.0 with -DUSE_ATOMIC_FALLBACKS and check whether this fixes the issue? There is no need for dSYMs, the assembly view is good enough.

@vincentneo
Copy link
Contributor Author

Sorry but can you advise where to add -DUSE_ATOMIC_FALLBACKS? I'm using the Python-Apple-Support script in the examples, with the version number modified for 3.4.0. Thanks.

@levlam
Copy link
Contributor

levlam commented Dec 8, 2024

The Makefile has the line

CFLAGS-watchOS=-mwatchos-version-min=$(VERSION_MIN-watchOS)

which can be replaced with
CFLAGS-watchOS=-mwatchos-version-min=$(VERSION_MIN-watchOS) -DUSE_ATOMIC_FALLBACKS

@vincentneo
Copy link
Contributor Author

Thanks will do

@vincentneo
Copy link
Contributor Author

vincentneo commented Dec 8, 2024

Looks like that solves it - the minimal example didn't crash

Will build TDLib 1.8.41 again, if it works fine I will send in a pull request for the patch file (update: yes it worked)

@levlam
Copy link
Contributor

levlam commented Dec 8, 2024

Thank you. Now we have enough data to be sure that this is an OpenSSL 3.4.0 bug and what causes it. OpenSSL introduced a new hashtable implementation this year, which didn't exist in 3.3.0 and was merged to 3.4.0. The crash happens on the first access to the hashtable, so I don't think that the specific called OpenSSL method matters. It is likely that the implementation of the hashtable is incorrect for arm64_32, which wasn't catched by regular OpenSSL tests. Given, the position of the crash it looks like the bug is actually in atomics implementation in the OpenSSL. Namely, in the function apple_atomic_load_n_pvoid introduced in the commit f5b5a35c84626823364b0c8535b968c106690a56. The bug affects only arm64_32.

Now we need to create an issue at https://github.com/openssl/openssl/issues. Do you want me to describe the issue to them or do you want to do this yourself?

@vincentneo
Copy link
Contributor Author

Ah I see. Thanks for the explanation.

I think you understand the problem better than I do, so I think it would be better if you describe the issue to them. If subsequently they need any more data then I can help to provide.

@vincentneo
Copy link
Contributor Author

vincentneo commented Dec 15, 2024

Closing this as issue is fixed with openssl/openssl@038b0f6

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 a pull request may close this issue.

2 participants