LMS-Community / slimserver

Server for Squeezebox and compatible players. This server is also called Lyrion Music Server.
https://lyrion.org
Other
1.15k stars 288 forks source link

Slim::Networking::SimpleAsync is blocking for HTTPS #261

Closed mavit closed 5 years ago

mavit commented 5 years ago

Symptoms

When browsing the web UI, everything freezes for some time when an attempt is made to load cover artwork from SoundCloud. Not only the web UI is blocked, but also the infra-red remote control for my Squeezebox 3.

Log file /var/log/squeezeboxserver/server.log says:

[19-06-12 12:56:32.2105] Slim::Web::ImageProxy::getImage (110) Get artwork for URL: imageproxy/https://i1.sndcdn.com/artworks-000135225682-stew2b-t500x500.jpg/image_25x25_o
[19-06-12 12:56:32.2108] Slim::Web::ImageProxy::__ANON__ (151) Found URL to get artwork: https://i1.sndcdn.com/artworks-000135225682-stew2b-t500x500.jpg
[19-06-12 12:56:32.2111] Slim::Networking::SimpleAsyncHTTP::_createHTTPRequest (110) GETing https://i1.sndcdn.com/artworks-000135225682-stew2b-t500x500.jpg
[19-06-12 12:56:32.2499] Slim::Networking::Async::connect (108) Connecting to i1.sndcdn.com:443
[19-06-12 12:57:09.7561] Slim::Networking::Async::connect (114) Failed to connect to i1.sndcdn.com:443, because
Slim::Networking::Async::Socket::HTTPS: SSL connect attempt failed with unknown errorerror:14077410:SSL routines:SSL23_GET_SERVER_HELLO:sslv3 alert handshake failure
[19-06-12 12:57:09.7567] Slim::Networking::SimpleAsyncHTTP::onError (229) Failed to connect to https://i1.sndcdn.com/artworks-000135225682-stew2b-t500x500.jpg (Connect timed out: )
[19-06-12 12:57:09.7573] Slim::Web::ImageProxy::_gotArtworkError (245) Artwork not found, returning 404: https://i1.sndcdn.com/artworks-000135225682-stew2b-t500x500.jpg

Environment

It seems that the SSL code in CentOS 6's Perl is too old to use TLS 1. I'm not too concerned about the images failing to load; CentOS 8 will be released shortly, and I will upgrade to that. It does seem a problem that a slow remote server could cause the entire LMS to block, however.

michaelherger commented 5 years ago

If that was true, then there would be a bigger issue than ImageProxy: it's SimpleAsyncHTTP doing the call. That's being used all over the place... What if you pasted that URL in to the "Tune In" input field in the web UI? Can't be played of course, but what would happen? Would the UI lock up, too?

mavit commented 5 years ago

What if you pasted that URL in to the "Tune In" input field in the web UI? Can't be played of course, but what would happen? Would the UI lock up, too?

Yes, the UI locks up in the same way.

I took a peek at Slim::Networking::Async::Socket::HTTP and noticed that it specially handles non-blocking, whereas Slim::Networking::Async::Socket::HTTPS does not.

michaelherger commented 5 years ago

Could you please give this change a try?

diff --git a/Slim/Networking/Async/Socket/HTTPS.pm b/Slim/Networking/Async/Socket/HTTPS.pm
index 00504af50..eddd97134 100644
--- a/Slim/Networking/Async/Socket/HTTPS.pm
+++ b/Slim/Networking/Async/Socket/HTTPS.pm
@@ -16,6 +16,15 @@ BEGIN {

 use base qw(Net::HTTPS Slim::Networking::Async::Socket);

+sub connect {
+       my $self = shift;
+
+       # set to non-blocking
+       Slim::Utils::Network::blocking( $self, 0 );
+
+       return $self->SUPER::connect(@_);
+}
+
 sub close {
        my $self = shift;
mavit commented 5 years ago

It doesn't help.

Stepping through with perl -d, the function that blocks is this one:

IO::Socket::SSL::connect_SSL(/usr/share/perl5/vendor_perl/IO/Socket/SSL.pm:389):
389:                    my $rv = Net::SSLeay::connect($ssl);

I tried upgrading to the latest Net::SSLeay and IO::Socket::SSL from CPAN, but that didn't help either.

mavit commented 5 years ago

Having Slim::Networking::Async::Socket::HTTPS inherit from Net::HTTPS::NB rather than Net::HTTPS solves the problem for me. Interestingly, this didn't make any difference before I upgraded Net::SSLeay and IO::Socket::SSL.

michaelherger commented 5 years ago

Just wanted to suggest you try Net::HTTPS::NB instead of Net::HTTP. Here's what I tried (which seems at least not to break here :-)):

diff --git a/Slim/Networking/Async/Socket/HTTPS.pm b/Slim/Networking/Async/Socket/HTTPS.pm
index 00504af50..e6a055a6a 100644
--- a/Slim/Networking/Async/Socket/HTTPS.pm
+++ b/Slim/Networking/Async/Socket/HTTPS.pm
@@ -14,7 +14,11 @@ BEGIN {
        use IO::Socket::SSL;
 }

-use base qw(Net::HTTPS Slim::Networking::Async::Socket);
+use base qw(Net::HTTPS::NB Slim::Networking::Async::Socket);

 sub close {
        my $self = shift;

What is your change looking like?

michaelherger commented 5 years ago

I think my sub new replacement actually doesn't change anything... but at least does not break things. The important part is using Net::HTTPS::NB instead of Net::HTTPS.