Implement ERR_raise() / ERR_raise_data() by levitte · Pull Request #9452 · openssl/openssl

levitte · 2019-07-24T12:23:09Z

ERR_raise() and ERR_raise_data is new functionality to raise errors, with this intended signature:

void ERR_raise(int lib, int reason);
void ERR_raise_data(int lib, int reason, const char *fmt, ...);

The intention with this is to have functionality that makes it easier to report errors with additional text, instead of the sometimes cumbersome error handling with ERR_set_error_data().

ERR_raise() and ERR_raise_data are implemented as macros which works with a few building blocks:

ERR_new() which simply allocates a new error record in the error queue.
ERR_set_debug() which sets debugging information in the current error record. "debugging information" are __FILE__, __LINE__ and __func__ or variants thereof (in reality, we use OPENSSL_FILE, OPENSSL_LINE and OPENSSL_FUNC, which are defined appropriately for the toolchain that builds this)
ERR_set_error() which sets the basic error information in the current error record; library number, reason code, and a format string followed by arguments that form the additional data.

All the FOOerr macros are reimplemented in terms of ERR_raise() / ERR_raise_data, including FUNCerr, which also means that ERR_put_func_error() lost its meaning.

This functionality was briefly discussed in this thread on openssl-project

levitte · 2019-07-24T12:26:04Z

One commit mentions the following little program

#include <stdio.h>

void first(void)
{
    printf("Hello! ");
}

void foo(const char *bar)
{
    printf("%s", bar);
}

int main()
{
    /* This */
    (first(),foo)("cookie");
}

It shows the trick used to implement ERR_raise(). I have tested that little program on Linux (using gcc as well as clang), Windows (using MSVC) and VMS (using DEC C)

t8m · 2019-07-24T12:48:19Z

Dropping ERR_put_error() is an API break.

t8m · 2019-07-24T12:51:16Z

Dropping ERR_put_error() is an API break.

Ah, I did not notice that it is now implemented as macro. OK then.

richsalz

So this makes #9441 needless? Or do folks still want the unused argument removed?

I am concerned that this could add variable timing to errors and therefore be a timing oracle. I'd be delighted to be convinced that's needless, can you do that?

include/openssl/err.h

levitte · 2019-07-24T14:06:11Z

So this makes #9441 needless? Or do folks still want the unused argument removed?

Depends... do people still want to use the FOOerr / FOOerror type of macro, or will they turn to ERR_raise? I don't see this competing directly with #9441, but I agree that if we make ERR_raise the norm, then we have no more use of FOOerr (and concequently, FOOerror), they will merely become a historical note, and I understand how FOOerror would be pointless in that case.

I am concerned that this could add variable timing to errors and therefore be a timing oracle.

Only if you have additional data, which is a problem we already have with ERR_add_error_data

levitte · 2019-07-24T14:08:47Z

Regarding the timing oracle and additional error data... we do have a problem here, 'cause our error message are quite terse and non-descript at times, so there's a desire to add additional data, but the clumsiness of ERR_add_error_data has perhaps made that less encouraged.

richsalz · 2019-07-24T14:38:10Z

If this gets merged, then I will look at changing the script in #9441 to rewrite things in terms of ERR_raise Or I will close the PR because adding 43 macros that are only temporary "glue" is a bad idea.

crypto/err/err_blocks.c

crypto/err/err_locl.h

crypto/provider_core.c

doc/man3/ERR_new.pod

doc/man3/ERR_put_error.pod

doc/man7/provider-base.pod

include/openssl/err.h

paulidale · 2019-07-25T00:48:33Z

This looks like a good approach.

richsalz · 2019-07-25T03:31:03Z

crypto/err/err_locl.h

these are not time-critical, just make them internal functions. Then you don't have to worry about parenthesizing or someone calling err_clear_data(p, i++)

err_allocate is a misleading name since it doesn't actually allocate anything.

But it does allocate a new error record slot. Sure, it's in a ring buffer, but still. Allocation as a term covers more than merely allocating memory on the heap, doesn't it? I'm pretty sure I have seen that word used more broadly.

However, feel free to suggest a name that you find more suitable.

