TYPE_free() does not document that TYPE_free(NULL) does nothing about openssl HOT 17 OPEN

Documentation changes, mostly yes. Confirming that every single *_free() function properly handles NULL... less so.

All our free functions should properly handle NULL. If they don't that's a bug IMO. I have no problem with a generic statement somewhere saying that all free functions properly handle NULL. Perhaps a good place would be in the "Library conventions" section of this page:

https://www.openssl.org/docs/manmaster/man7/ossl-guide-libraries-introduction.html

from openssl.

No worries about pronouns. I've just opened a PR. The change is ready from our side (mine, Jordan's). The tests completed fine. They were run in CI pipeline yesterday in my forked repo.

from openssl.

It certainly seems like a reasonable idea, would you be willing to submit a patch for this?

from openssl.

Documentation changes, mostly yes. Confirming that every single *_free() function properly handles NULL... less so.

from openssl.

Yes, I'm referring specifically to documentation for the api calls you are interested in checking.

I'll assign this to you as a community issue

from openssl.

I've done the work, but I realized (and confirmed) that I would need to do corporate paperwork to contribute it. I'm looking into having somebody who has already done the paperwork take it over.

A central statement in a library conventions page would be good, but it should probably also be on each man page since that's where one will look first. It's already there on about half of them.

BTW, my inspection said that OPENSSL_INIT_free() and OSS_LIB_CTX_free() in FIPS-140 mode did not handle NULL.

from openssl.

BTW, my inspection said that OPENSSL_INIT_free() and OSS_LIB_CTX_free() in FIPS-140 mode did not handle NULL.

Interesting. They definitely should handle NULL. However OPENSSL_INIT_free() is not a public API function so this doesn't impact anything from a public docs perspective.

The FIPS version of OSSL_LIB_CTX_free() is also not the version of OSSL_LIB_CTX_free() which is the public API function. My analysis suggests that in any case it does (kind of accidentally) handle NULL correctly anyway. It should handle it more explicitly (ossl_lib_ctx_is_default(ctx) returns 0; this ctx->ischild bit isn't compiled in FIPS; context_deinit returns NULL if its passed NULL)

from openssl.

BTW, my inspection said that OPENSSL_INIT_free() and OSS_LIB_CTX_free() in FIPS-140 mode did not handle NULL.

OSSL_LIB_CTX_free() handles NULL even with FIPS_MODULE defined (which is purely internal code so does not matter anyway). The reason is that both context_deinit() and OPENSSL_free() handle NULL argument gracefully.

You're right that OPENSSL_INIT_free() needs to be fixed. (#24681)

from openssl.

Ah, indeed. I didn't look at it deeply enough.

It works in non-FIPS-140 mode because ossl_lib_ctx_is_default() returns true and so OSSL_LIB_CTX_free() returns before reaching the if (ctx->child).
It works in FIPS-140 mode because the if (ctx->child) is ifdef'ed out and then the underlying functions handle it as you say.

from openssl.

(Sigh, and that's exactly what Matt said, but I didn't notice that there were two comments.)

from openssl.

I derived my list of functions to inspect from the public documentation. OPENSSL_INIT_free() is documented here:
https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_INIT_free.html

from openssl.

make that: https://www.openssl.org/docs/manmaster/man3/OPENSSL_INIT_free.html

from openssl.

I derived my list of functions to inspect from the public documentation

Oh - you're right! I misread the code. Yeah that definitely needs fixing.

from openssl.

My coworker Vita Bátrla @batrla has already done the relevant corporate paperwork and will be taking over this change. Thanks, Vita!

from openssl.

@jordanbrown 0if you could have them add a comment here, i'm happy to reassign it

from openssl.

Hi Neil, I'm going to work with Jordan and get a PR filed (probably next week).

from openssl.

Thank you, and I apologize, I didn't even think about my pronouns above

from openssl.