New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TlsSocket: Fix deprecated OpenSSL calls #600

Merged
merged 1 commit into from Jan 15, 2019

Conversation

Projects
None yet
2 participants
@fedux
Copy link
Contributor

fedux commented Jan 8, 2019

Since OpenSSL 1.1.0 we have:

  • ERR_remove_thread_state, ERR_remove_state: "They are now deprecated
    and do nothing".

  • ASN1_STRING_data: "This function is deprecated: applications should
    use ASN1_STRING_get0_data() instead".

@hugbug

This comment has been minimized.

Copy link
Member

hugbug commented Jan 8, 2019

Since the old code works fine in 1.1.0 (despite deprecation) what do these changes achieve?
Elimination of warnings?

@fedux

This comment has been minimized.

Copy link
Contributor Author

fedux commented Jan 8, 2019

Yes:

daemon/connect/TlsSocket.cpp:217:20: warning: ‘void ERR_remove_state(long unsigned int)’ is deprecated [-Wdeprecated-declarations]
  ERR_remove_state(0);
                    ^
In file included from /usr/include/openssl/e_os2.h:13,
                 from /usr/include/openssl/ssl.h:15,
                 from ./daemon/main/nzbget.h:259,
                 from daemon/connect/TlsSocket.cpp:21:
/usr/include/openssl/err.h:260:1: note: declared here
 DEPRECATEDIN_1_0_0(void ERR_remove_state(unsigned long pid))
 ^~~~~~~~~~~~~~~~~~
daemon/connect/TlsSocket.cpp: In member function ‘bool TlsSocket::ValidateCert()’:
daemon/connect/TlsSocket.cpp:575:57: warning: ‘unsigned char* ASN1_STRING_data(ASN1_STRING*)’ is deprecated [-Wdeprecated-declarations]
      certHost = (char*)ASN1_STRING_data(common_name_asn1);
                                                         ^
In file included from /usr/include/openssl/e_os2.h:13,
                 from /usr/include/openssl/ssl.h:15,
                 from ./daemon/main/nzbget.h:259,
                 from daemon/connect/TlsSocket.cpp:21:
/usr/include/openssl/asn1.h:554:1: note: declared here
 DEPRECATEDIN_1_1_0(unsigned char *ASN1_STRING_data(ASN1_STRING *x))
 ^~~~~~~~~~~~~~~~~~

@fedux fedux force-pushed the fedux:tls-pr branch from 35492a7 to 89d358b Jan 11, 2019

TlsSocket: Fix deprecated OpenSSL calls
Since OpenSSL 1.1.0 we have:

 - ERR_remove_thread_state, ERR_remove_state: "They are now deprecated
   and do nothing".

 - ASN1_STRING_data: "This function is deprecated: applications should
   use ASN1_STRING_get0_data() instead".

@fedux fedux force-pushed the fedux:tls-pr branch from 89d358b to 2f3ad4e Jan 12, 2019

@hugbug hugbug merged commit 8a59079 into nzbget:develop Jan 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hugbug

This comment has been minimized.

Copy link
Member

hugbug commented Jan 15, 2019

This PR has exposed a memory leak in NZBGet.

Apparently ERR_remove_state must be called from each thread which has used OpenSSL. However in NZBGet the function is called only when closing connection. Threads in NZBGet are short lived. One thread opens connection, downloads first article and terminates. Then another thread is created and it reuses the already active connection. Many many threads later the connection is closed and ERR_remove_state is executed, once (per connection).

If during download OpenSSL happens to create error queue in each thread then all of these queues remain unfreed.

To fix the issue every thread which might have to do anything with connections (certainly every download thread at least) must call that function.

I've found that topic where authors of libcurl encountered that issue as well:

Issue was filed yesterday, identifying yet another memory leak with libcurl using OpenSSL. I suppose it says something when we still after 17 years of working with the OpenSSL API keep finding ourselves leak memory by not understanding how to clean it up properly...

Well said, ha-ha.

I don't know if I should fix that or not. There is no simple and elegant fix for this. I'm certainly not going to put OpenSSL call into common thread termination code in Thread.cpp but I also don't want to call it from many other places which deal with connections.

@fedux

This comment has been minimized.

Copy link
Contributor Author

fedux commented Jan 15, 2019

On the bright side, there is no leak if you use OpenSSL >= 1.1.0 ;)

"They are now deprecated and do nothing, as the OpenSSL libraries now normally do all thread initialisation and deinitialisation automatically."

And from the 1.0.2 man page:

"ERR_remove_state() is available in all versions of SSLeay and OpenSSL. It was deprecated in OpenSSL 1.0.0 when ERR_remove_thread_state was introduced and thread IDs were introduced to identify threads instead of 'unsigned long'."

So, maybe ERR_remove_thread_state would help if openssl < 1.1.0 ?

fedux added a commit to fedux/nzbget that referenced this pull request Jan 15, 2019

nzbget#600: fixed deprecated OpenSSL calls
Since OpenSSL 1.1.0 we have:

 - ERR_remove_thread_state, ERR_remove_state: "They are now deprecated
   and do nothing".

 - ASN1_STRING_data: "This function is deprecated: applications should
   use ASN1_STRING_get0_data() instead".

@fedux fedux deleted the fedux:tls-pr branch Jan 15, 2019

@hugbug

This comment has been minimized.

Copy link
Member

hugbug commented Jan 15, 2019

ERR_remove_thread_state(nullptr) is equivalent to ERR_remove_state(0) - clears error queue of current thread. Challenges are the same - to find good place to call one of these functions.

Maybe I'll just pretend the issue doesn't exist 😉

@fedux

This comment has been minimized.

Copy link
Contributor Author

fedux commented Jan 15, 2019

nothing to see here.... next!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment