[openssl.org #2470] [PATCH] Cygwin: Don't call ERR_remove_state from DllMain

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

[openssl.org #2470] [PATCH] Cygwin: Don't call ERR_remove_state from DllMain

Rich Salz via RT
Hi,

the below patch is against current CVS HEAD, but it should be applied
to all supported branches of OpenSSL, starting with 0.9.8.

On systems running on the Windows platform, there's a DllMain function
in crypto/cryptlib.c which always calls ERR_remove_state(0) if an
application thread exits.  While this may be ok for native Windows
applications, it's wrong for applications running under Cygwin, given
that Cygwin is a POSIX environment.

Not only that compliant applications are written so that they call
ERR_remove_state by themselves right before exiting from a thread, the
gratuitous ERR_remove_state call in DllMain also potentially crashes
threaded applications.

For examples see http://bugs.python.org/issue3947 and
http://cygwin.com/ml/cygwin/2008-11/msg00341.html

The latest openssl package in the Cygwin net distribution also has this
patch applied.


Thanks,
Corinna


Index: crypto/cryptlib.c
===================================================================
RCS file: /home/cvs/cvsroot/src/openssl/crypto/cryptlib.c,v
retrieving revision 1.85
diff -u -p -r1.85 cryptlib.c
--- crypto/cryptlib.c 19 Nov 2010 00:12:01 -0000 1.85
+++ crypto/cryptlib.c 16 Mar 2011 20:45:45 -0000
@@ -744,7 +744,9 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL,
  case DLL_THREAD_ATTACH:
  break;
  case DLL_THREAD_DETACH:
+#ifndef __CYGWIN__
  ERR_remove_state(0);
+#endif
  break;
  case DLL_PROCESS_DETACH:
  break;


--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

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

Re: [openssl.org #2470] [PATCH] Cygwin: Don't call ERR_remove_state from DllMain

Rich Salz via RT
On Mar 17 09:11, Corinna Vinschen via RT wrote:

> Hi,
>
> the below patch is against current CVS HEAD, but it should be applied
> to all supported branches of OpenSSL, starting with 0.9.8.
>
> On systems running on the Windows platform, there's a DllMain function
> in crypto/cryptlib.c which always calls ERR_remove_state(0) if an
> application thread exits.  While this may be ok for native Windows
> applications, it's wrong for applications running under Cygwin, given
> that Cygwin is a POSIX environment.
>
> Not only that compliant applications are written so that they call
> ERR_remove_state by themselves right before exiting from a thread, the
> gratuitous ERR_remove_state call in DllMain also potentially crashes
> threaded applications.
>
> For examples see http://bugs.python.org/issue3947 and
> http://cygwin.com/ml/cygwin/2008-11/msg00341.html
>
> The latest openssl package in the Cygwin net distribution also has this
> patch applied.

Here's the patch against latest CVS.  However, it would be nice if it
could be applied in the 0.9.8 branch as well.


Thanks,
Corinna


Index: crypto/cryptlib.c
===================================================================
RCS file: /home/cvs/cvsroot/src/openssl/crypto/cryptlib.c,v
retrieving revision 1.87
diff -u -p -r1.87 cryptlib.c
--- crypto/cryptlib.c 3 Feb 2011 23:12:01 -0000 1.87
+++ crypto/cryptlib.c 22 Mar 2011 11:02:45 -0000
@@ -208,7 +208,7 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL,
  case DLL_THREAD_ATTACH:
  break;
  case DLL_THREAD_DETACH:
-#ifndef OPENSSL_FIPS
+#if !defined(OPENSSL_FIPS) && !defined(__CYGWIN__)
  ERR_remove_state(0);
 #endif
  break;


--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat


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

Re: [openssl.org #2470] [PATCH] Cygwin: Don't call ERR_remove_state from DllMain

Corinna Vinschen
Ping?

On Mar 22 12:03, Corinna Vinschen via RT wrote:

> On Mar 17 09:11, Corinna Vinschen via RT wrote:
> > Hi,
> >
> > the below patch is against current CVS HEAD, but it should be applied
> > to all supported branches of OpenSSL, starting with 0.9.8.
> >
> > On systems running on the Windows platform, there's a DllMain function
> > in crypto/cryptlib.c which always calls ERR_remove_state(0) if an
> > application thread exits.  While this may be ok for native Windows
> > applications, it's wrong for applications running under Cygwin, given
> > that Cygwin is a POSIX environment.
> >
> > Not only that compliant applications are written so that they call
> > ERR_remove_state by themselves right before exiting from a thread, the
> > gratuitous ERR_remove_state call in DllMain also potentially crashes
> > threaded applications.
> >
> > For examples see http://bugs.python.org/issue3947 and
> > http://cygwin.com/ml/cygwin/2008-11/msg00341.html
> >
> > The latest openssl package in the Cygwin net distribution also has this
> > patch applied.
>
> Here's the patch against latest CVS.  However, it would be nice if it
> could be applied in the 0.9.8 branch as well.
>
>
> Thanks,
> Corinna
>
>
> Index: crypto/cryptlib.c
> ===================================================================
> RCS file: /home/cvs/cvsroot/src/openssl/crypto/cryptlib.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 cryptlib.c
> --- crypto/cryptlib.c 3 Feb 2011 23:12:01 -0000 1.87
> +++ crypto/cryptlib.c 22 Mar 2011 11:02:45 -0000
> @@ -208,7 +208,7 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL,
>   case DLL_THREAD_ATTACH:
>   break;
>   case DLL_THREAD_DETACH:
> -#ifndef OPENSSL_FIPS
> +#if !defined(OPENSSL_FIPS) && !defined(__CYGWIN__)
>   ERR_remove_state(0);
>  #endif
>   break;
>
>
> --
> Corinna Vinschen
> Cygwin Project Co-Leader
> Red Hat
>
>
> ______________________________________________________________________
> OpenSSL Project                                 http://www.openssl.org
> Development Mailing List                       [hidden email]
> Automated List Manager                           [hidden email]

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #2470] [PATCH] Cygwin: Don't call ERR_remove_state from DllMain

Rich Salz via RT
Ping?

On Mar 22 12:03, Corinna Vinschen via RT wrote:

> On Mar 17 09:11, Corinna Vinschen via RT wrote:
> > Hi,
> >
> > the below patch is against current CVS HEAD, but it should be applied
> > to all supported branches of OpenSSL, starting with 0.9.8.
> >
> > On systems running on the Windows platform, there's a DllMain function
> > in crypto/cryptlib.c which always calls ERR_remove_state(0) if an
> > application thread exits.  While this may be ok for native Windows
> > applications, it's wrong for applications running under Cygwin, given
> > that Cygwin is a POSIX environment.
> >
> > Not only that compliant applications are written so that they call
> > ERR_remove_state by themselves right before exiting from a thread, the
> > gratuitous ERR_remove_state call in DllMain also potentially crashes
> > threaded applications.
> >
> > For examples see http://bugs.python.org/issue3947 and
> > http://cygwin.com/ml/cygwin/2008-11/msg00341.html
> >
> > The latest openssl package in the Cygwin net distribution also has this
> > patch applied.
>
> Here's the patch against latest CVS.  However, it would be nice if it
> could be applied in the 0.9.8 branch as well.
>
>
> Thanks,
> Corinna
>
>
> Index: crypto/cryptlib.c
> ===================================================================
> RCS file: /home/cvs/cvsroot/src/openssl/crypto/cryptlib.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 cryptlib.c
> --- crypto/cryptlib.c 3 Feb 2011 23:12:01 -0000 1.87
> +++ crypto/cryptlib.c 22 Mar 2011 11:02:45 -0000
> @@ -208,7 +208,7 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL,
>   case DLL_THREAD_ATTACH:
>   break;
>   case DLL_THREAD_DETACH:
> -#ifndef OPENSSL_FIPS
> +#if !defined(OPENSSL_FIPS) && !defined(__CYGWIN__)
>   ERR_remove_state(0);
>  #endif
>   break;
>
>
> --
> Corinna Vinschen
> Cygwin Project Co-Leader
> Red Hat
>
>
> ______________________________________________________________________
> OpenSSL Project                                 http://www.openssl.org
> Development Mailing List                       [hidden email]
> Automated List Manager                           [hidden email]

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat


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

Re: [openssl.org #2470] [PATCH] Cygwin: Don't call ERR_remove_state from DllMain

JoelKatz
In reply to this post by Corinna Vinschen

Shouldn't this code be removed on all platforms? It seems that the issue
Cygwin is having could occur on any platform, perhaps it just happens
not to on Win32 with the default locking callbacks.

It should be obvious that calling any OpenSSL functions that require the
locking callbacks to be intact would be illegal at thread attach time.
That would give the application no opportunity to register the callbacks
or set up any data structures they require.

It seems the same argument should apply here. What if the application
has already torn down the structures required to make the callbacks
work? My understanding was that fundamentally, OpenSSL never ran any of
its own threads or 'magically' called its own functions so the
application had complete control over when OpenSSL functionality was
invoked. This allows the application to set up and tear down any of the
callback functions. This includes both the threaded functions and memory
management.

This seems a gratuitous violation of fundamental API design principles.

The documentation at
http://www.openssl.org/docs/crypto/ERR_remove_state.html
says: "Since error queue data structures are allocated automatically for
new threads, they must be freed when threads are terminated in order to
avoid memory leaks."

        So non-broken applications are already calling this function at a time
they know it is safe to do so. Why call it at a potentially dangerous
time outside of the control over the application that manages the
library and thread lifetimes?

DS

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

Re: [openssl.org #2470] [PATCH] Cygwin: Don't call ERR_remove_state from DllMain

Corinna Vinschen
On May 16 07:46, David Schwartz wrote:

>
> Shouldn't this code be removed on all platforms? It seems that the
> issue Cygwin is having could occur on any platform, perhaps it just
> happens not to on Win32 with the default locking callbacks.
>
> It should be obvious that calling any OpenSSL functions that require
> the locking callbacks to be intact would be illegal at thread attach
> time. That would give the application no opportunity to register the
> callbacks or set up any data structures they require.
>
> It seems the same argument should apply here. What if the
> application has already torn down the structures required to make
> the callbacks work? My understanding was that fundamentally, OpenSSL
> never ran any of its own threads or 'magically' called its own
> functions so the application had complete control over when OpenSSL
> functionality was invoked. This allows the application to set up and
> tear down any of the callback functions. This includes both the
> threaded functions and memory management.
>
> This seems a gratuitous violation of fundamental API design principles.
>
> The documentation at
> http://www.openssl.org/docs/crypto/ERR_remove_state.html
> says: "Since error queue data structures are allocated automatically
> for new threads, they must be freed when threads are terminated in
> order to avoid memory leaks."
>
> So non-broken applications are already calling this function at a
> time they know it is safe to do so. Why call it at a potentially
> dangerous time outside of the control over the application that
> manages the library and thread lifetimes?

I agree.  I'm not sure this will potentially crash on native Win32, too.
But I didn't want to break another platform so I just created a patch for
Cygwin.


Corinna

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #2470] [PATCH] Cygwin: Don't call ERR_remove_state from DllMain

Rich Salz via RT
In reply to this post by Corinna Vinschen
Ping 2?


It's 3 months since I asked to apply this simple patch.  Is something
wrong with the patch, apart from removing the call to ERR_remove_state
entirely, as discussed in
http://www.mail-archive.com/openssl-dev@.../msg29218.html ?


Corinna

On May 16 15:38, Corinna Vinschen wrote:

> Ping?
>
> On Mar 22 12:03, Corinna Vinschen via RT wrote:
> > On Mar 17 09:11, Corinna Vinschen via RT wrote:
> > > Hi,
> > >
> > > the below patch is against current CVS HEAD, but it should be applied
> > > to all supported branches of OpenSSL, starting with 0.9.8.
> > >
> > > On systems running on the Windows platform, there's a DllMain function
> > > in crypto/cryptlib.c which always calls ERR_remove_state(0) if an
> > > application thread exits.  While this may be ok for native Windows
> > > applications, it's wrong for applications running under Cygwin, given
> > > that Cygwin is a POSIX environment.
> > >
> > > Not only that compliant applications are written so that they call
> > > ERR_remove_state by themselves right before exiting from a thread, the
> > > gratuitous ERR_remove_state call in DllMain also potentially crashes
> > > threaded applications.
> > >
> > > For examples see http://bugs.python.org/issue3947 and
> > > http://cygwin.com/ml/cygwin/2008-11/msg00341.html
> > >
> > > The latest openssl package in the Cygwin net distribution also has this
> > > patch applied.
> >
> > Here's the patch against latest CVS.  However, it would be nice if it
> > could be applied in the 0.9.8 branch as well.
> >
> >
> > Thanks,
> > Corinna
> >
> >
> > Index: crypto/cryptlib.c
> > ===================================================================
> > RCS file: /home/cvs/cvsroot/src/openssl/crypto/cryptlib.c,v
> > retrieving revision 1.87
> > diff -u -p -r1.87 cryptlib.c
> > --- crypto/cryptlib.c 3 Feb 2011 23:12:01 -0000 1.87
> > +++ crypto/cryptlib.c 22 Mar 2011 11:02:45 -0000
> > @@ -208,7 +208,7 @@ BOOL WINAPI DllMain(HINSTANCE hinstDLL,
> >   case DLL_THREAD_ATTACH:
> >   break;
> >   case DLL_THREAD_DETACH:
> > -#ifndef OPENSSL_FIPS
> > +#if !defined(OPENSSL_FIPS) && !defined(__CYGWIN__)
> >   ERR_remove_state(0);
> >  #endif
> >   break;
> >
> >
> > --
> > Corinna Vinschen
> > Cygwin Project Co-Leader
> > Red Hat
> >
> >
> > ______________________________________________________________________
> > OpenSSL Project                                 http://www.openssl.org
> > Development Mailing List                       [hidden email]
> > Automated List Manager                           [hidden email]
>
> --
> Corinna Vinschen
> Cygwin Project Co-Leader
> Red Hat
> ______________________________________________________________________
> OpenSSL Project                                 http://www.openssl.org
> Development Mailing List                       [hidden email]
> Automated List Manager                           [hidden email]

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]