ERR: Adapt `ERR_raise()` calls after making `CRYPTO_malloc()` and friends report `ERR_R_MALLOC_FAILURE` by DDvO · Pull Request #19072 · openssl/openssl

DDvO · 2022-08-26T07:06:25Z

As requested in #14833 (comment), this ~~is an extension of #14833 with a further commit that~~ replaces
all ERR_raise(ERR_LIB_FOO, ERR_R_MALLOC_FAILURE) calls
by ERR_raise(ERR_LIB_FOO, ERR_R_CRYPTO_LIB).

It does this change also for SSLfatal() and RLAYERfatal() calls ~~, ECerr(), EVPerr(),~~ as well as for the (legacy engine) CAPIerr() and ATTICerr() calls.

On this occasion, it also adapts remnant ECerr() and EVPerr() calls in crypto/ to ERR_raise().

levitte

... etc etc etc. Do you see the pattern well enough?

levitte · 2022-08-26T07:13:54Z

crypto/asn1/a_d2i_fp.c

    b = BUF_MEM_new();
    if (b == NULL) {
-        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_BUF_LIB, since that's the function that was called

levitte · 2022-08-26T07:14:02Z

crypto/asn1/a_d2i_fp.c


            if (len + want < len || !BUF_MEM_grow_clean(b, len + want)) {
-                ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+                ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_BUF_LIB, since that's the function that was called

levitte · 2022-08-26T07:14:14Z

crypto/asn1/a_d2i_fp.c


                    if (!BUF_MEM_grow_clean(b, len + chunk)) {
-                        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+                        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_BUF_LIB, since that's the function that was called

levitte · 2022-08-26T07:14:48Z

crypto/asn1/a_i2d_fp.c

    n = ASN1_item_i2d(x, &b, it);
    if (b == NULL) {
-        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_ASN1_LIB, since that's the function that was called

levitte · 2022-08-26T07:16:26Z

crypto/asn1/a_int.c


    if (ASN1_STRING_set(ret, NULL, len) == 0) {
-        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_ASN1_LIB, since that's the function that was called

levitte · 2022-08-26T07:16:47Z

crypto/asn1/a_mbstr.c

        dest = ASN1_STRING_type_new(str_type);
        if (dest == NULL) {
-            ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+            ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_ASN1_LIB, since that's the function that was called

levitte · 2022-08-26T07:17:23Z

crypto/asn1/a_sign.c


    if (ctx == NULL) {
-        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_EVP_LIB, since that's the function that was called

levitte · 2022-08-26T07:18:05Z

crypto/asn1/a_sign.c


    if (ctx == NULL) {
-        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_EVP_LIB, since that's the function that was called

levitte · 2022-08-26T07:18:57Z

crypto/asn1/asn_mime.c

    bf = BIO_new(BIO_f_buffer());
    if (bf == NULL) {
-        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_BIO_LIB, since that's the function that was called

levitte · 2022-08-26T07:19:11Z

crypto/asn1/asn_mime.c


    if ((b64 = BIO_new(BIO_f_base64())) == NULL) {
-        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Should be ERR_R_BIO_LIB, since that's the function that was called

DDvO · 2022-08-26T07:33:19Z

... etc etc etc. Do you see the pattern well enough?

Sure, but replacing ERR_R_CRYPTO_LIB by something more precise (such as ERR_R_BIO_LIB) where applicable was not what was discussed/decided in #14833.

If we go for this extra preciseness, this will require manually checking all 1200+ changes and adapting them where needed. And the gain would be IMO minimal.

Are you sure to require this effort for little benefit?

t8m · 2022-08-26T07:36:26Z

Hmm, the *err() calls were supposed to be replaced with ERR_raise already. Could you perhaps do it too?

DDvO · 2022-08-26T08:08:03Z

Hmm, the *err() calls were supposed to be replaced with ERR_raise already. Could you perhaps do it too?

Ok, adapted remnant ECerr() and EVPerr() calls in to use ERR_raise() (in separate commit).

Looks like not much can be done in this regard on CAPIerr(), ATTICerr(), and RLAYERfatal()
and I suspect that there was a decision not to adapt them before because engines are anyway deprecated.

levitte · 2022-08-26T08:15:47Z

... etc etc etc. Do you see the pattern well enough?

Sure, but replacing ERR_R_CRYPTO_LIB by something more precise (such as ERR_R_BIO_LIB) where applicable was not what was discussed/decided in #14833.

Apparently, I wasn't clear enough there, but that was exactly a point I tried to raise. Fact is, it was mentioned already a few times that ERR_R_MALLOC_FAILURE was wrong to begin with in those instances, 'cause the reason we got NULL in those spots could be for some other reason. They should already have been ERR_R_{whatever}_LIB.

If we go for this extra preciseness, this will require manually checking all 1200+ changes and adapting them where needed. And the gain would be IMO minimal.

I'm well aware that this is a big job. This is a reason I continually disagreed with certain statements back in #14833.

levitte · 2022-08-26T08:18:20Z

Hmm, the *err() calls were supposed to be replaced with ERR_raise already. Could you perhaps do it too?

Ok, adapted remnant ECerr() and EVPerr() calls in to use ERR_raise() (in separate commit).

Good. Apparently, we missed some spots when converting *err() calls to ERR_raise...

Looks like not much can be done in this regard on CAPIerr(), ATTICerr(), and RLAYERfatal() and I suspect that there was a decision not to adapt them before because engines are anyway deprecated.

There were more reasons than that, it turned out to be technically difficult. I don't remember all the details, unfortunately.

t8m · 2022-08-26T08:36:20Z

SSLfatal and RLAYERfatal and the err calls in engines of course need to be kept.

DDvO · 2022-08-26T08:40:04Z

SSLfatal and RLAYERfatal and the err calls in engines of course need to be kept.

Fine.
What could be done in engine/ is to replace the remaining (and meanwhile ignored) *_F_* constants in such calls by 0, if this felt worth the effort.

t8m · 2022-08-26T08:45:43Z

SSLfatal and RLAYERfatal and the err calls in engines of course need to be kept.

Fine. What could be done in engine/ is to replace the remaining (and meanwhile ignored) *_F_* constants in such calls by 0, if this felt worth the effort.

IMO not worth it.

DDvO · 2022-08-26T08:57:30Z

... etc etc etc. Do you see the pattern well enough?

Sure, but replacing ERR_R_CRYPTO_LIB by something more precise (such as ERR_R_BIO_LIB) where applicable was not what was discussed/decided in #14833.

Apparently, I wasn't clear enough there, but that was exactly a point I tried to raise. Fact is, it was mentioned already a few times that ERR_R_MALLOC_FAILURE was wrong to begin with in those instances, 'cause the reason we got NULL in those spots could be for some other reason. They should already have been ERR_R_{whatever}_LIB.

I'm aware of that, and so I consider getting rid of the potentially pretty misleading ERR_R_MALLOC_FAILURE a definite gain, and this benefit was easy to achieve.
Yet the potentially extra preciseness replacing ERR_R_CRYPTO_LIB with something more specific where applicable is a different story - it takes a lot of effort for little further benefit.

In particular, I feel ERR_R_CRYPTO_LIB not misleading, since ASN1, BIO, and other subsystems are all part of libcrypto, and many people likely will interpret l ERR_R_CRYPTO_LIB to refer to any part of the crypto library.

If we go for this extra preciseness, this will require manually checking all 1200+ changes and adapting them where needed. And the gain would be IMO minimal.

I'm well aware that this is a big job. This is a reason I continually disagreed with certain statements back in #14833.

If the change you requested is still of sufficient value to you, I suggest getting an OTC decision (in particular regarding the reviewing effort) and then executing the change yourself in a follow-up PR.

DDvO · 2022-08-27T08:21:18Z

Rebased this PR on the meanwhile merge parent PR #14833.

DDvO · 2022-08-27T08:32:43Z

@levitte, a few more thoughts on the partial inaccuracy of simply using ERR_R_CRYPTO_LIB for the ERR_raise() calls adapted here:

The main virtue of this change is that the partly misleading ERR_R_MALLOC_FAILURE are gone.
Another pro of keeping (as you wanted) all the ERR_raise() calls is that existing tracing of the caller is maintained (though the con is that they are somewhat redundant and occupy a further entry in the error queue).
Even though in various cases for example ERR_R_BIO_LIB may be a little more to the point,
for debugging all information needed is present in the error report, namely the exact source location in the calling function. Everything else (including the calls made before, which most likely will involve some memory allocation) may be derived from that - and essentially without extra effort, since having a look at the code context in the respective source file will be needed anyway.

paulidale · 2022-08-29T00:07:10Z

crypto/asn1/a_bitstr.c

        c = OPENSSL_clear_realloc(a->data, a->length, w + 1);
        if (c == NULL) {
-            ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+            ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


Raised errors following a memory allocation call should just be removed IMO. The memory allocation call will report file and line number. Subsequently reporting that it was LIB_ASN1 isn't helpful. It's obviously ASN1 from the file name.

In this instance, we'd get an error on lines 162 and 164. Polluting the error stack.

Yeah, for the cases where there is an OPENSSL_*alloc() call immediately before the ERR_raise(ERR_LIB_FOO, ERR_R_CRYPTO_LIB) call with just the NULL check in between, I agree it will be better to drop the ERR_raise() call.

Though in the rather rare event of an malloc error it would not hurt to have the essentially duplicate error report (in particular since the size of the error queue should not be an issue here),
it would look a little weird and the about 400 now superfluous ERR_raise() calls just clog the source code and increase the binary footprint.

paulidale · 2022-08-29T00:07:55Z

crypto/asn1/a_d2i_fp.c

    b = BUF_MEM_new();
    if (b == NULL) {
-        ERR_raise(ERR_LIB_ASN1, ERR_R_MALLOC_FAILURE);
+        ERR_raise(ERR_LIB_ASN1, ERR_R_CRYPTO_LIB);


This error is potentially useful. It will tell us where the failing BUF_MEM function was called from.

openssl-machine · 2022-09-29T00:08:43Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

levitte · 2022-09-29T03:12:40Z

In particular, I feel ERR_R_CRYPTO_LIB not misleading, since ASN1, BIO, and other subsystems are all part of libcrypto, and many people likely will interpret l ERR_R_CRYPTO_LIB to refer to any part of the crypto library.

... while in reality, it is a utility "sub-system", all declared in <openssl/crypto.h>

If you think about it, though, the extended line of thinking from where you're going with this is that we can essentially forego almost all sub-system indicators (what the ERR sub-system calls "lib"), and all that will remain for things in libcrypto is ERR_LIB_CRYPTO / ERR_R_CRYPTO_LIB. This also means that we can forego all sub-system specific reason codes, and collect them all in one big table (most probably doing some unification while at it), all called ERR_R_something.

However, that's not at all how it all works now, and I definitely do not believe that it's a good thing to do some kind of incremental (almost sneaky) shift toward that.
Mind you, I don't disagree with such an end goal, quite the contrary! However, if we're going to change the ERR system at that fundamental level, then it should be made with an effort dedicated to that.

If the change you requested is still of sufficient value to you, I suggest getting an OTC decision (in particular regarding the reviewing effort) and then executing the change yourself in a follow-up PR.

I'm actually working out that PR now.

DDvO · 2022-09-29T08:43:19Z

Rebased to fix merge conflict

levitte · 2022-09-29T12:18:09Z

I'm actually working out that PR now.

See #19301

DDvO · 2022-09-29T13:21:31Z

Closing in favor or #19301 and #19302.

DDvO mentioned this pull request Aug 26, 2022

ERR: Make CRYPTO_malloc() and friends report ERR_R_MALLOC_FAILURE #14833

Closed

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 26, 2022

levitte requested changes Aug 26, 2022

View reviewed changes

DDvO force-pushed the malloc_err_reporting branch from f1a55cd to df57553 Compare August 27, 2022 08:20

DDvO requested a review from levitte August 27, 2022 09:27

DDvO added this to the 3.1.0 milestone Aug 27, 2022

DDvO added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: bug The issue/pr is/fixes a bug triaged: refactor The issue/pr requests/implements refactoring labels Aug 27, 2022

paulidale reviewed Aug 29, 2022

View reviewed changes

DDvO added 2 commits September 29, 2022 10:37

Replace foo(..., ERR_R_MALLOC_FAILURE) foo(..., ERR_R_CRYPTO_LIB)

049dc0d

ERR: replace remnant ECerr() and EVPerr() calls in crypto/

6a6e27b

DDvO force-pushed the malloc_err_reporting branch from df57553 to 6a6e27b Compare September 29, 2022 08:42

levitte mentioned this pull request Sep 29, 2022

Stop raising ERR_R_MALLOC_FAILURE in most places #19301

Closed

DDvO mentioned this pull request Sep 29, 2022

ERR: replace remnant ECerr() and EVPerr() calls in crypto/ #19302

Closed

DDvO closed this Sep 29, 2022

DDvO removed the approval: review pending This pull request needs review by a committer label Aug 28, 2025

Uh oh!

Conversation

DDvO commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO commented Aug 26, 2022

Uh oh!

t8m commented Aug 26, 2022

Uh oh!

DDvO commented Aug 26, 2022

Uh oh!

levitte commented Aug 26, 2022

Uh oh!

levitte commented Aug 26, 2022

Uh oh!

t8m commented Aug 26, 2022

Uh oh!

DDvO commented Aug 26, 2022

Uh oh!

t8m commented Aug 26, 2022

Uh oh!

DDvO commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Aug 27, 2022

Uh oh!

DDvO commented Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulidale Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented Sep 29, 2022

Uh oh!

levitte commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Sep 29, 2022

Uh oh!

levitte commented Sep 29, 2022

Uh oh!

DDvO commented Sep 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DDvO commented Aug 26, 2022 •

edited

Loading

DDvO commented Aug 26, 2022 •

edited

Loading

DDvO commented Aug 27, 2022 •

edited

Loading

paulidale Aug 29, 2022 •

edited

Loading

levitte commented Sep 29, 2022 •

edited

Loading