Useful to have a Unix socket-pair to communicate with
a forked process.
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Change-Id: Ic4ad1eee62b6d3b40a03bc8e59bce6e0e16efc28
For some reason we only checked for the closed
socket case as failure and returned 0 in that case,
for error cases, we returned 1. Likely the API
had been modified in the early days, but this
return code was left lopsided.
This meant that even when the handshake failed, we
still called readIncomingData or writeOutgoindData,
depending on whether we wanted to read or write,
causing a rare race-condition.
When a client (HTTP request) connects to a server,
it needs to send the request, right after the
SSL handshake. SSL_do_handshake could need data
from the socket to complete the handshake. In such
a case it returns WANT_READ. Unfortunately,
because we always called SSL_read, the missing data
could have arrived between the SSL_do_handshake call
and the SSL_read call (a rather short duration, to
be sure, but an open window all the same).
SSL_read would of course read said data from the
socket and, since it still needs to finish the
handshake, will buffer it. It then returns the very
same error that the SSL_do_handshake returned:
WANT_READ. Of course we will oblige by polling with
POLLIN, which will time out (there is no more data
to come, and the server is waiting for *our* request
and has nothing to send us).
The only way this deadlock could break if
SSL_do_handshake was called (which will consume
the buffered data, return 1 to indicate handshake
has completed). Since we wouldn't call it unless
and until we get POLLIN, per WANT_READ, which won't
happen in this case. And since SSL_read doesn't call
SSL_do_handshake either, the request times out and
that's the end of it.
The fix is to not call SSL_read when the handshake
isn't complete and needs more data, which we do now.
Fortunately, we have very few SSL clients, outside
of unit-tests. Most notably the WOPI client. But
even then it's not a heavily used connection and
might not even be SSL-enabled (for LAN servers).
Change-Id: I04fd3dae151904194f3d7579dbf8c671b2580ffb
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
ERR_get_error cleared the returned error,
which meant the subsequent call to print
the error in string form, using
ERR_print_errors_cb, didn't find anything
to report.
Change-Id: If131a8cc0d2c1d8bbf705ed38f144b38abf6c8c6
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
A corner case where read doesn't return 0
on a closed socket can result in a stuck
state where we attempt to read when we
do get ECONNRESET (which we didn't check
on reads).
This makes the interface of readIncomingData
the same as writeOutgoingData by returning
the actual return value of the read syscall.
So now we handle both return 0 as well as
error codes returned on failed calls.
The logic hasn't changed, just that now
we handle errors better and similar to the
write case.
Change-Id: I0b38a63da4e6c92a482948478d5d8d446e0b8b58
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
This enables code that was protected with
EnableExperimental in the socket logic (and one
case in DocBroker). These changes are now deemed
safe to enable permanently.
Change-Id: Ie62f5d7bd281ade90f38d654b51b104b8d1f14bc
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Without knowing whether the write succeeded
or failed, we cannot trust errno has our
error or some earlier and unrelated error.
This was caught when there were two sockets,
one of which disconnected. The write to the
disconnected one returned -1 and set errno
to ECONNRESET. We subsequently wrote to the
second socket, which succeeded. However,
because errno wasn't reset, and since
writeOutgoingData didn't return anything
to indicate the success, errno's ECONNRESET
value meant the second socket was also
disconnected, which was incorrect.
writeOutgoingData now returns the last return
value from writeData so we can make informed
decision as to whether to check errno or not.
Also, to avoid incorrecly propagating errno,
we now capture errno only when readData and
writeData return -1. This has the nice
side-effect that we reset errno to 0 when
no errors occur during our call.
Change-Id: I911b31390f37cc71938bc4a6ae75393dbf24bb9d
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Since both implementations are identical, there
really is no benefit to having two version.
Change-Id: I4a5288243291c0d5706df8e8870b918fab425317
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Since the hostname argument is passed
to both the base class of SslStreamSocket
and SSL_set_tlsext_host_name, and since
the base class's getter, also called
hostname(), is hidden by the argument,
we cannot move it.
An empty hostname can result in 403 Forbidden
from the server due to missing Server Name
Indication (SNI).
Change-Id: I27990f64f17ec3c81a4dd543a078807629cd0c20
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
And improved socket logging in general while
making them more consistent.
Change-Id: I1ed7f2561476ca5370af91079d5d616804396f8e
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
While SSL is handshaking, there can be no general
application data communication. During that early
stage of connecting we have data to send (the
request, headers, etc.) and so we poll on POLLOUT.
Naturally, we also always want to poll on POLLIN,
because we can never know when there is data to
read (especially true for web-sockets).
The problem is when SSL will not send data just
yet because it is handshaking. It is typically
waiting for handshake negotiation data to read,
so when we POLLOUT, poll immediately returns, but
writing (via SSL_write) fails with WANTS_READ
error. This goes on in a busy-loop until the
negotiation data is available for read and the
handshake is completed. Very inefficient.
The solution is to poll on whatever SSL needs
during the handshake, exclusively. Once the
handshake is complete, we poll on whatever we
need. However, SSL can renegotiate at any time,
so we also merge with what it needs.
In addition, we avoid the unnecessary read when
poll doesn't give us POLLIN in revents, since the
read will more likely than not fail (except in
the rare case when data becomes available in the
interim). Notice that SSL_read will return
SSL_WANTS_READ when there is no data, which
is misleading (since SSL isn't in need of data to
read at all, nor are we, for that matter).
Best not to do noisy reads unnecessarily.
These changes are disabled by default and can
be enabled via the experimental_features option.
Change-Id: I6a7ed7d871ed257b30062cc720a8b8c7acbab3b7
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
In 'debug' log-level we expect a detailed, but
still readable output. Having one area with
disproportionately large number of logs reduces
the overall utility of the log output.
This patch reduces a number of redundant log
entries, including errors that are already
logged. It also reduces the level of some
others from 'information' to 'debug' and
from 'debug' to 'trace'.
The goal is to make 'debug' level as useful as
possible to read the progress and be able to
understand what was going on, such that one is
able to decide which area to dig deeper into.
Then, trace level could be used to get more
insight into that area, if necessary. For
example, when investigating a test failure,
one first enables 'debug' logs and reads through.
Once a section between two debug entries is
identified as being of interest, enabling 'trace'
level logs becomes more productive as it's
now possible to easily reach the first DBG
entry and read through until the second one.
It's unfortunate that we don't have per-area
control for enabling/disabling logs, so it
is common to see more and more 'debug' log
entries added all around, making logs
less and less readable.
It is also a limitation of the levels we have
that we really only have 3 usable levels:
one, two, many. That is, 'information' for
the most important events, 'debug' for
technical details needed to investigate issues,
and 'trace' for everything else. ('warning'
and 'error' aren't really 'levels'; they have
semantics that makes them special-cases.)
So we have to avoid degrading one into the
other, or have differences without distinction.
If any of these entries are needed to be
displayed more frequently, changing them
back to 'debug' or even 'information' should
be done. Though for me they seem special
cases that don't benefit most log readings.
Change-Id: Id2c6a9dc027483b81a066b0b4b50a298c5eff449
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
For some servers we receive failure with HTTP 403 Forbidden in WOPI::CheckFileInfo
"Reason: The client software did not provide a hostname using Server
Name Indication (SNI), which is required to access this server"
fixes#2771 : https://github.com/CollaboraOnline/online/issues/2771
Signed-off-by: Szymon Kłos <szymon.klos@collabora.com>
Change-Id: I761b179580481f8882a4526c1d8be4f1c14ad929
Also clear its input buffer explicitly.
Change-Id: I8badbb96d98eaf10433a65fcfd13b0d6d5893594
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Ignore input in a somewhat gentler way.
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Change-Id: I758302dc3bb1aa87f9fbfa726f73f4b9339e08c2
...Thread.
Conflicts:
net/Socket.hpp
net/SslSocket.hpp
Signed-off-by: Dennis Francis <dennis.francis@collabora.com>
Change-Id: I92b8f4b52e7bd60b69305c1916eed8a14a4c1560
Most C and Posix API clobber errno. By failing to save
it immediately after invoking an API we risk simply
reporting the result of an arbitrary subsequent API call.
This adds LOG_SYS_ERRNO to take errno explicitly.
This is necessary because sometimes logging is not done
immediately after calling the function for which we
want to report errno. Similarly, log macros that log
errno need to save errno before calling any functions.
This is necessary as the argements might contain calls
that clobber errno.
This also converts some LOG_SYS entries to LOG_ERR
because there can be no relevant errno in that context
(f.e. in a catch clause).
A couple of LOG_ macros have been folded into others,
reducing redundancy.
Finally, both of these log macros append errno to the
log message, so there is little point in ending the
messages with a period.
Change-Id: Iecc656f67115fec78b65cad4e7c17a17623ecf43
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
This merges OpenSSL's poll events with ours.
Effectively, we now do a single poll when
there are reads and writes to be done,
regardless of the reason (i.e. SSL-specific
or application-specific).
Simpler code, and more efficient performance
by sharing code with http and reducing the
number of poll syscalls.
Change-Id: Ib329c7e76fccfdadc4a0783c1ad79c3eedcdd8f3
Signed-off-by: Ashod Nakashian <ashod.nakashian@collabora.co.uk>
Hopefully reasonably simple; we perturb the count in the poll to
avoid starving a seventh socket in a poll.
Change-Id: I1a39cc36b9599ffe82186b896c6fd91d792c4127
Signed-off-by: Michael Meeks <michael.meeks@collabora.com>
Sometimes kit process goes into a heavy processing state (or even hangs)
and is not able to report its memory usage. Thus we can't implement cleanup
of problematic kit processes based on memory information reported by kit.
By moving memory reporting to admin module we avoid this problem.
Change-Id: Icf274e3a3a97b33623a93f9d2dc1e640ad9b7d99
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/92752
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
LibreOffice core uses that, too, and we support an even more
restricted set of compilers.
Change-Id: I0d0e2c8608e323eb5ef0f35ee8c46d02ab49a745
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/92467
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Tor Lillqvist <tml@collabora.com>
This mends several problems from commit
5710c86323.
Change-Id: I1b29f29ca81679608a2692488fa1ef22b2e62dfd
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/92032
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Essentially we want to be able to separate low-level socket code
for eg. TCP vs. UDS, from Protocol handling: eg. WebSocketHandler
and client sessions themselves which handle and send messages
which now implement the simple MessageHandlerInterface.
Some helpful renaming too:
s/SocketHandlerInterface/ProtocolHandlerInterface/
Change-Id: I58092b5e0b5792fda47498fb2c875851eada461d
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/90138
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
assert()'s are no-op in the release builds, but we still want to see threading
problems in the log at least.
Change-Id: Idb02bb018e8f2d628a57ab570249613ad00bcff2
Michael Meeks
Ashod Nakashian
Andras Timar
Szymon Kłos
Dennis Francis
Gabriel Masei
Tor Lillqvist
Miklos Vajna
Jan Holesovsky