[3.0.0] Error data flags can carry over from one error to another

[sfackler]

Using the 3.0.0-alpha5 release, the flags component of an error is not properly cleared out from the error stack's internal storage. This results in errors created without any string data still reporting ERR_TXT_STRING and ERR_TXT_MALLOCED if the error's slot in the internal stack previously contained an error with malloced string data.

As an example, here's a simple C program:

#include <openssl/err.h>
#include <stdio.h>

int main() {
    int flags;

    // ERR_raise_data(0, 0, "hello %s", "world");
    // ERR_clear_error();

    ERR_raise(0, 0);
    ERR_get_error_data(NULL, &flags);

    printf("flags: %d\n", flags);
}

If you compile and run that, it prints flags: 0, as expected. However, if you uncomment the two lines that create the earlier error, it now prints flags: 3 (i.e. ERR_TXT_STRING | ERR_TXT_MALLOCED), which is incorrect. The end result is that code that looks at the error sees an empty string for its data, rather than no string at all.

It looks like this is the result of #9459, which avoids deallocating malloced data buffers to avoid allocator traffic. The logic should probably be adjusted to clear the flags unconditionally and track the fact that the buffer is still malloced somewhere else.

[richsalz]

I am not so sure this is an error because, internally, the field does have a malloc'd buffer. It just so happens that the buffer is empty.

The ERR stuff exposes too many internals, anyway. Since that is being deprecated, and it's not incorrect (although it is confusing), I would close this as "wontfix" because that code is going to vanish.

I do think, however that the ERR_TXT_xxx numbers should be moved to not collided with the ERR_FLAG_xxx values. i can do a PR for that if wanted.

[sfackler]

If you replace ERR_get_error_data(NULL, &flags) with ERR_get_error_all(NULL, NULL, NULL, NULL, &flags) you see exactly the same behavior, so I'm not sure how ERR_get_error_data's removal would change things here.

As a consumer of OpenSSL, I don't care if OpenSSL's internal error glue has a malloc'd buffer to cut down on allocator traffic, I care if there's an extra message string in the data component or not. It seems valuable to properly distinguish that for consumers.

[richsalz]

The issue for the ERR stuff right now is that the "internal error glue" is exposed in public header files. We missed this when we made most things opaque in a previous release. I can argue it's a feature that lets users safely manipulate internal state.

[sfackler]

Oh sure, I am definitely in favor of providing a minimal clean interface to the error state. This is just a case where the internal implementation details leak out into the public API in a way that could be avoided by e.g. creating a new internal-only flag for a cached allocation as you (I think) mentioned above.

[richsalz]

No, that idea of a new internal-only flag doesn't work, without creating a parallel structure to the current errinfo and keeping that private. While also populating the necessary fields out to the (unfortunately public) error state. The key question is given that much effort, why spend anything cleaning up an API that will be made opaque soon.

[sfackler]

If the data pointer is no longer going to be publicly exposed, then it seems reasonable to close this issue.

What will the AIP to pull errors look like once it's all cleaned up out of curiosity?

[richsalz]

I am not a member of the project, but I would expect all API's to work except for the data structure and the "get state" function being made opaque and removed, respectively.

[paulidale]

I've created a PR with the flags fix, making the structures opaque will have to wait until after 3.0.