err_get_slot ?

Is the name changing for this one?

I tend to agree that (internal) functions might be a nicer choice.

crypto/property/property_parse.c

crypto/err/err_blocks.c

levitte · 2019-07-25T06:23:24Z

I've a question... Right now, this PR implements ERR_raise, which always requires a format string or NULL, i.e. you have to call it like this at a minimum:

    ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE, NULL);

Is that dangling NULL ok, or will people find that jarring? Should I then implement two variants, ERR_raise that simply takes two arguments, and ERR_raise_data that takes a formatting string and arguments? That would mean this simply case:

    ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);

and this for the more complex report:

    ERR_raise_data(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE, "name=%s", name);

t8m · 2019-07-25T07:23:57Z

I'd vote for the cleanliness approach with two functions. At least in case we expect to have many calls that do not need to supply any additional data - and I suppose it would be so because currently almost all the calls do not supply it.

beldmit · 2019-07-25T07:37:54Z

Sorry for brevity, can't browse the PR.

I'd prefer to have not an integer library code but a string one.

levitte · 2019-07-25T08:28:22Z

@beldmit, there are associated strings with the library codes, and for those who want to quickly check what library an error came from, having to strcmp is a bit overkill...

(plus, that would be a fairly serious API break)

levitte · 2019-07-25T08:58:50Z

@t8m, that's what I was thinking. So ok, I'll make that change.

paulidale · 2019-07-25T12:02:45Z

We've got a fast string to small integer map, so converting from a string library name to an integer one wouldn't be a major imposition (plus these are errors which should be uncommon). I agree with @beldmit that it would be nicer.

Doing the same for the reason would be trickier (typos) but it would sidestep the one vs two API question.

I'd love to see: ERR_raise(const char *library, const char *reason, ...) with printf formatting escapes in the reason. A quixotic dream I know.

levitte · 2019-07-25T13:02:26Z

We've got a fast string to small integer map, so converting from a string library name to an integer one wouldn't be a major imposition (plus these are errors which should be uncommon). I agree with @beldmit that it would be nicer.

That will still require that the caller does something more than just compare ERR_LIB(num) == ERR_LIB_RSA... the namemap is intended for dynamically generated numeric identities, so they will somehow have to find the number for "RSA" before they can compare. That puts a requirement on our users that doesn't quite fit with the idea of being source API compatible with pre-3.0 for things that exist in pre-3.0

levitte · 2019-07-25T13:03:15Z

In other words, I believe that the library and reason as strings is for some OpenSSL > 3.0.

levitte · 2019-07-25T15:58:10Z

Meanwhile, the CIs are finally happy

paulidale

If the name isn't changing and the macros aren't refactored into functions....

paulidale · 2019-07-30T03:05:07Z

crypto/err/err_locl.h

Is the name changing for this one?

I tend to agree that (internal) functions might be a nicer choice.

To deallocate the err_data field and then allocating it again might be a waste of processing, but may also be a source of errors when memory is scarce. While we normally tolerate that, the ERR sub-system is an exception and we need to pay closer attention to how we handle memory. This adds a new err_data flag, ERR_TXT_IGNORE, which means that even if there is err_data memory allocated, its contents should be ignored. Deallocation of the err_data field is much more selective, aand should only happen when ERR_free_state() is called. Fixes openssl#9458 Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#9459)

levitte · 2019-07-30T05:10:59Z

If the name isn't changing and the macros aren't refactored into functions....

Considering other ERR branches just went in, I need to rebase and refactor a little bit anyway, so...

You are dead to me now :P

paulidale · 2019-07-30T05:32:01Z

I'll wait for the new version.

New header file, include/openssl/macros.h, which contains diverse useful macros that we use elsewhere. We also add the new macro OPENSSL_FUNC, which is an alias for __FUNC__, __FUNCTION__, __FUNCSIG or __func__, depending on what the compiler supports. In the worst case, it's an alias for the string "(unknown function)".

The useful inner macros are now static inline functions. That will make them easier to debug in the future.

The new building block are ERR_new(), ERR_set_debug(), ERR_set_error(), ERR_vset_error(), which allocate a new error record and set the diverse data in them. They are designed in such a way that it's reasonably easy to create macros that use all of them but then rely completely on the function signature of ERR_set_error() or ERR_vset_error().

This macro uses a trick in C. The following is permitted: #include <stdio.h> void first(void) { printf("Hello! "); } void foo(const char *bar) { printf("%s", bar); } int main() { /* This */ (first(),foo)("cookie"); } ERR_raise() can be used to implement FUNCerr() as well, which takes away the need for the special function ERR_put_func_error(). squash! ERR: Implement the macro ERR_raise() and use it Make sure to mention ERR_raise_data() too

… macro Also, deprecate ERR_put_error()

The core now supplies its own versions of ERR_new(), ERR_set_debug() and ERR_vset_error(). This should suffice for a provider to have any OpenSSL compatible functionlity it desires. The main difference between the ERR functions and the core counterparts is that the core counterparts take an OSSL_PROVIDER parameter instead of the library number. That way, providers do not need to know what number they have been assigned, that information stays in the core.

If compiled with 'no-deprecated', ERR_put_error() is undefined. We had one spot where we were using it directly, because the file and line information was passed from elsewhere. Fortunately, it's possible to use ERR_raise() for that situation, and call ERR_set_debug() immediately after and thereby override the information that ERR_raise() stored in the error record. squash! Avoid using ERR_put_error() directly in OpenSSL code util/mkerr.pl needed a small adjustment to not generate code that won't compile in a 'no-deprecated' configuration.

richsalz · 2019-07-30T19:24:17Z

If you want two functions, make the "shorter" one a macro:

#define ERR_raise(l, r) ERR_raise_data((l), (r), NULL)

levitte · 2019-07-30T19:31:21Z

If you want two functions, make the "shorter" one a macro:
#define ERR_raise(l, r) ERR_raise_data((l), (r), NULL)

Isn't that exactly what I've done?

https://github.com/openssl/openssl/pull/9452/files#diff-9a5faf9ca0ffd5af12ebe8ea072ff981R249

richsalz · 2019-07-30T19:34:56Z

Oh yes, you did that, sorry. I was reading the comment threads to see what I’d have to do for #9441.

levitte · 2019-07-31T04:47:01Z

Merged.

e039ca3 Move some macros from include/openssl/opensslconf.h.in, add OPENSSL_FUNC
8a4dc42 ERR: refactor useful inner macros to err_locl.h. Add function name field
7c0e20d ERR: Add new building blocks for reporting errors
ed57f7f ERR: Implement the macros ERR_raise() and ERR_raise_data() and use them
add8c8e ERR: Remove ERR_put_func_error() and reimplement ERR_put_error() as a macro
49c6434 Refactor provider support for reporting errors
036913b Adapt the FIPS provider to use the new core error functions
c361297 Avoid using ERR_put_error() directly in OpenSSL code

New header file, include/openssl/macros.h, which contains diverse useful macros that we use elsewhere. We also add the new macro OPENSSL_FUNC, which is an alias for __FUNC__, __FUNCTION__, __FUNCSIG or __func__, depending on what the compiler supports. In the worst case, it's an alias for the string "(unknown function)". Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

The useful inner macros are now static inline functions. That will make them easier to debug in the future. Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

The new building block are ERR_new(), ERR_set_debug(), ERR_set_error(), ERR_vset_error(), which allocate a new error record and set the diverse data in them. They are designed in such a way that it's reasonably easy to create macros that use all of them but then rely completely on the function signature of ERR_set_error() or ERR_vset_error(). Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

The ERR_raise() macro uses a trick in C. The following is permitted: #include <stdio.h> void first(void) { printf("Hello! "); } void foo(const char *bar) { printf("%s", bar); } int main() { /* This */ (first(),foo)("cookie"); } ERR_raise_data() can be used to implement FUNCerr() as well, which takes away the need for the special function ERR_put_func_error(). Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

… macro Also, deprecate ERR_put_error() Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

