Unclear how to free 'data' allocated in ERR_get_error_line_data()

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Unclear how to free 'data' allocated in ERR_get_error_line_data()

Adam M
Hi,

I'm reading the documentation for ERR_get_error_line_data() here:
http://www.openssl.org/docs/crypto/ERR_get_error.html

The comments say that 'data' is dynamically allocated with
OPENSSL_malloc() if the ERR_TXT_MALLOCED bit is set in 'flags'. I
presume this means that we need to call OPENSSL_free() to free 'data',
but the documentation isn't clear on that.

I'm running into two issues in this regard. For one, 'data' is a 'const
char*', but OPENSSL_free() takes a 'void*', so we get a type mismatch.
See my sample code here, which includes the compiler error message:
http://pastebin.com/VNdkwf0G

The second issue is that I've been looking around on the web (in
particular on Ohloh) for usage of ERR_get_error_line_data(), and no one
seems to be checking for the ERR_TXT_MALLOCED bit in 'flags'. Maybe
doing so isn't necessary, but the documentation seems to suggest that it
is.

Can someone please help clarify what exactly to do here?

Thanks,
Adam
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

RE: Unclear how to free 'data' allocated in ERR_get_error_line_data()

J. J. Farrell-2
In C:

    if ( data != NULL  &&  flags & ERR_TXT_STRING ) {

      PRINT(data);

      if ( flags & ERR_TXT_MALLOCED ) {
        OPENSSL_free((void *)data);
      }
    }

> From: Adam M [mailto:[hidden email]]
> Sent: Tuesday, January 28, 2014 5:47 PM
> To: [hidden email]
>
> I'm reading the documentation for ERR_get_error_line_data() here:
> http://www.openssl.org/docs/crypto/ERR_get_error.html
>
> The comments say that 'data' is dynamically allocated with
> OPENSSL_malloc() if the ERR_TXT_MALLOCED bit is set in 'flags'. I
> presume this means that we need to call OPENSSL_free() to free 'data',
> but the documentation isn't clear on that.
>
> I'm running into two issues in this regard. For one, 'data' is a 'const
> char*', but OPENSSL_free() takes a 'void*', so we get a type mismatch.
> See my sample code here, which includes the compiler error message:
> http://pastebin.com/VNdkwf0G
>
> The second issue is that I've been looking around on the web (in
> particular on Ohloh) for usage of ERR_get_error_line_data(), and no one
> seems to be checking for the ERR_TXT_MALLOCED bit in 'flags'. Maybe
> doing so isn't necessary, but the documentation seems to suggest that
> it
> is.
>
> Can someone please help clarify what exactly to do here?
>
> Thanks,
> Adam
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Unclear how to free 'data' allocated in ERR_get_error_line_data()

Dr. Stephen Henson
In reply to this post by Adam M
On Tue, Jan 28, 2014, Adam M wrote:

> Hi,
>
> I'm reading the documentation for ERR_get_error_line_data() here:
> http://www.openssl.org/docs/crypto/ERR_get_error.html
>
> The comments say that 'data' is dynamically allocated with
> OPENSSL_malloc() if the ERR_TXT_MALLOCED bit is set in 'flags'. I
> presume this means that we need to call OPENSSL_free() to free 'data',
> but the documentation isn't clear on that.
>
> I'm running into two issues in this regard. For one, 'data' is a 'const
> char*', but OPENSSL_free() takes a 'void*', so we get a type mismatch.
> See my sample code here, which includes the compiler error message:
> http://pastebin.com/VNdkwf0G
>
> The second issue is that I've been looking around on the web (in
> particular on Ohloh) for usage of ERR_get_error_line_data(), and no one
> seems to be checking for the ERR_TXT_MALLOCED bit in 'flags'. Maybe
> doing so isn't necessary, but the documentation seems to suggest that it
> is.
>
> Can someone please help clarify what exactly to do here?
>

You can see and example of how it is used internally in the library in
crypto/err/err_prn.c all you need to check is that ERR_TXT_STRING is set. The
flag ERR_TXT_MALLOCED is an internal flag which indicates whether the string
should be freed when the error queue is emptied: applications must not free the
string themselves.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Unclear how to free 'data' allocated in ERR_get_error_line_data()

Adam M
On Tue, Jan 28, 2014, at 01:41 PM, Dr. Stephen Henson wrote:

