config partition sanity checker

[elliefm]

#5573 reports that, when Cyrus is configured with mail and search indexes being stored in the same directory, like this:

defaultpartition: default
partition-default: /var/spool/imap
defaultsearchtier: default
defaultsearchpartition-default: /var/spool/imap

... then compacting the xapian indexes will remove all the mail and metadata files. Seems like xapian, at least, expects total control over the directories it's configured to use. Given that, Cyrus shouldn't be allowed to start up in this configuration.

I don't think it makes sense for any of the *partition-* settings to share directories with one another. Even if some combination doesn't currently break, I don't believe we expect these to overlap, so that combination might break in the future.

This PR adds a sanity check to make sure that no partitions are sharing directories, nor are descendants of one another. If this sort of sharing is detected, master and squatter will now refuse to start, as will any other cyrus service or tool that requests partition data (via the CONFIG_NEED_PARTITION_DATA argument to cyrus_init()). Details of the collisions are logged to syslog. cyr_info conf-lint will detect and report the bad entries in the same way it reports other bad entries.

The sanity checking is based on string comparisons of the configured paths; actual filesystem details are not examined. So it's probably possible to subvert the checks using symlinks or other trickiness, but I don't think I care -- just don't do that.

For implementation reasons, if multiple partitions are configured as descendants of the same other partition, only the parent and one of the descendants can be reported. That's enough to prevent starting up with a bad configuration, but a really bad configuration might need several rounds of test/fix to identify and resolve all the issues.

If people already have configuration like this, what they can do to fix it prior to (or after) upgrading? Presumably rearrange their storage, then update their imapd.conf to describe the new layout. Hopefully they already know how to do this.

[rsto]

The code looks to do what the PR says it does, but I'd appreciate it getting somewhat more documented. I don't think a future reader will understand easily what that code does.

[rsto]

what is the interesting bit?

[ksmurchison]

Looks like SP has been moved to sort after /. Is that correct?

[elliefm]

The comment here only highlights which lines are actually different from a straight 00-ff order; the details are explained below, on the function where the lookup table is used.

Looks like SP has been moved to sort after /. Is that correct?

Not quite...

The 0x20 ... 0x2f rows have been rearranged to be 0x21 ... 0x2f, 0x20. This means when you look up index / (0x2f), the value retrieved is 0x20, which is the first printable. When you look up any other other index in the 0x2_ range (punctuation), the value you get back is one higher than it would otherwise have been, to compensate for 0x2f having been changed. This means / sorts before all other printable characters, but otherwise the relative sorting is unchanged. It does look like SP was moved, but this table is values retrieved, not the input chars, so actually it is / that was moved.

The comment on the function below says:

* Treats '/' (0x2f) as lower than other printables so that
 * file paths sort pre-order, depth-first.
 * XXX This should probably be in lib/bsearch.c, but that's in
 * XXX libcyrus, which we don't have here.
 */
static int cmpstringp_path(const void *aa, const void *bb)
{

Is this not sufficient?

[ksmurchison]

Code is approved. I agree with rsto that it could use some more commentary.

[wolfsage]

Nice safety check.

I don't think it accounts for symlinks. Should it? Or should the docs specify it has this weakness if not?

Beyond that, I think we should strarray_free's previous declaration as noted...

[elliefm]

I don't think it accounts for symlinks. Should it? Or should the docs specify it has this weakness if not?

I addressed that in the PR description already:

The sanity checking is based on string comparisons of the configured paths; actual filesystem details are not examined. So it's probably possible to subvert the checks using symlinks or other trickiness, but I don't think I care -- just don't do that.

If some sysadmin knows they can't configure multiple partitions to the same path (because Cyrus is refusing to start, or cyr_info conf-lint is complaining), but they manage to fudge things enough to trick the sanity checker into allowing it, the consequences are all theirs.

[wolfsage]

I don't think it accounts for symlinks. Should it? Or should the docs specify it has this weakness if not?

I addressed that in the PR description already:

Apologies, I had read the commit messages but skimmed the pr desc too quickly