strepsirrhini/libreoffice-online
3
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Get rid of some fragile string literal length counts

Browse source 
Basically, instead of foo.compare(0, 3, "bar") use
LOOLProtocol::getFirstToken(foo)!="bar". Using a separate length of a
string literal is very fragile, it is easy to get the length wrong, or
forget to change it in case the string changes. See
137e677eb0.

It isn't entirely sure to me whether was is kept in a message queue
follows LOOL protocol message syntax, but it seems to be so in
practice, so we can use LOOLProtocol::getFirstToken().

Also, calls like msg.compare(0, 4, "tile") add undocumented implicit
requirements that the token in question must be unique as an initial
substring of the first tokens of all messages. It actually isn't,
there is also "tilecombine". But in this case, that was taken care of
by checking for "tilecombine" first. Ugh. Whether that was by accident
or design is hard to say, but at least there was no comment indicating
intentionality.

We still have lots of similar fragile mis-use of std::string::find().
This commit is contained in:
Tor Lillqvist 2016-09-26 20:13:50 +03:00
parent 01c6cb40ef
commit 7922948142
1 changed files with 15 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

29
loolwsd/MessageQueue.cpp
View file

@ -13,8 +13,9 @@
#include <Poco/StringTokenizer.h>
#include <TileDesc.hpp>
#include <LOOLProtocol.hpp>
#include <Log.hpp>
#include <TileDesc.hpp>
using Poco::StringTokenizer;
@ -76,8 +77,9 @@ void MessageQueue::clear_impl()
void TileQueue::put_impl(const Payload& value)
{
const auto msg = std::string(value.data(), value.size());
const std::string firstToken = LOOLProtocol::getFirstToken(value);
if (msg.compare(0, 11, "canceltiles") == 0)
if (firstToken == "canceltiles")
{
Log::trace("Processing " + msg);
Log::trace() << "Before canceltiles have " << _queue.size() << " in queue." << Log::end;
@ -104,7 +106,7 @@ void TileQueue::put_impl(const Payload& value)
Log::trace() << "After canceltiles have " << _queue.size() << " in queue." << Log::end;
return;
}
else if (msg.compare(0, 11, "tilecombine") == 0)
else if (firstToken == "tilecombine")
{
// Breakup tilecombine and deduplicate (we are re-combining the tiles
// in the get_impl() again)
@ -119,7 +121,7 @@ void TileQueue::put_impl(const Payload& value)
}
return;
}
else if (msg.compare(0, 4, "tile") == 0)
else if (firstToken == "tile")
{
removeDuplicate(msg);
@ -135,8 +137,12 @@ void TileQueue::put_impl(const Payload& value)
void TileQueue::removeDuplicate(const std::string& tileMsg)
{
assert(tileMsg.compare(0, 4, "tile") == 0);
assert(LOOLProtocol::getFirstToken(tileMsg) == "tile");
// FIXME: This looks rather fragile; but OTOH if I understand correctly this doesn't handle
// input from clients, but strings we have created ourselves here in C++ code, so probably we
// can be sure that the "ver" parameter is always in such a location that this does what we
// mean.
const std::string newMsg = tileMsg.substr(0, tileMsg.find(" ver"));
for (size_t i = 0; i < _queue.size(); ++i)
@ -155,11 +161,6 @@ void TileQueue::removeDuplicate(const std::string& tileMsg)
bool TileQueue::priority(const std::string& tileMsg)
{
if (tileMsg.compare(0, 5, "tile ") != 0)
{
return false;
}
auto tile = TileDesc::parse(tileMsg); //FIXME: Expensive, avoid.
for (int view : _viewOrder)
@ -179,7 +180,8 @@ MessageQueue::Payload TileQueue::get_impl()
auto msg = std::string(front.data(), front.size());
if (msg.compare(0, 5, "tile ") != 0 || msg.find("id=") != std::string::npos)
std::string id;
if (LOOLProtocol::getFirstToken(msg) != "tile" || LOOLProtocol::getTokenStringFromMessage(msg, "id", id))
{
// Don't combine non-tiles or tiles with id.
Log::trace() << "MessageQueue res: " << msg << Log::end;
@ -198,7 +200,7 @@ MessageQueue::Payload TileQueue::get_impl()
// avoid starving - stop the search when we reach a non-tile,
// otherwise we may keep growing the queue of unhandled stuff (both
// tiles and non-tiles)
if (prio.compare(0, 5, "tile ") != 0)
if (LOOLProtocol::getFirstToken(prio) != "tile")
break;
if (priority(prio))
@ -221,8 +223,7 @@ MessageQueue::Payload TileQueue::get_impl()
{
auto& it = _queue[i];
msg = std::string(it.data(), it.size());
if (msg.compare(0, 5, "tile ") != 0 ||
msg.find("id=") != std::string::npos)
if (LOOLProtocol::getFirstToken(msg) != "tile" || LOOLProtocol::getTokenStringFromMessage(msg, "id", id))
{
// Don't combine non-tiles or tiles with id.
++i;