exmdb_client: discard connections when EOF is detected

After an unsuccessful RPC (and the response delivered to the client),
the exmdb server may close the connection. The closing however is
only noticed by the client at the next call, where it is erroneously
interpreted as that second RPC failing.

Whenever the client draws a connection from the connpool, evaluate
whether the file descriptor is still usable, else ditch it.

.gitignore

@@ -72,6 +72,7 @@ Makefile.in
 /tests/compress
 /tests/cryptest
 /tests/epv_unpack
+/tests/exrpctest
 /tests/gxl-383
 /tests/jsontest
 /tests/lzxpress

Makefile.am

@@ -258,7 +258,7 @@ mapi_la_LDFLAGS = ${plugin_LDFLAGS} ${PHP_LDFLAGS}
 mapi_la_LIBADD = libphp_mapi.la
 EXTRA_mapi_la_DEPENDENCIES = ${default_sym}
 
-noinst_PROGRAMS = dldcheck tests/bdump tests/bodyconv tests/compress tests/cryptest tests/gxl-383 tests/jsontest tests/lzxpress tests/utiltest tests/vcard tests/zendfake tools/tzdump
+noinst_PROGRAMS = dldcheck tests/bdump tests/bodyconv tests/compress tests/cryptest tests/exrpctest tests/gxl-383 tests/jsontest tests/lzxpress tests/utiltest tests/vcard tests/zendfake tools/tzdump
 if HAVE_ESEDB
 noinst_PROGRAMS += tests/epv_unpack
 endif
@@ -275,6 +275,8 @@ tests_cryptest_SOURCES = tests/cryptest.cpp
 tests_cryptest_LDADD = libgromox_common.la
 tests_epv_unpack_SOURCES = tests/epv_unpack.cpp tools/edb_pack.cpp tools/edb_pack.hpp
 tests_epv_unpack_LDADD = ${libesedb_LIBS} ${libHX_LIBS} libgromox_common.la libgromox_mapi.la
+tests_exrpctest_SOURCES = tests/exrpctest.cpp
+tests_exrpctest_LDADD = libgromox_common.la libgromox_exrpc.la
 tests_gxl_383_SOURCES = tests/gxl-383.cpp
 tests_gxl_383_LDADD = libgromox_common.la libgromox_exrpc.la libgromox_mapi.la
 tests_jsontest_SOURCES = tests/jsontest.cpp

doc/changelog.rst

@@ -1,6 +1,10 @@
-Development 2.21.10
+Development 2.21.13
 ===================
 
+Fixes:
+
+* exmdb_client: discard connections when EOF is detected
+
 Enhancements:
 
 * nsp,ab: support name resolution of IDN addresses

lib/exmdb_client.cpp

@@ -37,7 +37,7 @@ static constexpr unsigned int mdcl_ping_timeout = 2;
 static_assert(SOCKET_TIMEOUT >= mdcl_ping_timeout);
 static std::list<agent_thread> mdcl_agent_list;
 static std::list<remote_svr> mdcl_server_list;
-static std::mutex mdcl_server_lock;
+static std::mutex mdcl_server_lock; /* he protecc mdcl_server_list+mdcl_agent_list */
 static atomic_bool mdcl_notify_stop;
 static unsigned int mdcl_conn_max, mdcl_threads_max;
 static pthread_t mdcl_scan_id;
@@ -332,6 +332,7 @@ static int launch_notify_listener(remote_svr &srv) try
  if (mdcl_event_proc == nullptr)
  return 0;
  mdcl_agent_list.emplace_back();
+ /* Notification thread creates its own socket. */
  auto &ag = mdcl_agent_list.back();
  ag.pserver = &srv;
  ag.sockd = -1;
@@ -442,6 +443,16 @@ bool exmdb_client_check_local(const char *prefix, BOOL *pvt)
  return true;
 }
 