The core now supplies its own versions of ERR_new(), ERR_set_debug() and ERR_vset_error(). This should suffice for a provider to have any OpenSSL compatible functionlity it desires. The main difference between the ERR functions and the core counterparts is that the core counterparts take an OSSL_PROVIDER parameter instead of the library number. That way, providers do not need to know what number they have been assigned, that information stays in the core. Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

If compiled with 'no-deprecated', ERR_put_error() is undefined. We had one spot where we were using it directly, because the file and line information was passed from elsewhere. Fortunately, it's possible to use ERR_raise() for that situation, and call ERR_set_debug() immediately after and thereby override the information that ERR_raise() stored in the error record. util/mkerr.pl needed a small adjustment to not generate code that won't compile in a 'no-deprecated' configuration. Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

levitte added the branch: master Applies to master branch label Jul 24, 2019

levitte requested review from mattcaswell and paulidale July 24, 2019 12:23

richsalz reviewed Jul 24, 2019

View reviewed changes

include/openssl/err.h Outdated Show resolved Hide resolved

paulidale reviewed Jul 25, 2019

View reviewed changes

richsalz reviewed Jul 25, 2019

View reviewed changes

levitte force-pushed the raise-error branch from f1ed571 to 2af4382 Compare July 25, 2019 09:11

levitte changed the title ~~Implement ERR_raise()~~ Implement ERR_raise() / ERR_raise_data() Jul 25, 2019

richsalz mentioned this pull request Jul 25, 2019

re-use allocated space in error structure #9458

Closed

levitte mentioned this pull request Jul 25, 2019

Make ERR_STATE opaque #9082

Closed

paulidale previously approved these changes Jul 30, 2019

View reviewed changes

levitte added 9 commits July 30, 2019 09:01

ERR: refactor useful inner macros to err_locl.h. Add function name field

ba480be

The useful inner macros are now static inline functions. That will make them easier to debug in the future.

ERR: Remove ERR_put_func_error() and reimplement ERR_put_error() as a…

feec0db

… macro Also, deprecate ERR_put_error()

Adapt the FIPS provider to use the new core error functions

a6d30e1

fixup! ERR: Add new building blocks for reporting errors

d2b8aab

levitte requested a review from paulidale July 30, 2019 08:17

levitte force-pushed the raise-error branch from c06f5dc to d2b8aab Compare July 30, 2019 08:17

richsalz mentioned this pull request Jul 30, 2019

Add script to convert XXXerr() to XXXerror #9441

Closed

paulidale approved these changes Jul 31, 2019

View reviewed changes

levitte closed this Jul 31, 2019

levitte added a commit that referenced this pull request Jul 31, 2019

ERR: Remove ERR_put_func_error() and reimplement ERR_put_error() as a…

add8c8e

… macro Also, deprecate ERR_put_error() Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

levitte added a commit that referenced this pull request Jul 31, 2019

Adapt the FIPS provider to use the new core error functions

036913b

Reviewed-by: Paul Dale <[email protected]> (Merged from #9452)

richsalz mentioned this pull request Jul 31, 2019

Replace FUNCerr with ERR_raise_data #9496

Closed

levitte mentioned this pull request Jun 27, 2020

Completely remove "func" from ERR state. #12289

Closed

Uh oh!

Conversation

levitte commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t8m commented Jul 24, 2019

Uh oh!

t8m commented Jul 24, 2019

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

levitte commented Jul 24, 2019

Uh oh!

levitte commented Jul 24, 2019

Uh oh!

richsalz commented Jul 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulidale commented Jul 25, 2019

Uh oh!

richsalz Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

richsalz Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

levitte Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

richsalz Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

levitte Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

paulidale Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

levitte commented Jul 25, 2019

Uh oh!

t8m commented Jul 25, 2019

Uh oh!

beldmit commented Jul 25, 2019

Uh oh!

levitte commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Jul 25, 2019

Uh oh!

paulidale commented Jul 25, 2019

Uh oh!

levitte commented Jul 25, 2019

Uh oh!

levitte commented Jul 25, 2019

Uh oh!

levitte commented Jul 25, 2019

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

paulidale Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

levitte commented Jul 24, 2019 •

edited

Loading

levitte commented Jul 24, 2019 •

edited

Loading

levitte commented Jul 25, 2019 •

edited

Loading