> On Tue, Jan 28, 2014, Adam M wrote:
>
> > Hi,
> >
> > I'm reading the documentation for ERR_get_error_line_data() here:
> > http://www.openssl.org/docs/crypto/ERR_get_error.html
> >
> > The comments say that 'data' is dynamically allocated with
> > OPENSSL_malloc() if the ERR_TXT_MALLOCED bit is set in 'flags'. I
> > presume this means that we need to call OPENSSL_free() to free 'data',
> > but the documentation isn't clear on that.
> >
> > I'm running into two issues in this regard. For one, 'data' is a 'const
> > char*', but OPENSSL_free() takes a 'void*', so we get a type mismatch.
> > See my sample code here, which includes the compiler error message:
> > http://pastebin.com/VNdkwf0G
> >
> > The second issue is that I've been looking around on the web (in
> > particular on Ohloh) for usage of ERR_get_error_line_data(), and no one
> > seems to be checking for the ERR_TXT_MALLOCED bit in 'flags'. Maybe
> > doing so isn't necessary, but the documentation seems to suggest that it
> > is.
> >
> > Can someone please help clarify what exactly to do here?
> >
>
> You can see and example of how it is used internally in the library in
> crypto/err/err_prn.c all you need to check is that ERR_TXT_STRING is set.
> The
> flag ERR_TXT_MALLOCED is an internal flag which indicates whether the
> string
> should be freed when the error queue is emptied: applications must not
> free the
> string themselves.
>
> Steve.
> --
> Dr Stephen N. Henson. OpenSSL project core developer.
> Commercial tech support now available see: http://www.openssl.org

Thanks for the clarification Steve. If I'm understanding this correctly,
then the pattern should be something like this:

do
{
  rc = ERR_get_error_line_data()
  if rc == 0 break
} while true
ERR_clear_error() // to free the memory that may have been allocated in
the error queue

Does this make sense? Is the extra call to ERR_clear_error() necessary
to clean up?

Thanks,
Adam
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

RE: Unclear how to free 'data' allocated in ERR_get_error_line_data()

J. J. Farrell-2
In reply to this post by Dr. Stephen Henson
> From: Dr. Stephen Henson [mailto:[hidden email]]
> Sent: Tuesday, January 28, 2014 6:41 PM
> On Tue, Jan 28, 2014, Adam M wrote:
>
> > Hi,
> >
> > I'm reading the documentation for ERR_get_error_line_data() here:
> > http://www.openssl.org/docs/crypto/ERR_get_error.html
> >
> > The comments say that 'data' is dynamically allocated with
> > OPENSSL_malloc() if the ERR_TXT_MALLOCED bit is set in 'flags'. I
> > presume this means that we need to call OPENSSL_free() to free
> 'data',
> > but the documentation isn't clear on that.
> >
> > I'm running into two issues in this regard. For one, 'data' is a
> 'const
> > char*', but OPENSSL_free() takes a 'void*', so we get a type
> mismatch.
> > See my sample code here, which includes the compiler error message:
> > http://pastebin.com/VNdkwf0G
> >
> > The second issue is that I've been looking around on the web (in
> > particular on Ohloh) for usage of ERR_get_error_line_data(), and no
> one
> > seems to be checking for the ERR_TXT_MALLOCED bit in 'flags'. Maybe
> > doing so isn't necessary, but the documentation seems to suggest that
> it
> > is.
> >
> > Can someone please help clarify what exactly to do here?
> >
>
> You can see and example of how it is used internally in the library in
> crypto/err/err_prn.c all you need to check is that ERR_TXT_STRING is
> set. The
> flag ERR_TXT_MALLOCED is an internal flag which indicates whether the
> string
> should be freed when the error queue is emptied: applications must not
> free the
> string themselves.
>
> Steve.

Surely ERR_get_error_line_data() passes ownership of the data to the caller, and removes it from the error queue? How would the library know to free it subsequently? I remember analyzing the source in some detail a while ago when I was wondering the same as Adam, and that's the conclusion I came to at least.
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Unclear how to free 'data' allocated in ERR_get_error_line_data()