+static bool sock_ready_for_write(int fd)
+{
+ struct pollfd pfd = {fd, POLLIN};
+ /*
+ * If there was already data to read (poll returns 1) or EOF was hit
+ * (poll returns 1), the socket is not ready for write.
+ */
+ return poll(&pfd, 1, 0) == 0;
+}
+
 static remote_conn_ref exmdb_client_get_connection(const char *dir)
 {
  remote_conn_ref fc;
@@ -452,9 +463,12 @@ static remote_conn_ref exmdb_client_get_connection(const char *dir)
  mlog(LV_ERR, "exmdb_client: cannot find remote server for %s", dir);
  return fc;
  }
- if (i->conn_list.size() > 0) {
- fc.tmplist.splice(fc.tmplist.end(), i->conn_list, i->conn_list.begin());
- return fc;
+ while (i->conn_list.size() > 0) {
+ if (sock_ready_for_write(i->conn_list.front().sockd)) {
+ fc.tmplist.splice(fc.tmplist.end(), i->conn_list, i->conn_list.begin());
+ return fc;
+ }
+ i->conn_list.pop_front();
  }
  if (i->active_handles >= mdcl_conn_max) {
  mlog(LV_ERR, "exmdb_client: reached maximum connections (%u) to [%s]:%hu/%s",
@@ -496,20 +510,24 @@ BOOL exmdb_client_do_rpc(const exreq *rq, exresp *rsp)
  if (!exmdb_client_read_socket(conn->sockd, bin, mdcl_rpc_timeout))
  return false;
  conn->last_time = time(nullptr);
- conn.reset();
  if (bin.pb == nullptr)
  return false;
  if (bin.cb == 1) {
- fprintf(stderr, "%s(%s): %s\n", __func__, znul(rq->dir),
- exmdb_rpc_strerror(static_cast<exmdb_response>(bin.pb[0])));
  exmdb_rpc_free(bin.pb);
+ /* Connection is still good in principle. */
+ conn.reset();
  return false;
  }
  if (bin.cb < 5) {
- /* Malformed packet? */
  exmdb_rpc_free(bin.pb);
+ /*
+ * Malformed packet? Let connection die
+ * (~exmdb_connection_ref), lest the next response might pick
+ * up garbage from the current response.
+ */
  return false;
  }
+ conn.reset();
  rsp->call_id = rq->call_id;
  bin.cb -= 5;
  bin.pb += 5;

tests/exrpctest.cpp

@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+// SPDX-FileCopyrightText: 2024 grommunio GmbH
+// This file is part of Gromox.
+#include <cstdint>
+#include <cstdlib>
+#include <gromox/exmdb_client.hpp>
+#include <gromox/exmdb_rpc.hpp>
+#include <gromox/paths.h>
+#include <gromox/util.hpp>
+
+using namespace gromox;
+namespace exmdb_client = exmdb_client_remote;
+
+int main(int argc, char **argv)
+{
+ exmdb_client_init(1, 0);
+ if (exmdb_client_run(PKGSYSCONFDIR) != 0)
+ return EXIT_FAILURE;
+
+ static constexpr STORE_ENTRYID other_store = {0, 0, 0, 0, {}, 0, deconst(""), deconst("")};
+ const char *g_storedir = argc == 1 ? "/" : argv[1];
+ char *newdir = nullptr;
+ unsigned int user_id = 0, domain_id = 0;
+
+ printf("req 1\n");
+ if (!exmdb_client::store_eid_to_user(g_storedir, &other_store, &newdir,
+ &user_id, &domain_id))
+ mlog(LV_DEBUG, "store_eid_to_user failed as expected");
+ else
+ mlog(LV_ERR, "store_eid_to_user unexpectedly succeeded");
+ // Connection should have died by now
+ printf("req 2\n");
+
+ static constexpr uint32_t tags[] = {PR_STORE_RECORD_KEY};
+ static constexpr PROPTAG_ARRAY ptags[] = {std::size(tags), deconst(tags)};
+ TPROPVAL_ARRAY props{};
+ exmdb_client::get_store_properties(g_storedir, CP_UTF8, ptags, &props);
+ return 0;
+}