Fix client_init() assert

configure.ac

@@ -10,7 +10,7 @@ AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
 
 AC_CONFIG_HEADERS([imaptest-config.h])
-AM_INIT_AUTOMAKE
+AM_INIT_AUTOMAKE([serial-tests])
 AM_SILENT_RULES([yes])
 
 AM_MAINTAINER_MODE

src/Makefile.am

@@ -1,16 +1,17 @@
 bin_PROGRAMS = imaptest
 noinst_PROGRAMS = test-test-parser
+check_PROGRAMS = test-client-stalled
+TESTS = $(check_PROGRAMS)
 
 AM_CPPFLAGS = $(LIBDOVECOT_INCLUDE) -DSTATIC_OPENSSL=1
 imaptest_LDFLAGS = $(AM_LDFLAGS)
 
-imaptest_SOURCES = \
+common_sources = \
 	checkpoint.c \
 	client.c \
 	client-state.c \
 	commands.c \
 	imap-client.c \
-	imaptest.c \
 	imaptest-lmtp.c \
 	mailbox.c \
 	mailbox-source.c \
@@ -26,10 +27,20 @@ imaptest_SOURCES = \
 	test-parser.c \
 	user.c
 
+imaptest_SOURCES = \
+	$(common_sources) \
+	imaptest.c
+
 test_test_parser_SOURCES = test-test-parser.c test-parser.c settings.c
 test_test_parser_LDADD = $(imaptest_LDADD)
 test_test_parser_CFLAGS = $(imaptest_CFLAGS)
 
+test_client_stalled_SOURCES = \
+	$(common_sources) \
+	test-client-stalled.c
+test_client_stalled_LDADD = $(imaptest_LDADD)
+test_client_stalled_CFLAGS = $(imaptest_CFLAGS)
+
 noinst_HEADERS = \
 	checkpoint.h \
 	client.h \

src/client.c

@@ -30,7 +30,7 @@
 int clients_count = 0;
 unsigned int total_disconnects = 0;
 ARRAY_TYPE(client) clients;
-ARRAY(unsigned int) stalled_clients;
+ARRAY_TYPE(unsigned_int) stalled_clients;
 bool stalled = FALSE, disconnect_clients = FALSE, scripted_tests_running = FALSE;
 
 static unsigned int client_min_free_idx = 0;
@@ -198,21 +198,23 @@ client_new_full(unsigned int i, struct user *user, struct user_client *uc)
 {
 	struct imap_client *imap_c;
 	struct pop3_client *pop3_c;
-
-	if (client_min_free_idx == i)
-		client_min_free_idx++;
+	struct client *client;
 
 	if (uc == NULL || uc->profile == NULL ||
 	    strcmp(uc->profile->protocol, "imap") == 0) {
 		if (imap_client_new(i, user, uc, &imap_c) < 0)
 			return NULL;
-		return &imap_c->client;
+		client = &imap_c->client;
 	} else if (strcmp(uc->profile->protocol, "pop3") == 0) {
 		if (pop3_client_new(i, user, uc, &pop3_c) < 0)
 			return NULL;
-		return &pop3_c->client;
+		client = &pop3_c->client;
 	} else
 		i_unreached();
+
+	if (client_min_free_idx == client->idx)
+		client_min_free_idx++;
+	return client;
 }
 
 struct client *client_new_user(struct user *user)