Adam McLaurin
In reply to this post by J. J. Farrell-2
I suspect this will result in a double free bug, as I don't think memory
ownership of 'data' is actually passed back to the caller (which is why
it's 'const char**'). The error isn't really 'popped' from the queue -
the queue just gets some indexes adjusted but the structure itself seems
unmodified by ERR_get_error_line_data(). What is still moderately
unclear is exactly at what point the OpenSSL library goes in and cleans
out the error queue. My guess (as I said in my previous email) is that
the user should call ERR_clear_error() when the error queue becomes
empty, to actually go through and clean out the internal structures.
I'll let the OpenSSL experts clarify that, however. In any case, the
documentation could definitely be improved in this regard.

-Adam

On Tue, Jan 28, 2014, at 01:33 PM, Jeremy Farrell wrote:

> In C:
>
>     if ( data != NULL  &&  flags & ERR_TXT_STRING ) {
>
>       PRINT(data);
>
>       if ( flags & ERR_TXT_MALLOCED ) {
>         OPENSSL_free((void *)data);
>       }
>     }
>
> > From: Adam M [mailto:[hidden email]]
> > Sent: Tuesday, January 28, 2014 5:47 PM
> > To: [hidden email]
> >
> > I'm reading the documentation for ERR_get_error_line_data() here:
> > http://www.openssl.org/docs/crypto/ERR_get_error.html
> >
> > The comments say that 'data' is dynamically allocated with
> > OPENSSL_malloc() if the ERR_TXT_MALLOCED bit is set in 'flags'. I
> > presume this means that we need to call OPENSSL_free() to free 'data',
> > but the documentation isn't clear on that.
> >
> > I'm running into two issues in this regard. For one, 'data' is a 'const
> > char*', but OPENSSL_free() takes a 'void*', so we get a type mismatch.
> > See my sample code here, which includes the compiler error message:
> > http://pastebin.ct om/VNdkwf0G
> >
> > The second issue is that I've been looking around on the web (in
> > particular on Ohloh) for usage of ERR_get_error_line_data(), and no one
> > seems to be checking for the ERR_TXT_MALLOCED bit in 'flags'. Maybe
> > doing so isn't necessary, but the documentation seems to suggest that
> > it
> > is.
> >
> > Can someone please help clarify what exactly to do here?
> >
> > Thanks,
> > Adam
> ______________________________________________________________________
> OpenSSL Project                                 http://www.openssl.org
> User Support Mailing List                    [hidden email]
> Automated List Manager                           [hidden email]
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Unclear how to free 'data' allocated in ERR_get_error_line_data()

Dr. Stephen Henson
On Tue, Jan 28, 2014, Adam McLaurin wrote:

