A related general question also here:

I wonder why malloc failures are not simply reported once and for all by CRYPTO_malloc() itself and instead the burden is put on the callers. There are presumably many instances like the ones fixed in this PR where a NULL pointer result is misinterpreted to be due to an out-of-memory condition, and very likely there are even much more cases where malloc failures are caught but not reported.

I wonder why malloc failures are not simply reported once and for all by CRYPTO_malloc() itself and instead the burden is put on the callers. There are presumably many instances like the ones fixed in this PR where a NULL pointer result is misinterpreted to be due to an out-of-memory condition, and very likely there are even much more cases where malloc failures are caught but not reported.

I suppose, in theory, a malloc failure could either be because the system is running low on memory, or the call to CRYPTO_malloc() was erroneous (e.g. because it incorrectly attempts to allocate a vast amount of memory). In the second case this kind of problem would be difficult to find if all you have is the error code in CRYPTO_malloc itself.

Thanks Matt for your response.

In my question I should have consistently written 'malloc failure' (and not partly written 'out-of-memory') because I was not after a distinction between specific failure reasons.
My aim was/is to understand why CRYTPO_malloc does not report the failure (no matter for what reason) in the OpenSSL error queue itself but relies on all callers to do so.
If CRYTPO_malloc did the reporting itself, this would relieve the callers from reporting the failure and eliminate the risk of callers doing wrong here (like not reporting or wrongly reporting the malloc failure). Moreover, CRYPTO_malloc could be more specific on the failure reason (simply because it should know better than its callers what exactly went wrong and why).

If we had an error API that let you specify the file and line number we could use that in malloc, since it gets those line/file as parameters.

But there's also the issue in that the ERR code uses malloc, and we need to avoid the loop.

If we had an error API that let you specify the file and line number we could use that in malloc, since it gets those line/file as parameters.

ERR_PUT_error()?

But there's also the issue in that the ERR code uses malloc, and we need to avoid the loop.

That's a point, Yet one could implement the ERR queue such that there is always an element pre-allocated&reserved for use by CRYPTO_malloc, such that in case this function adds an entry (possibly by using a specific function) there is no need to (recursively) allocate it.

With the approach I just sketched, of course the queue cell should be reserved for actual out-of-memory situations only (and not any other potential failures of CRYPTO_malloc). When this reserved cell has been filled, any further requests to add elements to the ERR queue should be ignored in case the queue is full and would need re-allocation - I think this would be acceptable because at least from the lib's point of view an out-of-memory condition would be fatal anyway.

Too late for 1.1.1 to change the ERR queue stuff. Can you copy the relevant comments about that into a new issue?

Sure, will do soon.
I see no urgent need here anyway; this was just a general thought for a useful improvement.
I just checked: currently there are 900 (!) ERR calls for the ERR_R_MALLOC_FAILURE reason.

I just checked: currently there are 900 (!) ERR calls for the ERR_R_MALLOC_FAILURE reason.

FYI: Many of them were added only recently:

fe1128d Fix last(?) batch of malloc-NULL places
f06080c Add missing error code when alloc-return-null
f90bc6c Add missing malloc-return-null instance
7fcdbd8 X509: add more error codes on malloc or sk_TYP_push failure
7de2b9c Set error code if alloc returns NULL
6b49b30 Prevent a possible recursion in ERR_get_state and fix the problem that was pointed out in commit aef84bb differently.
cdb10ba Set error code on alloc failures

My aim was/is to understand why CRYTPO_malloc does not report the failure (no matter for what reason) in the OpenSSL error queue itself but relies on all callers to do so.

IIRC, the entire discussion about the malloc error codes started way back in #5781, which triggered a sequence of pull requests. There was even a discussion in that thread about doing the reporting in CRYPTO_malloc() itself, using file and line information provided by the caller. I don't really remember, why that thought was discarded at that time.

Thanks a lot @mspncp for pointing out this related discussion and the number of recent commits.
A pity that all this work has been invested going IMHO in a non-optimal direction.
Yet the two misplaced ERR_R_MALLOC_FAILURE removed in this PR apparently were introduced earlier, by

425f3300072 (Dr. Stephen Henson 2015-09-27 13:42:04 +0100 157)

who should have known better that ...

• if there is actually a malloc failure in the functions called by PKCS12_SAFEBAG_create_pkcs8_encrypt(), the intermediate (ASN.1) functions already flag this failure with reason ERR_R_MALLOC_FAILURE, and thus it is is superfluous to do that again in PKCS12_SAFEBAG_create_pkcs8_encrypt()
• there are also other reasons for p8 and bag to get NULL assigned, and in these cases the reason ERR_R_MALLOC_FAILURE is simply wrong.

Further, more general discussion on this topic has been started in issue #6251.

What is the status of this PR? Can it be merged?

Good reminder. Looks like the actual PR got buried under the more general discussion that it brought up, which has then been moved to a new thread.
Yes, at least in my view this could be merged; there are also two reviews with positive outcome.

Pushed. Thanks.