noxxi / p5-io-socket-ssl

IO::Socket::SSL Perl Module
36 stars 59 forks source link

new_from_fd() erases {io_socket_timeout} #7

Closed olegwtf closed 10 years ago

olegwtf commented 10 years ago

Don't really know is it a bug. Feel free to reject. Test example below:

use strict;
use IO::Socket;
use IO::Socket::SSL;

my $sock = IO::Socket::INET->new(PeerAddr => 'mail.ru', PeerPort => 443, Timeout => 5)
    or die $@;

warn ${*$sock}{io_socket_timeout}; # 5

my $ssl = IO::Socket::SSL->new_from_fd($sock, Timeout => 5)
    or die $@;

warn ${*$ssl}{io_socket_timeout}; # undef

I found this problem while used LWP::Protocol::connect. Here we got plain socket where {io_socket_timeout} has some value from LWP constructor: https://metacpan.org/source/BENNING/LWP-Protocol-connect-6.08/lib/LWP/Protocol/https/connect/Socket.pm#L16 And after this action {io_socket_timeout} for socket is undefined: https://metacpan.org/source/BENNING/LWP-Protocol-connect-6.08/lib/LWP/Protocol/https/connect/Socket.pm#L18 And here how LWP uses this value: https://metacpan.org/source/Net::HTTP::Methods#L290 So we may get select() with undefined timeout (and block forever): https://metacpan.org/source/Net::HTTP::Methods#L298

Maybe this needs to be documented (if this behaviour is correct).

noxxi commented 10 years ago

new_from_fd works on a file handle, not on a socket object. It uses new_from_fd from the super class (e.g.IO::Socket::INET) to create the socket object from the file handle - the timeout information are lost there. If you just want to upgrade an existing socket to SSL (which is the case in LWP::Protocol::connect) you should directly use start_SSL and not new_from_fd. Also, the Timeout given to start_SSL (or new_from_fd) will not be used as a general socket timeout but only as the timeout for the initial SSL handshake (this is documented) and will therefore not be stored in the io_socket_timeout attribute.

In 355fc38 I enhanced the documentation, so that the inner workings of new_from_fd are documented and it is explicily said, that start_SSL should be used to upgrade existing IO::Socket objects

olegwtf commented 10 years ago

Thanks! I'll redirect this information to the author of LWP::Protocol::connect