common/Common.hpp | 4
net/Socket.hpp | 39 ++++++
wsd/LOOLWSD.cpp | 312 ++++++++++++++++++++++++------------------------------
3 files changed, 181 insertions(+), 174 deletions(-)
New commits:
commit faf24f7888badcca4d851d289a7b9e31da9d4e5c
Author: Michael Meeks <michael.me...@collabora.com>
Date: Tue Apr 25 22:12:07 2017 +0100
SocketDisposition - controls socket movement & lifecyle.
It is important that moving sockets between polls happens in a
safe and readable fashion, this enforces that.
diff --git a/net/Socket.hpp b/net/Socket.hpp
index 014f023d..694e82a5 100644
--- a/net/Socket.hpp
+++ b/net/Socket.hpp
@@ -198,6 +198,11 @@ public:
}
}
+ const std::thread::id &getThreadOwner()
+ {
+ return _owner;
+ }
+
/// Asserts in the debug builds, otherwise just logs.
void assertCorrectThread()
{
@@ -250,7 +255,6 @@ private:
std::thread::id _owner;
};
-
/// Handles non-blocking socket event polling.
/// Only polls on N-Sockets and invokes callback and
/// doesn't manage buffers or client data.
@@ -913,6 +917,39 @@ protected:
friend class SimpleResponseClient;
};
+/// Helper to allow us to easily defer the movement of a socket
+/// between polls to clarify thread ownership.
+class SocketDisposition
+{
+ std::shared_ptr<StreamSocket> _socket;
+ typedef std::function<void(const std::shared_ptr<StreamSocket> &)>
MoveFunction;
+ MoveFunction _socketMove;
+ SocketHandlerInterface::SocketOwnership _socketOwnership;
+public:
+ SocketDisposition(const std::shared_ptr<StreamSocket> &socket) :
+ _socket(socket),
+ _socketOwnership(SocketHandlerInterface::SocketOwnership::UNCHANGED)
+ {}
+ ~SocketDisposition()
+ {
+ assert (!_socketMove);
+ }
+ void setMove(MoveFunction moveFn)
+ {
+ _socketMove = moveFn;
+ _socketOwnership = SocketHandlerInterface::SocketOwnership::MOVED;
+ }
+ SocketHandlerInterface::SocketOwnership execute()
+ {
+ // We should have hard ownership of this socket.
+ assert(_socket->getThreadOwner() == std::this_thread::get_id());
+ if (_socketMove)
+ _socketMove(_socket);
+ _socketMove = nullptr;
+ return _socketOwnership;
+ }
+};
+
namespace HttpHelper
{
/// Sends file as HTTP response.
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 6071c85e..a783b269 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -1591,7 +1591,7 @@ private:
return SocketHandlerInterface::SocketOwnership::UNCHANGED;
}
- SocketHandlerInterface::SocketOwnership socketOwnership =
SocketHandlerInterface::SocketOwnership::UNCHANGED;
+ SocketDisposition tailDisposition(socket);
try
{
// Routing
@@ -1607,12 +1607,13 @@ private:
// Admin connections
else if (reqPathSegs.size() >= 2 && reqPathSegs[0] == "lool" &&
reqPathSegs[1] == "adminws")
{
- LOG_ERR("Admin request: " << request.getURI());
+ LOG_INF("Admin request: " << request.getURI());
if (AdminSocketHandler::handleInitialRequest(_socket, request))
{
- // Hand the socket over to the Admin poll.
- Admin::instance().insertNewSocket(socket);
- socketOwnership =
SocketHandlerInterface::SocketOwnership::MOVED;
+ tailDisposition.setMove([](const
std::shared_ptr<StreamSocket> &moveSocket){
+ // Hand the socket over to the Admin poll.
+ Admin::instance().insertNewSocket(moveSocket);
+ });
}
}
// Client post and websocket connections
@@ -1637,12 +1638,12 @@ private:
reqPathTokens.count() > 0 && reqPathTokens[0] == "lool")
{
// All post requests have url prefix 'lool'.
- socketOwnership = handlePostRequest(request, message);
+ handlePostRequest(request, message, tailDisposition);
}
else if (reqPathTokens.count() > 2 && reqPathTokens[0] ==
"lool" && reqPathTokens[2] == "ws" &&
request.find("Upgrade") != request.end() &&
Poco::icompare(request["Upgrade"], "websocket") == 0)
{
- socketOwnership = handleClientWsUpgrade(request,
reqPathTokens[1]);
+ handleClientWsUpgrade(request, reqPathTokens[1],
tailDisposition);
}
else
{
@@ -1671,7 +1672,7 @@ private:
// if we succeeded - remove the request from our input buffer
// we expect one request per socket
in.erase(in.begin(), itBody);
- return socketOwnership;
+ return tailDisposition.execute();
}
int getPollEvents(std::chrono::steady_clock::time_point /* now */,
@@ -1806,14 +1807,14 @@ private:
return "application/octet-stream";
}
- SocketHandlerInterface::SocketOwnership handlePostRequest(const
Poco::Net::HTTPRequest& request, Poco::MemoryInputStream& message)
+ void handlePostRequest(const Poco::Net::HTTPRequest& request,
Poco::MemoryInputStream& message,
+ SocketDisposition &tailDisposition)
{
LOG_INF("Post request: [" << request.getURI() << "]");
Poco::Net::HTTPResponse response;
auto socket = _socket.lock();
- SocketHandlerInterface::SocketOwnership socketOwnership =
SocketHandlerInterface::SocketOwnership::UNCHANGED;
StringTokenizer tokens(request.getURI(), "/?");
if (tokens.count() >= 3 && tokens[2] == "convert-to")
{
@@ -1851,21 +1852,22 @@ private:
auto clientSession = createNewClientSession(nullptr, _id,
uriPublic, docBroker, isReadOnly);
if (clientSession)
{
- // Transfer the client socket to the DocumentBroker.
- socketOwnership =
SocketHandlerInterface::SocketOwnership::MOVED;
+ tailDisposition.setMove([docBroker, clientSession,
format]
+ (const
std::shared_ptr<StreamSocket> &moveSocket)
+ { // Perform all of this after removing the socket
// Make sure the thread is running before adding
callback.
docBroker->startThread();
// We no longer own this socket.
- socket->setThreadOwner(std::thread::id(0));
+ moveSocket->setThreadOwner(std::thread::id(0));
- docBroker->addCallback([docBroker, socket,
clientSession, format]()
+ docBroker->addCallback([docBroker, moveSocket,
clientSession, format]()
{
- clientSession->setSaveAsSocket(socket);
+ clientSession->setSaveAsSocket(moveSocket);
// Move the socket into DocBroker.
- docBroker->addSocketToPoll(socket);
+ docBroker->addSocketToPoll(moveSocket);
// First add and load the session.
docBroker->addSession(clientSession);
@@ -1889,6 +1891,7 @@ private:
std::vector<char> saveasRequest(saveas.begin(),
saveas.end());
clientSession->handleMessage(true,
WebSocketHandler::WSOpCode::Text, saveasRequest);
});
+ });
sent = true;
}
@@ -1902,8 +1905,6 @@ private:
// TODO: We should differentiate between bad request and
failed conversion.
throw BadRequestException("Failed to convert and send file.");
}
-
- return socketOwnership;
}
else if (tokens.count() >= 4 && tokens[3] == "insertfile")
{
@@ -1943,7 +1944,7 @@ private:
File(tmpPath).moveTo(fileName);
response.setContentLength(0);
socket->send(response);
- return socketOwnership;
+ return;
}
}
}
@@ -2010,21 +2011,20 @@ private:
{
LOG_ERR("Download file [" << filePath.toString() << "] not
found.");
}
-
(void)responded;
- return socketOwnership;
}
throw BadRequestException("Invalid or unknown request.");
}
- SocketHandlerInterface::SocketOwnership handleClientWsUpgrade(const
Poco::Net::HTTPRequest& request, const std::string& url)
+ void handleClientWsUpgrade(const Poco::Net::HTTPRequest& request, const
std::string& url,
+ SocketDisposition &tailDisposition)
{
auto socket = _socket.lock();
if (!socket)
{
LOG_WRN("No socket to handle client WS upgrade for request: " <<
request.getURI() << ", url: " << url);
- return SocketHandlerInterface::SocketOwnership::UNCHANGED;
+ return;
}
LOG_INF("Client WS request: " << request.getURI() << ", url: " << url
<< ", socket #" << socket->getFD());
@@ -2036,7 +2036,7 @@ private:
{
LOG_ERR("Limit on maximum number of connections of " <<
MAX_CONNECTIONS << " reached.");
shutdownLimitReached(ws);
- return SocketHandlerInterface::SocketOwnership::UNCHANGED;
+ return;
}
LOG_INF("Starting GET request handler for session [" << _id << "] on
url [" << url << "].");
@@ -2064,8 +2064,6 @@ private:
LOG_INF("URL [" << url << "] is " << (isReadOnly ? "readonly" :
"writable") << ".");
- SocketHandlerInterface::SocketOwnership socketOwnership =
SocketHandlerInterface::SocketOwnership::UNCHANGED;
-
// Request a kit process for this doc.
auto docBroker = findOrCreateDocBroker(ws, url, docKey, _id,
uriPublic);
if (docBroker)
@@ -2073,28 +2071,28 @@ private:
auto clientSession = createNewClientSession(&ws, _id, uriPublic,
docBroker, isReadOnly);
if (clientSession)
{
- // Transfer the client socket to the DocumentBroker.
-
- // Remove from current poll as we're moving ownership.
- socketOwnership =
SocketHandlerInterface::SocketOwnership::MOVED;
-
- // Make sure the thread is running before adding callback.
- docBroker->startThread();
+ // Transfer the client socket to the DocumentBroker when we
get back to the poll:
+ tailDisposition.setMove([docBroker, clientSession]
+ (const std::shared_ptr<StreamSocket>
&moveSocket)
+ {
+ // Make sure the thread is running before adding callback.
+ docBroker->startThread();
- // We no longer own this socket.
- socket->setThreadOwner(std::thread::id(0));
+ // We no longer own this socket.
+ moveSocket->setThreadOwner(std::thread::id(0));
- docBroker->addCallback([docBroker, socket, clientSession]()
- {
- // Set the ClientSession to handle Socket events.
- socket->setHandler(clientSession);
- LOG_DBG("Socket #" << socket->getFD() << " handler is " <<
clientSession->getName());
+ docBroker->addCallback([docBroker, moveSocket,
clientSession]()
+ {
+ // Set the ClientSession to handle Socket events.
+ moveSocket->setHandler(clientSession);
+ LOG_DBG("Socket #" << moveSocket->getFD() << " handler
is " << clientSession->getName());
- // Move the socket into DocBroker.
- docBroker->addSocketToPoll(socket);
+ // Move the socket into DocBroker.
+ docBroker->addSocketToPoll(moveSocket);
- // Add and load the session.
- docBroker->addSession(clientSession);
+ // Add and load the session.
+ docBroker->addSession(clientSession);
+ });
});
}
else
@@ -2105,8 +2103,6 @@ private:
}
else
LOG_WRN("Failed to create DocBroker with docKey [" << docKey <<
"].");
-
- return socketOwnership;
}
private:
commit c8d1c18cb54acf82f8b6de2252365eac93e90a58
Author: Michael Meeks <michael.me...@collabora.com>
Date: Tue Apr 25 21:35:20 2017 +0100
Revert "wsd: correctly remove request from socket buffer"
This reverts commit c851c3e93be565dfa14906febbdf6208e9d422cd.
diff --git a/common/Common.hpp b/common/Common.hpp
index b0b018b2..09bafad3 100644
--- a/common/Common.hpp
+++ b/common/Common.hpp
@@ -34,8 +34,8 @@ constexpr auto CHILD_URI = "/loolws/child?";
constexpr auto NEW_CHILD_URI = "/loolws/newchild?";
constexpr auto LO_JAIL_SUBPATH = "lo";
-/// The HTTP response User-Agent.
-constexpr auto HTTP_AGENT_STRING = "LOOLWSD Agent " LOOLWSD_VERSION;
+/// The HTTP response User-Agent. TODO: Include version.
+constexpr auto HTTP_AGENT_STRING = "LOOLWSD Agent";
// The client port number, both loolwsd and the kits have this.
extern int ClientPortNumber;
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 4e136d4e..6071c85e 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -1598,47 +1598,46 @@ private:
Poco::URI requestUri(request.getURI());
std::vector<std::string> reqPathSegs;
requestUri.getPathSegments(reqPathSegs);
- const StringTokenizer reqPathTokens(request.getURI(), "/?",
StringTokenizer::TOK_IGNORE_EMPTY | StringTokenizer::TOK_TRIM);
- if (request.getMethod() == HTTPRequest::HTTP_GET ||
- request.getMethod() == HTTPRequest::HTTP_HEAD)
+ // File server
+ if (reqPathSegs.size() >= 1 && reqPathSegs[0] == "loleaflet")
{
- // Clear the request header already consumed.
- in.erase(in.begin(), itBody);
-
- // File server
- if (!(request.find("Upgrade") != request.end() &&
Poco::icompare(request["Upgrade"], "websocket") == 0) &&
- reqPathTokens.count() >= 5 && reqPathTokens[0] == "lool")
- {
- // All post requests have url prefix 'lool'.
- handleFileDownloadRequest(request);
- }
- else if (reqPathSegs.size() >= 1 && reqPathSegs[0] ==
"loleaflet")
- {
- handleFileServerRequest(request, message);
- }
- else if (reqPathSegs.size() >= 2 && reqPathSegs[0] == "lool"
&& reqPathSegs[1] == "adminws")
- {
- // Admin connections
- LOG_INF("Admin request: " << request.getURI());
- if (AdminSocketHandler::handleInitialRequest(_socket,
request))
- {
- // Hand the socket over to the Admin poll.
- Admin::instance().insertNewSocket(socket);
- socketOwnership =
SocketHandlerInterface::SocketOwnership::MOVED;
- }
- }
- else if (request.getURI() == "/")
- {
- handleRootRequest(request);
- }
- else if (request.getURI() == "/favicon.ico")
+ handleFileServerRequest(request, message);
+ }
+ // Admin connections
+ else if (reqPathSegs.size() >= 2 && reqPathSegs[0] == "lool" &&
reqPathSegs[1] == "adminws")
+ {
+ LOG_ERR("Admin request: " << request.getURI());
+ if (AdminSocketHandler::handleInitialRequest(_socket, request))
{
- handleFaviconRequest(request);
+ // Hand the socket over to the Admin poll.
+ Admin::instance().insertNewSocket(socket);
+ socketOwnership =
SocketHandlerInterface::SocketOwnership::MOVED;
}
- else if (request.getURI() == "/hosting/discovery")
+ }
+ // Client post and websocket connections
+ else if ((request.getMethod() == HTTPRequest::HTTP_GET ||
+ request.getMethod() == HTTPRequest::HTTP_HEAD) &&
+ request.getURI() == "/")
+ {
+ handleRootRequest(request);
+ }
+ else if (request.getMethod() == HTTPRequest::HTTP_GET &&
request.getURI() == "/favicon.ico")
+ {
+ handleFaviconRequest(request);
+ }
+ else if (request.getMethod() == HTTPRequest::HTTP_GET &&
request.getURI() == "/hosting/discovery")
+ {
+ handleWopiDiscoveryRequest(request);
+ }
+ else
+ {
+ StringTokenizer reqPathTokens(request.getURI(), "/?",
StringTokenizer::TOK_IGNORE_EMPTY | StringTokenizer::TOK_TRIM);
+ if (!(request.find("Upgrade") != request.end() &&
Poco::icompare(request["Upgrade"], "websocket") == 0) &&
+ reqPathTokens.count() > 0 && reqPathTokens[0] == "lool")
{
- handleWopiDiscoveryRequest(request);
+ // All post requests have url prefix 'lool'.
+ socketOwnership = handlePostRequest(request, message);
}
else if (reqPathTokens.count() > 2 && reqPathTokens[0] ==
"lool" && reqPathTokens[2] == "ws" &&
request.find("Upgrade") != request.end() &&
Poco::icompare(request["Upgrade"], "websocket") == 0)
@@ -1647,31 +1646,19 @@ private:
}
else
{
- throw std::runtime_error("Bad request.");
- }
-
- return socketOwnership;
- }
- else if (request.getMethod() == HTTPRequest::HTTP_POST)
- {
- if (reqPathSegs.size() >= 1 && reqPathSegs[0] == "loleaflet")
- {
- handleFileServerRequest(request, message);
- return socketOwnership;
- }
-
- if (!(request.find("Upgrade") != request.end() &&
Poco::icompare(request["Upgrade"], "websocket") == 0) &&
- reqPathTokens.count() > 0 && reqPathTokens[0] == "lool")
- {
- // All post requests have url prefix 'lool'.
- socketOwnership = handlePostRequest(request, message);
- in.erase(in.begin(), itBody);
- return socketOwnership;
+ LOG_ERR("Unknown resource: " << request.getURI());
+
+ // Bad request.
+ std::ostringstream oss;
+ oss << "HTTP/1.1 400\r\n"
+ << "Date: " <<
Poco::DateTimeFormatter::format(Poco::Timestamp(),
Poco::DateTimeFormat::HTTP_FORMAT) << "\r\n"
+ << "User-Agent: LOOLWSD WOPI Agent\r\n"
+ << "Content-Length: 0\r\n"
+ << "\r\n";
+ socket->send(oss.str());
+ socket->shutdown();
}
}
-
- LOG_ERR("#" << socket->getFD() << " Unknown request: [" <<
- LOOLProtocol::getAbbreviatedMessage(in) << "].");
}
catch (const std::exception& exc)
{
@@ -1681,17 +1668,10 @@ private:
LOOLProtocol::getAbbreviatedMessage(in) << "]: " <<
exc.what());
}
- // Bad request.
- std::ostringstream oss;
- oss << "HTTP/1.1 400\r\n"
- << "Date: " << Poco::DateTimeFormatter::format(Poco::Timestamp(),
Poco::DateTimeFormat::HTTP_FORMAT) << "\r\n"
- << "User-Agent: " << HTTP_AGENT_STRING << "\r\n"
- << "Content-Length: 0\r\n"
- << "\r\n";
- socket->send(oss.str());
- socket->shutdown();
-
- return SocketHandlerInterface::SocketOwnership::UNCHANGED;
+ // if we succeeded - remove the request from our input buffer
+ // we expect one request per socket
+ in.erase(in.begin(), itBody);
+ return socketOwnership;
}
int getPollEvents(std::chrono::steady_clock::time_point /* now */,
@@ -1967,81 +1947,75 @@ private:
}
}
}
-
- throw BadRequestException("Invalid or unknown request.");
- }
-
- void handleFileDownloadRequest(const Poco::Net::HTTPRequest& request)
- {
- LOG_INF("File download request.");
-
- Poco::Net::HTTPResponse response;
- auto socket = _socket.lock();
-
- StringTokenizer tokens(request.getURI(), "/?");
- // TODO: Check that the user in question has access to this file!
-
- // 1. Validate the dockey
- std::string decodedUri;
- URI::decode(tokens[2], decodedUri);
- const auto docKey =
DocumentBroker::getDocKey(DocumentBroker::sanitizeURI(decodedUri));
- std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
- auto docBrokerIt = DocBrokers.find(docKey);
- if (docBrokerIt == DocBrokers.end())
+ else if (tokens.count() >= 6)
{
- throw BadRequestException("DocKey [" + docKey + "] is invalid.");
- }
+ LOG_INF("File download request.");
+ // TODO: Check that the user in question has access to this file!
- // 2. Cross-check if received child id is correct
- if (docBrokerIt->second->getJailId() != tokens[3])
- {
- throw BadRequestException("ChildId does not correspond to docKey");
- }
+ // 1. Validate the dockey
+ std::string decodedUri;
+ URI::decode(tokens[2], decodedUri);
+ const auto docKey =
DocumentBroker::getDocKey(DocumentBroker::sanitizeURI(decodedUri));
+ std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
+ auto docBrokerIt = DocBrokers.find(docKey);
+ if (docBrokerIt == DocBrokers.end())
+ {
+ throw BadRequestException("DocKey [" + docKey + "] is
invalid.");
+ }
- // 3. Don't let user download the file in main doc directory containing
- // the document being edited otherwise we will end up deleting main
directory
- // after download finishes
- if (docBrokerIt->second->getJailId() == tokens[4])
- {
- throw BadRequestException("RandomDir cannot be equal to ChildId");
- }
- docBrokersLock.unlock();
-
- std::string fileName;
- bool responded = false;
- URI::decode(tokens[5], fileName);
- const Path filePath(LOOLWSD::ChildRoot + tokens[3]
- + JAILED_DOCUMENT_ROOT + tokens[4] + "/" +
fileName);
- LOG_INF("HTTP request for: " << filePath.toString());
- if (filePath.isAbsolute() && File(filePath).exists())
- {
- std::string contentType = getContentType(fileName);
- if (Poco::Path(fileName).getExtension() == "pdf")
+ // 2. Cross-check if received child id is correct
+ if (docBrokerIt->second->getJailId() != tokens[3])
{
- contentType = "application/pdf";
- response.set("Content-Disposition", "attachment; filename=\""
+ fileName + "\"");
+ throw BadRequestException("ChildId does not correspond to
docKey");
}
- try
+ // 3. Don't let user download the file in main doc directory
containing
+ // the document being edited otherwise we will end up deleting
main directory
+ // after download finishes
+ if (docBrokerIt->second->getJailId() == tokens[4])
{
- HttpHelper::sendFile(socket, filePath.toString(), contentType,
response);
- responded = true;
+ throw BadRequestException("RandomDir cannot be equal to
ChildId");
+ }
+ docBrokersLock.unlock();
+
+ std::string fileName;
+ bool responded = false;
+ URI::decode(tokens[5], fileName);
+ const Path filePath(LOOLWSD::ChildRoot + tokens[3]
+ + JAILED_DOCUMENT_ROOT + tokens[4] + "/" +
fileName);
+ LOG_INF("HTTP request for: " << filePath.toString());
+ if (filePath.isAbsolute() && File(filePath).exists())
+ {
+ std::string contentType = getContentType(fileName);
+ if (Poco::Path(fileName).getExtension() == "pdf")
+ {
+ contentType = "application/pdf";
+ response.set("Content-Disposition", "attachment;
filename=\"" + fileName + "\"");
+ }
+
+ try
+ {
+ HttpHelper::sendFile(socket, filePath.toString(),
contentType, response);
+ responded = true;
+ }
+ catch (const Exception& exc)
+ {
+ LOG_ERR("Error sending file to client: " <<
exc.displayText() <<
+ (exc.nested() ? " (" + exc.nested()->displayText()
+ ")" : ""));
+ }
+
+ FileUtil::removeFile(File(filePath.parent()).path(), true);
}
- catch (const Exception& exc)
+ else
{
- LOG_ERR("Error sending file to client: " << exc.displayText()
<<
- (exc.nested() ? " (" + exc.nested()->displayText() +
")" : ""));
+ LOG_ERR("Download file [" << filePath.toString() << "] not
found.");
}
- FileUtil::removeFile(File(filePath.parent()).path(), true);
- }
- else
- {
- LOG_ERR("Download file [" << filePath.toString() << "] not
found.");
+ (void)responded;
+ return socketOwnership;
}
- // TODO: Use this to send failure response.
- (void)responded;
+ throw BadRequestException("Invalid or unknown request.");
}
SocketHandlerInterface::SocketOwnership handleClientWsUpgrade(const
Poco::Net::HTTPRequest& request, const std::string& url)
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
