crystal-community / jwt

JWT implementation in Crystal
MIT License
206 stars 24 forks source link

Memory leak in an underlying library #35

Closed marzhaev closed 3 years ago

marzhaev commented 3 years ago

Valgrind points to a memory leak in a dependency library - spider-gazelle/openssl_ext

==29088== 316,624 (215,880 direct, 100,744 indirect) bytes in 1,799 blocks are definitely lost in loss record 110 of 110
==29088==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29088==    by 0x5243F08: CRYPTO_zalloc (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==29088==    by 0x517513C: BIO_new (in /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1)
==29088==    by 0x2362D6: *OpenSSL::GETS_BIO#initialize<IO::Memory>:Nil (bio.cr:37)
==29088==    by 0x236296: *OpenSSL::GETS_BIO::new<IO::Memory>:OpenSSL::GETS_BIO (bio.cr:36)
==29088==    by 0x235D23: *OpenSSL::PKey::RSA::new<IO::Memory, Nil>:OpenSSL::PKey::RSA (rsa.cr:20)
==29088==    by 0x235C5D: *OpenSSL::PKey::RSA::new<String, Nil>:OpenSSL::PKey::RSA (rsa.cr:10)
==29088==    by 0x235C42: *OpenSSL::PKey::RSA::new<String>:OpenSSL::PKey::RSA (rsa.cr:9)
==29088==    by 0x16B515: __crystal_main (my_test.cr:18)

where my code is

loop do
  rsa = OpenSSL::PKey::RSA.new(MY_KEY_HERE)
end

Based on my experience, I believe to have narrowed it down to here. Unfortunately, I'm unqualified to deal with OpenSSL's BIO and spider-gazelle/openssl_ext repository has got Issues disabled.

I hope someone qualified enough will be here to help out.

stakach commented 3 years ago

Sorry, I didn't realise issues were disabled is it possible that the process is exiting before GC cleans up the objects?

GC seems to only clean up objects once the scope they were created in has disappeared https://github.com/crystal-lang/crystal/issues/10469

marzhaev commented 3 years ago

The issue was discovered in production and replicated in different ways. I don't believe it has got to do with crystal-lang/crystal#10469.

My fix for Crystal MongoDB driver could be found here. Scroll down to the very bottom to collection.cr. It may be tough to read the code, let me explain.

C binding inits 'reply' which is a C type and hence does not get GC-ed by Crystal automatically. If it were inside a Crystal type then we should have used Crystal's finalized to manually free memory.

The same may be happening with openssl_ext. Around line 20 we may be bypassing Crystal's GC and not freeing bio manually via LibCrypto.bio_free further down the code. Unfortunately, I have never been able to get acquainted with OpenSSL's API to dig further.

stakach commented 3 years ago

Great point! I can't imagine many people are too intimately acquainted with OpenSSL's API - I'll have a dig

stakach commented 3 years ago

sweet that is fixed in the v2.1.4 release