I think the general policy should be to always log errno using both
Util::symbolicErrno() and std::strerror(), never log a naked errno.
But only in cases where we know that it is highly likely that it is
the most recent system call that has failed, so errno makes sense.
Change-Id: I4a1fb31e375ea949e7da17687464361efe7c1761
We had:
auto ipv4 = (struct sockaddr_in *)&clientInfo.sin_addr
even if the clientInfo variable itself was a struct sockaddr_in.
And then we also had:
auto ipv6 = (struct sockaddr_in6 *)&clientInfo.sin_addr
which makes even less sense.
Instead, make clientInfo into a struct sockaddr_in6, which is big
enough to also to be interpreted as a sockaddr_in. Pass the address of
the correct field, either sin_addr or sin6_addr, to inet_ntop(). (Note
that sin_addr in sockaddr_in has a different offset than sin6_addr in
sockaddr_in6.)
At least on my Fedora 28, when I connect using IPv4, accept4() still
returns the client address as an IPv4 mapped IPv6 address, that is
::ffff:192.168.1.113 for 192.168.1.113. So the sample
net.post_allow.host value in loolwsd.xml.in should probably be changed
to have that ::ffff: prefix, too.
Change-Id: I0ad774616b210d94b904982e2f7dc928adc879ed
It was inconsistent that the StreamSocket ctor had a const parameter,
but the SslStreamSocket had a non-const one and used std::move(). Use
the later in the parent class as well for consistency.
Change-Id: I58597b97432a801d164d6ed9d831a0372280e687
moreover:
* noCache is always true in debug mode
* when noCache is true we return an explicit "Cache-Control: no-cache" line
Change-Id: I157a410df0a90f9ab151b899e44566b95cbd9929
Reviewed-on: https://gerrit.libreoffice.org/54517
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Michael Meeks <michael.meeks@collabora.com>
This resolves the erroneous warnings of pinging
on a non-upgraded (i.e. HTTP) socket.
This was due to the fact that we moved the socket
from one SocketHandlerInterface to a WebSocketHandler
after upgrading and since the WSState was a property
of the handler, the WebSocketHandler didn't know
that the socket had already been upgraded.
Also other cosmetics and cleanups.
Change-Id: I1a88edef750117ed551d23245e49380371561422
Reviewed-on: https://gerrit.libreoffice.org/49911
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
And improve the logging support in unit-tests to
help troubleshoot issues faster and more accurately.
Also makes the code more readable (hopefully).
Change-Id: I4f8aafb5245e2f774b03231591a74544f9ec84aa
Reviewed-on: https://gerrit.libreoffice.org/48645
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Using null ssl context to set options is surely not a good idea:
unsigned long SSL_CTX_set_options(SSL_CTX *ctx, unsigned long op)
{
return ctx->options |= op;
}
Change-Id: I2700350e0c3928e372488c81b8111c9ab0b48e06
No derived classes override it, and if they would, that would be a
problem, as setNoDelay() is called from init(), which is called from the
Socket ctor. Calling virtual functions from the base class ctor is not a
good idea, since the object is not fully constructed yet.
Change-Id: I4993e26d09f5d3429c7e2afae7688b84c0061c9d
Two problem types:
- non-final class has virtual functions but no virtual dtor -> mark the
class final
- abstract class has no virtual dtor -> add a virtual dtor
Change-Id: Iae208b65c774e6da7a3dda5e725fe07d4d589e4f
With help from Valgrind to find and verify
these leaks.
Change-Id: I3afeed89dc4bcd714a222f81822144477a346fb0
Reviewed-on: https://gerrit.libreoffice.org/39464
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Thread-affinity checks must be inhibited
not just on Socket, but on the SocketPoll as well,
before destroying DocumentBroker instances.
Also, properly initialize the inhibit statics.
Change-Id: I2ced1554d477f0c3faf09bda74034cbae99e4ce1
Reviewed-on: https://gerrit.libreoffice.org/37608
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Dung out overlapping return enumerations. Move more work into 'move'
callbacks at a safer time, etc.
Change-Id: I62ba5a35f12073b7b9c8de4674be9dae519a8aca
HTTP HEAD verb requires sending back NO content.
Currently we don't support HEAD and that harms
the browser from using its cache where possible.
Change-Id: I3c67dc106df95312c73f6ae786b7b1657a4167fb
Reviewed-on: https://gerrit.libreoffice.org/36871
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This slows things down terribly, particularly the setting on the websocket
made tiles appearing one by one. Let's keep the possibility to zero the buffer
sizes for debugging, but hide that behind an env. variable (and in debug
builds only anyway).
Change-Id: Ie4d2cdb3c0ec3c50f1a2b4f9941a462ac4f2d196
The server correctly saves all documents
and waits to upload them before exiting.
Change-Id: I04dc9ce588bc0fa39a9deb298d0a5efa61a03f1a
Reviewed-on: https://gerrit.libreoffice.org/36654
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Without an explicit WS message, the client
does not get this message and the handler
is not invoked at all.
Change-Id: I71e210a9958965cff35dd4d0f1d99985429b82f4
Reviewed-on: https://gerrit.libreoffice.org/36593
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Apparently pinging was enabled only when
_not_ WebSocket upgraded, which is wrong.
Removed sending ping immediately after
upgrading to WS as it's superfluous.
Change-Id: Ic8103bab063d87f58d371f0eab49f7b7530e2374
Reviewed-on: https://gerrit.libreoffice.org/36322
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Don't think it is necessary/useful to have this header at other places.
This is the most important and perhaps the only where presence of this
header is required and seems sensible to prevent potential attacks.
Change-Id: Iad318e4b83264ac83620b86a40a49e7384e4015e
None of the subclasses override it, and if they would, it would be
problematic, since e.g. the StreamSocket dtor calls it (and virtual
calls during dtors are a problem).
Change-Id: Ie0891349808a81539078fd1f2d95a55a4ce5107a
Plenty of time to do that next time around the cleanup.
We should still, really be doing the majority of the timeout work
inside the DocumentBroker poll itself.
First reason is that compression is very slow, and we re-compress the
files again and again.
Another reason is that IE/Edge doesn't work well with deflate turned on.
Related: https://connect.microsoft.com/IE/feedbackdetail/view/950689
The documents are not loaded at all with current code snapshot modulo
this patch.
Change-Id: I1fdd85856f448dc4ce02e1ab79e9c7474c3bb7f3
Only during shutdown do we expect a callback
to invalidate the pollSockets, but if it happens
we shouldn't invoke the poll handlers.
Change-Id: I2f56da19aec2f04cc871bd4eae1f93da110e0eb9
Reviewed-on: https://gerrit.libreoffice.org/36189
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
When shutting down accept_poll from
main, we can't remove sockets or cleanup.
That work needs to be done fro within accept_poll's
thread. This is different from when DocBroker's
poll needs to cleanup its own sockets before
it exists.
So we split the stop and removeSockets so they
can each be called in the proper way.
For accept_poll and others that joinThread
we queue a callback to cleanup before stopping.
Change-Id: If780d6a97ac0fc6da6897f895d5b4dda443f9e73
Reviewed-on: https://gerrit.libreoffice.org/36186
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
And assume correct thread if poll thread is
not running (i.e. no race).
Change-Id: I17958e682aba434ebb47fe0de199b9f530b54dee
Reviewed-on: https://gerrit.libreoffice.org/36183
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.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
The DocBroker might not get a chance to
take ownership of a socket (which is done
via callbacks that are invoked in the poll loop)
if it (or WSD) is flagged for termination.
In that case, DocBroker doesn't take ownership
but ultimately needs to disconnect the socket.
By detaching ownership we signal that any thread
can rightly take ownership and thus avoid spurious
warning or assertions.
Change-Id: Idb192bfaac05c5c86809cb21876f3926a080b775
Reviewed-on: https://gerrit.libreoffice.org/36117
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
And insert sockets after starting the
thread so we poll the socket immediately.
Change-Id: Id336e1838f2f624ebfe59c4c2caf33eaa1a638c9
Reviewed-on: https://gerrit.libreoffice.org/36110
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
...and warn if we are in the wrong thread.
This can happen when the socket is not properly
closed from the poll thread and is being destroyed.
Change-Id: I749c09b15d04b49038f7cee6a7a13e8f0145acff
Reviewed-on: https://gerrit.libreoffice.org/36057
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Valgrind found a number of erroneous data access
during the construction and destruction of SslContext.
Change-Id: Ie5072798a3660ed8acc707ba32ac196fa2d0f8af
Reviewed-on: https://gerrit.libreoffice.org/36055
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
This was a workaround to Poco's limitation
of requiring socket receiveFrame be given
preallocated buffer, which couldn't be
exceeded by a larger payload. This meant
the receiver had to know the maximum
payload in advance.
Since only the Kit uses Poco sockets,
and the Kit never receives large payloads,
this preamble is now obsolete.
100% (94/94) of old-style tests PASS.
Change-Id: I76776f89497409e5755e335a3e25553e91cf0876
Reviewed-on: https://gerrit.libreoffice.org/36037
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
Callbacks are used to initialize handlers,
as is the case with addSession on DocumentBroker.
If the socket gets data before the callback is
invoked, the handler will fail since the expected
initialization hasn't happened yet.
This race indeed happens (rarely) with addSession.
100% (94/94) of old-style tests PASS.
Change-Id: Id9b4f63b45c5564add252e1671b7b0b08aff8150
Reviewed-on: https://gerrit.libreoffice.org/36035
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
==20033== Invalid read of size 4
==20033== at 0x466504: ChildProcess::close(bool) (DocumentBroker.hpp:111)
==20033== by 0x44EA28: DocumentBroker::terminateChild(std::string const&, bool) (DocumentBroker.cpp:1313)
==20033== by 0x45F70E: DocumentBroker::pollThread() (DocumentBroker.cpp:264)
==20033== by 0x504B2F: SocketPoll::pollingThreadEntry() (Socket.hpp:486)
==20033== by 0x7310E6F: execute_native_thread_routine (thread.cc:84)
==20033== by 0x7AF60A3: start_thread (pthread_create.c:309)
==20033== by 0x7DF002C: clone (clone.S:111)
==20033== Address 0x0 is not stack'd, malloc'd or (recently) free'd
When not sending ping the ping time is not set
which results in the setting the poll timeout to
a negative value, forcing it to return immediately.
This happens when sending ping before upgrading
to WebSocket, which isn't common. One way to
reproduce it, however, is to connect to the
admin console with an unauthenticated socket.
Change-Id: I9f3db1a02b8f8e2781d23d843e848068ad434958
This prevents a race where the thread is started
a second time before the first gets a chance to
set the flag.
Change-Id: Ib106aa0626cdfa403b321822180b0545d3aa9139
Once a socket has changed ownership to a new
poll it will assert thread affinity with said
new poll. So we cannot do any IO on the old
poll's thread at that point and on.
Change-Id: I662f188dea7c377a18f3e546839ec43f2875dc7b
This reverts commit 388d7b1dbf.
It is vital to have clean control of thread start. By starting
a thread during init. of a member (or base-clase) we loose lots of
control, some examples:
Any members initialized after a member that auto-starts a
thread, is effectively un-defined, and cannot be accessed
reliably.
This is particularly problematic for sub-classes.
I've seen threads started before the base-class has
finished constructing in the original creating thread -
such that the vtable was not yet updated to the sub-class
causing the transient parent vtable used during construction
to be used in-error.
Now that DocumentBroker has SocketPoll thread,
it's isAlive() must be defined by the lifetime of
both the SocketPoll thread and the ChildProcess,
which it previously did.
Change-Id: I093f8774cf4374d01729a383f6c535de4143fec6
Reviewed-on: https://gerrit.libreoffice.org/35122
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
We only move sockets in response to input on the socket, which
must happen in the thread that we're processing that input on.
Ergo - no need for this complexity.
The situation made more problematic since the std::thread is only
created in startThread - so getting ownership right before then in
Socket is problematic.
Only set nodelay and small socket buffers on WebSockets.
Avoid writing more data than can be absorbed by our socket buffer.
It is fine to set socket buffer sizes after bind/accept.
As there isn't support (yet) to send files
asynchronously, when the socket native buffer
is small, asynchronous writes naturally return
EWOULDBLOCK. As a temp solution, we send files
synchronously, so there is no need to poll.
This should be replaced witha file-server
polling/serving thread that is dedicated to
sending files only (which closes the connection
when done).
Change-Id: I062fea44bfe54ab8d147b745da97bd499bf00657
SSL only requests what to poll for next.
So it's more accurate to rename ReadOrWrite
to Neither, since in that case SSL really
isn't blocked on either read or write.
Change-Id: I62dd4f94730d51666a7661b10a9d582d69fbf45e
No longer necessary to have them enabled
permanently, especially that they affect
performance significantly negatively.
Change-Id: I02ef99da00ba4ecb8e24647ee372a03d79d07fe7
