search

open-quantum-safe / liboqs

C library for prototyping and experimenting with quantum-resistant cryptography
https://openquantumsafe.org/
Other
1.82k stars 447 forks source link

Replace malloc/free with OPENSSL_malloc/OpenSSL_free #1823

Open songlingatpan opened 3 months ago

songlingatpan commented 3 months ago

In OpenSSL code, we can customize malloc/free functions with CRYPTO_set_mem_functions. in oqs provider, it also utilized OPENSSL_malloc/OPENSSL_free. so CRYPTO_set_mem_functions can be used to customize allocator. We should replace malloc/free with OPENSSL_malloc/OpenSSL_free in liboqs as well.

SWilson4 commented 2 months ago

We certainly could do this, but I don't think it's very high on our priority list given the amount of manual effort it would require and the minimal (to my knowledge) improvement to the library. However, if you would like to take a stab at it, please feel free to open a PR for us to review.

songlingatpan commented 2 weeks ago

@baentsch @SWilson4 I can help on this. However, I have a few questions.

  1. For the malloc and free from upstream code, if we change them to OPENSSL_malloc and OPENSSL_free, how do we maintain the code? We can write a script to replace malloc and free. Would you like to run the script during merge the code from upstream, or during build the code?
  2. Would you like to use OPENSSL_malloc and OPENSSL_free only when OQS_USE_OPENSSL = 1? or we can use it by default?
  3. I found quite some error handling issues, and memory leak after failure in both oqs and upstream code. Again, how do you maintain the code change to fix upstream issues? Thanks
baentsch commented 2 weeks ago

:> Would you like to run the script during merge the code from upstream, or during build the code?

My preference would be to do this during copy_from_upstreamsuch as for everyone to read what code gets built.

Would you like to use OPENSSL_malloc and OPENSSL_free only when OQS_USE_OPENSSL = 1? or we can use it by default?

Not by default. If OQS_USE_OPENSSL=0, the current, openssl-free build must still succeed. Therefore, I'd suggest you'd add 2 macros in step 1 and define those suitably depending on the setting of OQS_USE_OPENSSL.

I found quite some error handling issues, and memory leak after failure in both oqs and upstream code. Again, how do you maintain the code change to fix upstream issues?

That's a scary question but a good one. Our usual recommendation is to contribute any fixes you develop to the upstreams so they flow back into liboqs when we run copy_from_upstream. If this does not succeed, we can create local patches -- but those would need to be maintained (as and when the upstreams change) and someone would need to take responsibility for this maintenance: Would you be willing to take that on @songlingatpan ?

baentsch commented 2 weeks ago

I found quite some error handling issues, and memory leak after failure in both oqs and upstream code.

In addition to the comment around fixing things, what would be really helpful would be if you @songlingatpan would be willing to share (contribute by PR to our CI) testing unearthing these problems so we can track (resolutions, hopefully, too :).

SWilson4 commented 2 weeks ago

To the best of my knowledge, none of the upstreams from which we pull code automatically (PQClean, pq-crystals, MAYO, and CROSS) use malloc outside common code (SHA3, AES, etc). I don't think we have to worry about copy_from_upstream overwriting changes.