> I suspect this will result in a double free bug, as I don't think memory
> ownership of 'data' is actually passed back to the caller (which is why
> it's 'const char**'). The error isn't really 'popped' from the queue -
> the queue just gets some indexes adjusted but the structure itself seems
> unmodified by ERR_get_error_line_data(). What is still moderately
> unclear is exactly at what point the OpenSSL library goes in and cleans
> out the error queue. My guess (as I said in my previous email) is that
> the user should call ERR_clear_error() when the error queue becomes
> empty, to actually go through and clean out the internal structures.
> I'll let the OpenSSL experts clarify that, however. In any case, the
> documentation could definitely be improved in this regard.
>

Yes the documention is rather old and could be clearer.

I had to double check with the source to see what was happening. The functions
that retrieve errors all end up calling get_error_values in crypto/err/err.c .

Errors are stored in a per-thread circular buffer.

In the case of ERR_get_error_line_data:

If you don't retrieve the extra error data then it is freed immediately.

Otherwise you get an internal pointer into the error queue (which is why it is
const). The memory will be freed either when you clear the queue explicitly
with ERR_clear_error() or a new entry is added which overwrites the internal
extra data pointer.

Additionally when thread cleanup is performed using ERR_remove_thread_state()
the whole table for that thread is freed which includes any extra error data
which hasn't been already freed.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

RE: Unclear how to free 'data' allocated in ERR_get_error_line_data()

J. J. Farrell-2
> From: Dr. Stephen Henson [mailto:[hidden email]]
> Sent: Tuesday, January 28, 2014 10:19 PM
> To: [hidden email]
>
> On Tue, Jan 28, 2014, Adam McLaurin wrote:
>
> > I suspect this will result in a double free bug, as I don't think
> memory
> > ownership of 'data' is actually passed back to the caller (which is
> why
> > it's 'const char**'). The error isn't really 'popped' from the queue
> -
> > the queue just gets some indexes adjusted but the structure itself
> seems
> > unmodified by ERR_get_error_line_data(). What is still moderately
> > unclear is exactly at what point the OpenSSL library goes in and
> cleans
> > out the error queue. My guess (as I said in my previous email) is
> that
> > the user should call ERR_clear_error() when the error queue becomes
> > empty, to actually go through and clean out the internal structures.
> > I'll let the OpenSSL experts clarify that, however. In any case, the
> > documentation could definitely be improved in this regard.
>
> Yes the documention is rather old and could be clearer.
>
> I had to double check with the source to see what was happening. The
> functions
> that retrieve errors all end up calling get_error_values in
> crypto/err/err.c .
>
> Errors are stored in a per-thread circular buffer.
>
> In the case of ERR_get_error_line_data:
>
> If you don't retrieve the extra error data then it is freed
> immediately.
>
> Otherwise you get an internal pointer into the error queue (which is
> why it is
> const). The memory will be freed either when you clear the queue
> explicitly
> with ERR_clear_error() or a new entry is added which overwrites the
> internal
> extra data pointer.
>
> Additionally when thread cleanup is performed using
> ERR_remove_thread_state()
> the whole table for that thread is freed which includes any extra error
> data
> which hasn't been already freed.

Ugh. Thanks for checking Steve, that's rather different from the
understanding I'd built up. I suggest a quick fix to improve the
documentation would be simply to delete the sentence "If it has been
allocated by OPENSSL_malloc(), *flags&ERR_TXT_MALLOCED is true".
At the moment, that appears to be giving a hint that the caller must
free it, whereas it's actually an internal detail of no use to the
caller and rather dangerous for him to know.

If I remember correctly, few (if any) bits of code malloc the string
at the moment, so it's not currently a big issue in practice.
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Unclear how to free 'data' allocated in ERR_get_error_line_data()

Dr. Stephen Henson
On Tue, Jan 28, 2014, Jeremy Farrell wrote:

>
> Ugh. Thanks for checking Steve, that's rather different from the
> understanding I'd built up. I suggest a quick fix to improve the
> documentation would be simply to delete the sentence "If it has been
> allocated by OPENSSL_malloc(), *flags&ERR_TXT_MALLOCED is true".
> At the moment, that appears to be giving a hint that the caller must
> free it, whereas it's actually an internal detail of no use to the
> caller and rather dangerous for him to know.
>
> If I remember correctly, few (if any) bits of code malloc the string
> at the moment, so it's not currently a big issue in practice.

Actually just about everything mallocs the extra data string. It is almost
exclusively used used when additional data is added to an error code.

The code in question is ERR_add_error_data which calls ERR_add_error_vdata
which concatenates strings into a OPENSSL_malloc'ed buffer and then sets it
using ERR_set_error_data which includes ERR_TXT_MALLOCED.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

RE: Unclear how to free 'data' allocated in ERR_get_error_line_data()

J. J. Farrell-2
> From: Dr. Stephen Henson [mailto:[hidden email]]
> Sent: Wednesday, January 29, 2014 12:50 AM
> To: [hidden email]
>
> On Tue, Jan 28, 2014, Jeremy Farrell wrote:
>
> >
> > Ugh. Thanks for checking Steve, that's rather different from the
> > understanding I'd built up. I suggest a quick fix to improve the
> > documentation would be simply to delete the sentence "If it has been
> > allocated by OPENSSL_malloc(), *flags&ERR_TXT_MALLOCED is true".
> > At the moment, that appears to be giving a hint that the caller must
> > free it, whereas it's actually an internal detail of no use to the
> > caller and rather dangerous for him to know.
> >
> > If I remember correctly, few (if any) bits of code malloc the string
> > at the moment, so it's not currently a big issue in practice.
>
> Actually just about everything mallocs the extra data string. It is
> almost
> exclusively used used when additional data is added to an error code.
>
> The code in question is ERR_add_error_data which calls
> ERR_add_error_vdata
> which concatenates strings into a OPENSSL_malloc'ed buffer and then
> sets it
> using ERR_set_error_data which includes ERR_TXT_MALLOCED.
>
> Steve.

Man, my memory's getting worse than a ... a ... what d'you call it.
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

RE: Unclear how to free 'data' allocated in ERR_get_error_line_data()

J. J. Farrell-2
> From: Jeremy Farrell
> Sent: Wednesday, January 29, 2014 1:39 AM
>
> > From: Dr. Stephen Henson [mailto:[hidden email]]
> > Sent: Wednesday, January 29, 2014 12:50 AM
> > To: [hidden email]
> >
> > On Tue, Jan 28, 2014, Jeremy Farrell wrote:
> >
> > >
> > > Ugh. Thanks for checking Steve, that's rather different from the
> > > understanding I'd built up. I suggest a quick fix to improve the
> > > documentation would be simply to delete the sentence "If it has
> been
> > > allocated by OPENSSL_malloc(), *flags&ERR_TXT_MALLOCED is true".
> > > At the moment, that appears to be giving a hint that the caller
> must
> > > free it, whereas it's actually an internal detail of no use to the
> > > caller and rather dangerous for him to know.
> > >
> > > If I remember correctly, few (if any) bits of code malloc the
> string
> > > at the moment, so it's not currently a big issue in practice.
> >
> > Actually just about everything mallocs the extra data string. It is
> > almost
> > exclusively used used when additional data is added to an error code.
> >
> > The code in question is ERR_add_error_data which calls
> > ERR_add_error_vdata
> > which concatenates strings into a OPENSSL_malloc'ed buffer and then
> > sets it
> > using ERR_set_error_data which includes ERR_TXT_MALLOCED.
> >
> > Steve.
>
> Man, my memory's getting worse than a ... a ... what d'you call it.

And thanks for the quick doc fixes Steve, should prevent others making
my mistake.
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Unclear how to free 'data' allocated in ERR_get_error_line_data()

Adam M
In reply to this post by Dr. Stephen Henson
On Tue, Jan 28, 2014, at 05:18 PM, Dr. Stephen Henson wrote:

> On Tue, Jan 28, 2014, Adam McLaurin wrote:
>
> > I suspect this will result in a double free bug, as I don't think memory
> > ownership of 'data' is actually passed back to the caller (which is why
> > it's 'const char**'). The error isn't really 'popped' from the queue -
> > the queue just gets some indexes adjusted but the structure itself seems
> > unmodified by ERR_get_error_line_data(). What is still moderately
> > unclear is exactly at what point the OpenSSL library goes in and cleans
> > out the error queue. My guess (as I said in my previous email) is that
> > the user should call ERR_clear_error() when the error queue becomes
> > empty, to actually go through and clean out the internal structures.
> > I'll let the OpenSSL experts clarify that, however. In any case, the
> > documentation could definitely be improved in this regard.
> >
>
> Yes the documention is rather old and could be clearer.
>
> I had to double check with the source to see what was happening. The
> functions
> that retrieve errors all end up calling get_error_values in
> crypto/err/err.c .
>
> Errors are stored in a per-thread circular buffer.
>
> In the case of ERR_get_error_line_data:
>
> If you don't retrieve the extra error data then it is freed immediately.
>
> Otherwise you get an internal pointer into the error queue (which is why
> it is
> const). The memory will be freed either when you clear the queue
> explicitly
> with ERR_clear_error() or a new entry is added which overwrites the
> internal
> extra data pointer.
>
> Additionally when thread cleanup is performed using
> ERR_remove_thread_state()
> the whole table for that thread is freed which includes any extra error
> data
> which hasn't been already freed.
>
> Steve.
> --
> Dr Stephen N. Henson. OpenSSL project core developer.
> Commercial tech support now available see: http://www.openssl.org

Thanks, this makes it 100% clear what needs to be done here. I have to
wonder though how many double-free bugs exist out there in the wild due
to the misleading documentation.

In any case, thanks for updating the online docs so quickly!

-Adam
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

RE: Unclear how to free 'data' allocated in ERR_get_error_line_data()

J. J. Farrell-2
> From: Adam M [mailto:[hidden email]]
> Sent: Wednesday, January 29, 2014 2:56 AM
>
> On Tue, Jan 28, 2014, at 05:18 PM, Dr. Stephen Henson wrote:
> >
> > Yes the documention is rather old and could be clearer.
> >
> > I had to double check with the source to see what was happening.
> > ...
>
> Thanks, this makes it 100% clear what needs to be done here. I have to
> wonder though how many double-free bugs exist out there in the wild due
> to the misleading documentation.

Well I've stamped on one just before it escaped, thanks for raising
this. Having looked at the code now, it's nothing like what I remember;
I suspect I'm confused by some other "how is this API meant to work"
search. Perhaps I just got my bad understanding from the doc.

> In any case, thanks for updating the online docs so quickly!

Indeed, very clear now.
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
User Support Mailing List                    [hidden email]
Automated List Manager                           [hidden email]