noxxi / p5-io-socket-ssl

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

syswrite does not properly report SSL_ERROR_SYSCALL #62

Closed klenin closed 7 years ago

klenin commented 7 years ago

Documentation states (http://search.cpan.org/~sullr/IO-Socket-SSL-2.050/lib/IO/Socket/SSL.pod#syswrite):

syswrite will write all the data within a single SSL frame, which means, that no more than 16.384 bytes, which is the maximum size of an SSL frame, can be written at once.

There are two issues here. First, it is unclear whether a call with a length of more than 16384 should result in an error or a partial write. Second, it is actually an error, but it is not reported very well.

syswrite calls _generic_write which (in case of non-blocking socket) calls Net::SSLeay::write_partial. When write_partial returns error, _skip_rw_error checks for ERROR_WANT_READ and ERROR_WANT_READ, but silently discards all other errors.

As a result, syswrite returns undef but does not set $!, which confuses callers. In particular, see libwww-perl/libwww-perl#264.

Without digging deeper into code, my first proposal would be to report all SSL error codes, not only whose corresponding to $!{EWOULDBLOCK}.

noxxi commented 7 years ago

As for the documentation:
syswrite behaves similar to syswrite without SSL: if it cannot write everything at once it will write part of it and return the number of bytes written. The additional documentation regarding SSL frame size just adds to this that one should not expect that syswrite will ever write more than the maximum SSL frame size. This is similar to that one should not expect plain syswrite to write more than the socket buffer size. There is no need to document a non-existing error handling in case the given length is larger here since it is already said explicitly that the actually written bytes might be less than the given length.

As for the second problem:
IO::Socket::SSL syswrite returns undef on error similar to IO::Socket::whatever::syswrite or CORE::syswrite. On SSL_ERROR_SYSCALL it will not reset but keep and report $! as set by the underlying library which for example propagates an EPIPE on a failed write. It is true that it relies on the underlying library to set errno properly and will not attempt to produce an error if SSL_ERROR_SYSCALL is returned but $! (i.e. errno) is not set. But, I'm curious in which situation such behavior happens because I don't expect this. Can you please create an example which triggers a SSL_ERROR_SYSCALL but will not result in setting $! so I can reproduce the problem and work around it?

klenin commented 7 years ago

There is no need to document a non-existing error handling

Ok, then I suggest removing "all the data" part to avoid future confusion: "syswrite will write a single SSL frame. So no more than 16.384 bytes, which is the maximum size of an SSL frame, can be written at once."

Here is an example (Windows 7, Strawberry Perl 5.20):

use HTTP::Request::Common;
use LWP::UserAgent;

my $ua = LWP::UserAgent->new;
my $request = $ua->post('https://google.com', [ zzz => 'z' x 200000 ]);
noxxi commented 7 years ago

Here is an example (Windows 7, Strawberry Perl 5.20):

This example did not work for me but I got the idea and created an example without LWP where I could reproduce the case. I'm not sure if this is yet another behavior change of OpenSSL 1.1.0, similar to the one worked around in 2.039 where it reported syscall error without setting $! on SSL_read. Anyway, I've also worked around this problem where it claims to be that a syscall failed without setting $! by setting $! explicitly to EPIPE. This workaround is released in 2.051.

Ok, then I suggest removing "all the data" part to avoid future confusion: ...

I've modified the documentation slightly in 137359e (not in 2.051 yet)

klenin commented 7 years ago

Thank you.