@@ -249,8 +251,25 @@ int client_init(struct client *client, unsigned int idx,
 	const struct ip_addr *ip;
 	int fd;
 
+	if (idx < array_count(&clients) &&
+	    array_idx_elem(&clients, idx) != NULL) {
+		/* The requested index is already occupied. This can happen
+		   when clients_unstalled() reuses a stalled client's index
+		   that was subsequently reassigned to another client.
+		   Find a free slot. */
+		struct client *const *c;
+		unsigned int i, count;
+
+		c = array_get(&clients, &count);
+		for (i = 0; i < count; i++) {
+			if (c[i] == NULL)
+				break;
+		}
+		idx = i;
+	}
+
 	i_assert(idx >= array_count(&clients) ||
-		 *(struct client **)array_idx(&clients, idx) == NULL);
+		 array_idx_elem(&clients, idx) == NULL);
 	if (stalled) {
 		array_append(&stalled_clients, &idx, 1);
 		return -1;
@@ -429,7 +448,8 @@ unsigned int clients_get_random_idx(void)
 
 void clients_init(void)
 {
-	i_array_init(&stalled_clients, CLIENTS_COUNT);
+	i_array_init(&clients, conf.clients_count);
+	i_array_init(&stalled_clients, conf.clients_count);
 }
 
 void clients_deinit(void)

src/client.h

@@ -51,10 +51,12 @@ struct client {
 	bool idling:1;
 };
 ARRAY_DEFINE_TYPE(client, struct client *);
+ARRAY_DEFINE_TYPE(unsigned_int, unsigned int);
 
 extern int clients_count;
 extern unsigned int total_disconnects;
 extern ARRAY_TYPE(client) clients;
+extern ARRAY_TYPE(unsigned_int) stalled_clients;
 extern bool stalled, disconnect_clients, scripted_tests_running;
 
 struct client *client_new_user(struct user *user);

src/imaptest.c

@@ -862,7 +862,6 @@ int main(int argc ATTR_UNUSED, char *argv[])
 	ssl_iostream_openssl_init();
 #endif
 
-	i_array_init(&clients, CLIENTS_COUNT);
 	if (testpath == NULL)
 		imaptest_run();
 	else

src/test-client-stalled.c

@@ -0,0 +1,176 @@
+/* Copyright (c) ImapTest authors, see the included COPYING file */
+
+#include "lib.h"
+#include "ioloop.h"
+#include "array.h"
+#include "settings.h"
+#include "client.h"
+#include "imap-client.h"
+#include "user.h"
+#include "mailbox.h"
+#include "mailbox-source.h"
+#include "test-common.h"
+
+struct settings conf;
+bool profile_running = FALSE;
+
+/* Mock net_connect_ip to return a real (but unconnected) socket.
+   This allows using real i_stream_create_fd() etc. without real network.
+   Since we link against libdovecot, we can provide our own version of this
+   function to override the one in the library for this test. */
+int net_connect_ip(const struct ip_addr *ip, in_port_t port, const struct ip_addr *my_ip)
+{
+	return socket(AF_INET, SOCK_STREAM, 0);
+}
+
+/* These are needed by imap-client.c and other files but are defined in imaptest.c */
+bool imaptest_has_clients(void) { return FALSE; }
+void sig_die(int signo ATTR_UNUSED, void *context ATTR_UNUSED) { exit(1); }
+void error_quit(void) { exit(1); }
+
+static void test_stalled_client_reassignment(void)
+{
+	struct user *user;
+	struct imap_client *c1, *c2, *c3;
+	struct ioloop *ioloop;
+	struct mailbox_source *source;
+
+	test_begin("stalled client index reassignment");
+
+	ioloop = io_loop_create();
+
+	conf.clients_count = 10;
+	conf.ips_count = 1;
+	conf.ips = i_new(struct ip_addr, 1);
+	conf.mailbox = "INBOX";
+
+	clients_init();
+	mailboxes_init();
+
+	source = mailbox_source_new_random(1024);
+	users_init(NULL, source);
+	user = user_get("testuser", source);
+
+	/* 1. Add a client at index 0 */
+	if (imap_client_new(0, user, NULL, &c1) < 0)
+		i_fatal("imap_client_new 1 failed");
+
+	test_assert(array_count(&clients) == 1);
+	test_assert(array_idx_elem(&clients, 0) == &c1->client);
+
+	/* 2. Set stalled = TRUE, then try to init another client at index 0.
+	   This simulates clients_unstalled() trying to reuse index 0. */
+	stalled = TRUE;
+	/* In the fixed code, client_init should have found that index 0 is occupied,
+	   found index 1 is free, and appended index 1 to stalled_clients. */
+	if (imap_client_new(0, user, NULL, &c2) != -1)
+		i_fatal("imap_client_new 2 should have stalled and returned -1");
+
+	const unsigned int *stalled_idxs;
+	unsigned int stalled_count;
+	stalled_idxs = array_get(&stalled_clients, &stalled_count);
+	test_assert(stalled_count == 1);
+	test_assert(stalled_idxs[0] == 1);
+
+	/* 3. Now let's occupy index 1 as well */
+	stalled = FALSE;
+	if (imap_client_new(1, user, NULL, &c3) < 0)
+		i_fatal("imap_client_new 3 failed");
+
+	test_assert(array_count(&clients) == 2);
+	test_assert(array_idx_elem(&clients, 1) == &c3->client);
+
+	/* 4. Now try to stall index 0 again. It should find index 2. */
+	stalled = TRUE;
+	if (imap_client_new(0, user, NULL, &c2) != -1)
+		i_fatal("imap_client_new 4 should have stalled and returned -1");
+
+	stalled_idxs = array_get(&stalled_clients, &stalled_count);
+	test_assert(stalled_count == 2);
+	test_assert(stalled_idxs[1] == 2);
+
+	client_unref(&c1->client, FALSE);
+	client_unref(&c3->client, FALSE);
+
+	users_deinit();
+	mailbox_source_unref(&source);
+	mailboxes_deinit();
+	clients_deinit();
+	array_free(&clients);
+	i_free(conf.ips);
+
+	io_loop_destroy(&ioloop);
+
+	test_end();
+}
+
+static void test_nonstalled_client_reassignment(void)
+{
+	struct user *user;
+	struct imap_client *c1, *c2;
+	struct ioloop *ioloop;
+	struct mailbox_source *source;
+
+	test_begin("non-stalled client index reassignment");
+
+	ioloop = io_loop_create();
+
+	conf.clients_count = 10;
+	conf.ips_count = 1;
+	conf.ips = i_new(struct ip_addr, 1);
+	conf.mailbox = "INBOX";
+
+	clients_init();
+	mailboxes_init();
+
+	source = mailbox_source_new_random(1024);
+	users_init(NULL, source);
+	user = user_get("testuser", source);
+
+	/* 1. Add a client at index 0 */
+	if (imap_client_new(0, user, NULL, &c1) < 0)
+		i_fatal("imap_client_new 1 failed");
+
+	test_assert(array_count(&clients) == 1);
+	test_assert(c1->client.idx == 0);
+
+	/* 2. With stalled = FALSE, request index 0 which is occupied.
+	   client_init should find index 1 free and create the client there. */
+	stalled = FALSE;
+	if (imap_client_new(0, user, NULL, &c2) < 0)
+		i_fatal("imap_client_new 2 failed");
+
+	test_assert(array_count(&clients) == 2);
+	test_assert(c2->client.idx == 1);
+	test_assert(array_idx_elem(&clients, 1) == &c2->client);
+
+	/* 3. Verify stalled_clients array is empty - nothing was stalled */
+	const unsigned int *stalled_idxs;
+	unsigned int stalled_count;
+	stalled_idxs = array_get(&stalled_clients, &stalled_count);
+	test_assert(stalled_count == 0);
+
+	client_unref(&c1->client, FALSE);
+	client_unref(&c2->client, FALSE);
+
+	users_deinit();
+	mailbox_source_unref(&source);
+	mailboxes_deinit();
+	clients_deinit();
+	array_free(&clients);
+	i_free(conf.ips);
+
+	io_loop_destroy(&ioloop);
+
+	test_end();
+}
+
+int main(void)
+{
+	static void (*const test_functions[])(void) = {
+		test_stalled_client_reassignment,
+		test_nonstalled_client_reassignment,
+		NULL
+	};
+	return test_run(test_functions);
+}