config partition sanity checker

cassandane/Cassandane/Cyrus/Info.pm

@@ -271,6 +271,100 @@ sub test_lint_partitions
     );
 }
 
+sub test_lint_partitions_dups
+    :NoStartInstances
+{
+    my ($self) = @_;
+
+    $self->config_set(
+        # each has its own disk path: good
+        'partition-good' => '/tmp/pgood',
+        'metapartition-good' => '/tmp/mgood',
+        'archivepartition-good' => '/tmp/agood',
+        'foosearchpartition-good' => '/tmp/sgood',
+
+        # reusing disk paths for same-named partition: bad
+        'partition-bad1' => '/tmp/bad',
+        'metapartition-bad1' => '/tmp/bad',
+
+        # reusing disk paths for other-named partition: bad
+        'partition-bad2' => '/tmp/bad',
+    );
+
+    # master should fail to start
+    eval {
+        $self->_start_instances();
+    };
+    my $e = $@;
+    $self->assert_not_null($e);
+
+    xlog $self, "test 'cyr_info conf-lint' with duplicated partitions configured";
+
+    my @output = $self->{instance}->run_cyr_info('conf-lint');
+    @output = grep { !m/_db: / } @output;  # skip database types
+
+    $self->assert_deep_equals(
+        [ sort(
+            "partition-bad1: /tmp/bad\n",
+            "metapartition-bad1: /tmp/bad\n",
+            "partition-bad2: /tmp/bad\n",
+        ) ],
+        [ sort @output ]
+    );
+
+    $self->assert_syslog_matches($self->{instance},
+                                 qr{disk path used by multiple partitions});
+}
+
+sub test_lint_partitions_subdirs
+    :NoStartInstances
+{
+    my ($self) = @_;
+
+    $self->config_set(
+        # each has its own disk path: good
+        'partition-good' => '/tmp/pgood',
+        'metapartition-good' => '/tmp/mgood',
+        'archivepartition-good' => '/tmp/agood',
+        'foosearchpartition-good' => '/tmp/sgood',
+
+        # disk paths that are children of other's disk paths: bad
+        'partition-bad1' => '/tmp/bad1',
+        'metapartition-bad1' => '/tmp/bad1/meta',
+        # XXX it can't currently report more than one bad child per parent
+
+        # disk paths that are descendents of other's disk paths: bad
+        'partition-bad2' => '/tmp/bad2',
+        'metapartition-bad2' => '/tmp/bad2/blah/meta',
+        # XXX it can't currently report more than one bad child per parent
+    );
+
+    # master should fail to start
+    eval {
+        $self->_start_instances();
+    };
+    my $e = $@;
+    $self->assert_not_null($e);
+
+    xlog $self, "test 'cyr_info conf-lint' with subdir partitions configured";
+
+    my @output = $self->{instance}->run_cyr_info('conf-lint');
+    @output = grep { !m/_db: / } @output;  # skip database types
+
+    $self->assert_deep_equals(
+        [ sort(
+            "partition-bad1: /tmp/bad1\n",
+            "metapartition-bad1: /tmp/bad1/meta\n",
+            "partition-bad2: /tmp/bad2\n",
+            "metapartition-bad2: /tmp/bad2/blah/meta\n",
+        ) ],
+        [ sort @output ]
+    );
+
+    $self->assert_syslog_matches($self->{instance},
+                                 qr{disk path is a prefix of others});
+}
+
 sub test_lint_services
     :want_service_http :needs_component_httpd :NoStartInstances
 {

changes/next/cyr-2103-sanity-check-dup-partitions

@@ -0,0 +1,28 @@
+Description:
+
+:cyrusman:`master(8)` now refuses to start when multiple partitions are
+configured with the same directory, or a subdirectory thereof.  This
+prevents potential store corruption when multiple things each think they
+own everything.  :cyrusman:`cyr_info(8)` ``conf-lint`` will report the
+invalid partitions.
+
+
+Documentation:
+
+:cyrusman:`cyr_info(8)`
+
+
+Config changes:
+
+``*partition-*`` entries in :cyrusman:`imapd.conf(5)` are now checked against
+one another.
+
+
+Upgrade instructions:
+
+Nothing required, unless your configuration was already bad.
+
+
+GitHub issue:
+
+#5573

cunit/libconfig.testc

@@ -1125,4 +1125,67 @@ static void test_deprecated_bytesize(void)
     CU_ASSERT_EQUAL(val, 12LL * 1024 * 1024);
 }
 
