Skip to content

Commit

Permalink
event server: fix possible problems when client sends multiple reques…
Browse files Browse the repository at this point in the history
…ts (OpenPHDGuiding#1228)

The `connect` command can cause the event loop to run and for the event server to process
additional buffered client commands.  As the JSON parser instance is reused
for all client commands, the parser's state will reflect the most recent client
command and may lose the state of the unfinished connect command.

The fix is to not reuse the JsonParser instance -- instantiate a dedicated
parser for each client command.

Testing:
 - manually exercised the event server by issuing commands with an event server client
 - checked the runtime overhead of instantiating the JsonParser instance per request (negligible, < 100 ns)
  • Loading branch information
agalasso authored Jul 29, 2024
1 parent ef6bb9c commit 2d2046a
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 6 deletions.
18 changes: 13 additions & 5 deletions src/event_server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2442,8 +2442,13 @@ static bool handle_request(JRpcCall& call)
}
}

static void handle_cli_input_complete(wxSocketClient *cli, char *input, JsonParser& parser)
static void handle_cli_input_complete(wxSocketClient *cli, char *input)
{
// a dedicated JsonParser instance is used for each line of input since
// handle_request can recurse if the request causes the event loop to run and we
// don't want the parser to be reused.
JsonParser parser;

if (!parser.Parse(input))
{
JRpcCall call(cli, nullptr);
Expand Down Expand Up @@ -2490,7 +2495,7 @@ static void handle_cli_input_complete(wxSocketClient *cli, char *input, JsonPars
}
}

static void handle_cli_input(wxSocketClient *cli, JsonParser& parser)
static void handle_cli_input(wxSocketClient *cli)
{
// Bump refcnt to protect against reentrancy.
//
Expand Down Expand Up @@ -2526,7 +2531,10 @@ static void handle_cli_input(wxSocketClient *cli, JsonParser& parser)
char *end;
while ((end = static_cast<char *>(memchr(rdbuf->buf(), '\n', rdbuf->len()))) != nullptr)
{
// Copy to temporary buffer to avoid rentrancy problem
// Move the newline-terminated chunk from the read buffer to a temporary
// buffer on the stack, and consume the chunk from the read buffer before
// processing the line. This leaves the read buffer in the correct state to
// be used again if this function is caller reentrantly.
char line[ClientReadBuf::SIZE];
size_t len1 = end - rdbuf->buf();
memcpy(line, rdbuf->buf(), len1);
Expand All @@ -2537,7 +2545,7 @@ static void handle_cli_input(wxSocketClient *cli, JsonParser& parser)
memmove(rdbuf->buf(), next, len2);
rdbuf->dest = rdbuf->buf() + len2;

handle_cli_input_complete(cli, line, parser);
handle_cli_input_complete(cli, line);
}
}
}
Expand Down Expand Up @@ -2639,7 +2647,7 @@ void EventServer::OnEventServerClientEvent(wxSocketEvent& event)
}
else if (event.GetSocketEvent() == wxSOCKET_INPUT)
{
handle_cli_input(cli, m_parser);
handle_cli_input(cli);
}
else
{
Expand Down
1 change: 0 additions & 1 deletion src/event_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ class EventServer : public wxEvtHandler
typedef std::set<wxSocketClient *> CliSockSet;

private:
JsonParser m_parser;
wxSocketServer *m_serverSocket;
CliSockSet m_eventServerClients;
wxTimer *m_configEventDebouncer;
Expand Down

0 comments on commit 2d2046a

Please sign in to comment.