+static void test_partition_sanity_ok(void)
+{
+    int r;
+
+    config_read_string(
+        "configdirectory: "DBDIR"/conf\n"
+        "partition-foo: /srv/foo\n"
+        "archivepartition-foo: /srv/archive/foo\n"
+        "metapartition-foo: /srv/meta/foo\n"
+        "t1searchpartition-foo: /srv/search/t1/foo\n"
+        "t2searchpartition-foo: /srv/search/t2/foo\n"
+        "partition-bar: /srv/bar\n"
+        "archivepartition-bar: /srv/archive/bar\n"
+        "metapartition-bar: /srv/meta/bar\n"
+        "t1searchpartition-bar: /srv/search/t1/bar\n"
+        "t2searchpartition-bar: /srv/search/t2/bar\n"
+    );
+
+    r = config_check_partitions(NULL);
+    CU_ASSERT_EQUAL(r, 0);
+}
+
+static void test_partition_sanity_bad_subdirs(void)
+{
+    int r;
+
+    config_read_string(
+        "configdirectory: "DBDIR"/conf\n"
+        "partition-foo: /srv/foo\n"
+        "archivepartition-foo: /srv/foo/archive\n" /* bad */
+        "metapartition-foo: /srv/foo/meta\n" /* bad */
+        "partition-foobar: /srv/foo-bar\n" /* this better not get in the way */
+        "archivepartition-foobar: /srv/archive/foo-bar\n" /* okay */
+    );
+
+    CU_SYSLOG_MATCH("disk path is a prefix of others");
+    r = config_check_partitions(NULL);
+    /* there are two subdirs of /srv/foo configured, but since we're doing
+     * substring matches rather than a real tree lookup, we only notice the
+     * first.
+     */
+    CU_ASSERT_EQUAL(r, -1); // not -2
+    CU_ASSERT_SYSLOG(/*all*/0, 1); // not 2
+}
+
+static void test_partition_sanity_bad_dups(void)
+{
+    int r;
+
+    config_read_string(
+        "configdirectory: "DBDIR"/conf\n"
+        "partition-foo: /srv/foo\n"
+        "t1searchpartition-foo: /srv/foo\n" /* uh oh */
+        "partition-bar: /srv/bar\n"
+        "archivepartition-bar: /srv/bar\n" /* uh oh */
+    );
+
+    CU_SYSLOG_MATCH("disk path used by multiple partitions");
+    r = config_check_partitions(NULL);
+    CU_ASSERT_EQUAL(r, -2);
+    CU_ASSERT_SYSLOG(/*all*/0, 2);
+}
+
 /* vim: set ft=c: */

docsrc/imap/reference/manpages/systemcommands/cyr_info.rst

@@ -59,8 +59,8 @@ configuring Cyrus easier.
 
 .. option:: conf-lint
 
-    Print only configuration options which are NOT recognised.  This
-    command should not print anything.  It uses cyrus.conf to find
+    Print configuration options which are NOT recognised, or are invalid.
+    This command should not print anything.  It uses cyrus.conf to find
     the names of configured services to avoid displaying any known
     configuration options for the named service.
 

imap/cyr_info.c

@@ -416,7 +416,10 @@ static void do_lint(void)
     /* check all overflow strings */
     config_foreachoverflowstring(lint_callback, &rock);
 
-    /* XXX - check directories and permissions? */
+    /* check partitions */
+    config_check_partitions(stdout);
+
+    /* XXX - permissions? */
 
     /* clean up */
     struct service_item *ks = rock.known_services;

lib/hash.c

@@ -343,10 +343,10 @@ EXPORTED void hash_enumerate(hash_table *table, void (*func)(const char *, void
       }
 }
 
-EXPORTED strarray_t *hash_keys(hash_table *table)
+EXPORTED strarray_t *hash_keys(const hash_table *table)
 {
+    const bucket *temp;
     unsigned i;
-    bucket *temp;
 
     strarray_t *sa = strarray_new();
 

lib/hash.h

@@ -87,7 +87,7 @@ void hash_enumerate_sorted(hash_table *table,void (*func)(const char *,void *,vo
                     void *rock, strarray_cmp_fn_t *cmp);
 
 /* gets all the keys from the hashtable */
-strarray_t *hash_keys(hash_table *table);
+strarray_t *hash_keys(const hash_table *table);
 
 /* counts the number of nodes in the hash table */