- wrap flow-controlled statements that contain blocks into blocks
themselves
- wrap bodies of do-while()s into blocks
- use braces on 'else' symmetrically (this obviously has a cascading
effect, so this patch touches lots of lines)
- attach braces
unavoidably, the rules are sometimes broken around #ifdef-ery.
instead of specifying the mailboxes and sync state verbatim, use a
format which deals only with "subjects" (but no UIDs), and specifies the
whole state for each subject on a single line (exceptions prove the
rule).
the dumpers don't try to re-create the abstraction, as that's deemed
to be an unreasonable effort.
while rewriting most of the test data anyway, move it to the bottom of
the file, which is a more natural location for it.
don't abort the comparison if continuing makes sense, and try to be more
specific about the problems.
we give up if messages are excessive/missing or the subject is wrong,
as that touches upon the rather complex problem of diff optimization.
this avoids the ugly and error-prone repeated reading of the state
after a failure.
cmpbox() had to be made non-destructive on the box state.
readchan() had to be created.
parse the test data into hierarchical structures instead of using it in
its raw form. this is semantically cleaner and allows us to change the
input format more easily.
it doesn't really add any value when resuming after an interruption, as
incomplete propagations are perfectly expected, and are recovered from.
but it does make sense in the non-UIDPLUS path, where not finding just
stored messages indicates a problem.
so move the reporting from match_tuids() to msgs_found_new(), and remove
a redundant warning from the latter.
amends 6577bf3e.
while at it, add/fix some licenses/copyrights/comments:
- it makes no sense to have a GPL exception in scripts
- ted did not contribute to the man page
- tst_timers is not part of the mbsync executable
- explicitly put the build system under GPL and add copyrights
manual:
- explain what "rename on move" really means
- reword "remote" to "opposite" to make it less confusing
(possibly renaming TrashRemoteNew left as an exercise for later)
- mention example mbsyncrc
- consistently capitalize Store/Channel/Group where they refer to the
respective configuration entities
- emphasize that SyncState may need a trailing slash (as we do for Path)
- fix missing mention of global usage/default for some options
example mbsyncrc:
- add big fat note that empty lines matter
- stop demoing deprecated options
- point out that CertificateFile is optional
REFMAIL: 877dd11jb3.fsf@angela.anarc.at
the messages are trashed in mailbox (and thus UID) order, and in
practice we expect the operations to complete in order. however, if
older messages need to be trashed after a journal replay, and we get
interrupted again, the next replay would produce an unsorted array,
and thus break the binary search.
amends 2bba9b9.
there isn't really a reason for that; DEBUG_CRASH is quite unlike the
other DEBUG_ flags.
note that the DEBUG_*_ALL flags are not checked, because they always
come with their corresponding less verbose flag anyway.
copy-pasto; it certainly wasn't meant to respond to --debug-sync.
the problem was barely noticeable, as the maildir driver's only debugs
are in the rarely triggered rescan path, apart from the flags usually
being used en bloc anyway.
amends 0e1f8f9a.
in certain configurations, under very unlikely conditions (which are
practically impossible to control remotely), we'd overflow ranges[].
in a typical gcc build, the values (which are also practically
impossible to control remotely) would be written at the end of buf[],
which would be rather harmless, as only a tiny part of buf is used
subsequently. so i'm not classifying this as a security issue.
amends 77acc268.
the input isn't necessarily null-terminated (it currently is for imap,
but not for maildir), so if the message ended somewhere within the
header field name, we'd read beyond its end, which theoretically could
cause a crash. no other adverse effects could result, as we'd stop
processing such a broken message right afterwards.
amends 70bad661.
this wasn't really a security problem, as the name mapping we actually
do does not change the string length, and the iteration was already
safe after the literal length fix, but it's still better to catch weird
input.
that shouldn't really be a problem, as we have 2GB of headroom, and most
growth would happen when sending an all-newlines message from maildir to
imap (due to CR additions), which is mostly non-critical. but better
safe than sorry.
don't try to read messages > 2G, as that will only lead to trouble down
the line.
this wouldn't have worked on linux anyway (we read in one chunk, and
that is limited to (2^31 - 2^12) on all architectures), but on
platforms were big reads work, this was a security problem if one
synchronized other users' maildirs.
as a minor fix on the side, we now also clip the reported message size,
so MaxSize works for excessively big messages.
we didn't limit the 32-bit size of literals so far, which, given that we
use int-sized lengths & offsets, permitted all kinds of buffer
overflows. malicious/compromised servers may have been able to exploit
this. actual email senders would be constrained by size limits for
delivered mails, and to cause more than a crash they'd have to predict
the exact size of the final message.
we now limit to 2GB, which, given that we use unsigned ints since
e2d3b4d55 (v1.4.0), gives the handlers downstream plenty of headroom.
an alternative would have been using 64-bit offsets, but this seems like
major overkill, even if IMAP4rev2 recently mandated it (we talk only
IMAP4rev1, so we can ignore it).
when a broken/compromised/malicious server gives us a message that
starts with an empty line, we'd enter the path for inserting a pristine
placeholder subject, for which we unfortunately didn't actually allocate
space (unless MaxSize is in use and the message exceeds it).
note that this cannot be triggered by merely receiving a crafted mail
with no headers (yes, it's actually possible to send such a thing), as
the delivery of mails adds plenty of headers.
amends 70bad661.
this is a cheap way to catch symlink loops. 10 seems like a reasonable
limit, as it's unlikely that anyone would be able to actually work with
such a deeply nested mailbox tree.
fixes debian bug #990117.
the AUTHENTICATE command may get insanely long for GSSAPI when SASL-IR
is available. instead of growing the buffers each time someone hits the
limit (as done in f7cec306), remove the limitation altogether.
imap_vprintf() still contains a fixed-size buffer which could overflow
when really long strings (e.g., mailbox names) need to be quoted. this
seems very unlikely, so we'll deal with it if someone actually hits it.
REFMAIL: 87sg1qxdye.fsf@cern.ch
if the code was sent in response to anything but a STORE, we'd overwrite
a data pointer in one of our imap_cmd subclasses, an allocator data
structure, or the start of the next allocation, with an int that was
completely under the server's control. it's plausible that this could be
exploited for remote code execution.
to avoid this, we could ensure that the object is of the right type
prior to casting, by using a new flag in the parameter block. but it's
easier to just dispose of the out_uid field altogether and reuse the uid
field that is present in the parameter block anyway, but was used only
for FETCH commands so far.
this problem was found by Lukas Braun <koomi@moshbit.net> using a
fuzzer.
while it's technically reasonable to expect the user to match the
server's casing of INBOX if they set Path, this might come as a
surprise to those who know that the IMAP INBOX is case-insensitive.
so tolerate any casing instead. as a minor side effect, we'd now even be
able to deal with a server using different casing in NAMESPACE and LIST.
in particular, this covers the case of a mailbox being replaced with an
empty new one, which would subsequently lead to the opposite end being
emptied as well, which would typically be undesired.
also add plenty of comments.
don't print the actual values, which are meaningless technicalities
to the average user, and can be obtained separately for debugging if
really necessary.
also, fix the omission of the affected mailboxes from one of the
messages.
in particular, '..' in the name could be used to escape the Path/Inbox
of a Maildir Store, which could be exploited for stealing or deleting
data, or staging a (mild) DoS attack.
fastmail sends flags containing ']' in PERMANENTFLAGS, which is formally
illegal. however, if we parse the embedded list before looking for the
response code's closing ']', things work out fine.
as a side effect we won't complain about similarly or completely
malformed response codes we don't recognize at all, which may or may not
be considered an improvement ...
on error, parse_imap_list() needs to reset the nesting level in the
state, as imap_socket_read() uses that as an indicator whether list
parsing is ongoing.
while the spec says that the server SHOULD not send FETCH responses
about STORE FLAGS when .SILENT is used, at least gmail and fastmail seem
to do it nonetheless. also, in case of concurrent flag updates on the
affected messages such responses can be legitimately sent.
in earlier versions of mbsync this would lead to duplicate messages
piling up in the store, though that would pose no problem at that point.
In POSIX, poll() should be accessible using <poll.h>, although most
implementations keep <sys/poll.h> to avoid breakage. This fixes some
warnings when building on musl.
The SASL library will refuse to use the EXTERNAL module when no auth id
is set a priori.
Tested to work with Dovecot, using TLS client certificates for
authentication.
to test async operation of the syncing core while using the synchronous
maildir driver, we add a mode to the proxy driver where it queues
callback invocations to the next main loop iteration.
the struct declarations got uglier, but their usage requires a lot fewer
explicit references to the parent struct (though some are added where
using the derived struct is more practical now).
we also use something i'd term "covariant members": derivatives of
store_t also reference derivatives of store_conf_t, etc., which
drastically cuts down the number of casts.
fwiw, to achieve this with "proper" inheritance in C++, we'd use
covariant getter functions which hide the still existing casts.
C11 is almost a decade old now, and compilers supported that feature
even longer than that, so i don't expect this to be a problem.
use the indentation of the placeholder, not the replacement.
this doesn't matter right now, as all placeholders are indented by one
step, but that will change soon.
the indent function cannot be inlined into the substitution, as for some
reason ^ then matches the end of the string, not the embedded line
starts (with perl v5.32). also, $1 needs to go into a temporary anyway.
this is a de-optimization, but it makes the code consistent with the
other sections (which do not use the shortcut due to having to
post-process the data or being encapsulated by a function call).
that's mostly hypothetical, but let's not make assumptions.
this also adds EXPUNGE response handling to make total_msgs reliable. in
principle, this affects the post-SELECT UIDNEXT fallback as well, but
there the racing window is so short that this barely improves anything.
amends 94022a67.
the uidnext query following message stores can be interleaved with
message fetches. that means that we cannot rely on the 1st command in
flight being that query. but instead of iterating over all commands in
flight, move the uidnext query flag to imap_store (and make sure to
check for the presence of a message body before testing it) - this
avoids the loop and an extra byte in every command.
this also makes it clear that the query is mutually exclusive with
loading messages (the untagged responses are not distinguishable).
don't say DRV_CANCELED when it's really DRV_STORE_BAD, as apart from
being just wrong, it lead to the confusing effect of canceling a store
as the result of a supposed cancellation of the same store.
properly distribute the certificates between the SSL context's trust
store and our host cert list.
as a drive-by, clean up some nasty type casts at the cost of including
a second OpenSSL header into socket.h.
delay reporting success of STORE FLAGS until a subsequent CHECK
succeeds.
this fixes (inverse flag change propagation) and (deletes not being
propagated) after an interruption due to prematurely logged flag
updates.
delay the creation of the new state and journal until there is actually
something interesting to write. this saves some cpu cycles and prolongs
ssd life a whee bit.
now that expiration order is determined by a single loop ordered by
far-side UIDs, it is no longer necessary to accurately track the highest
seen UID.
as a side effect, this fixes a problem reported (way too long ago) by
Yuri D'Elia: we failed to up newmaxuid for messages we produced
ourselves, so we would keep enumerating the same messages until we also
propagated externally generated messages from that mailbox - which might
have been never for the server side of archive/trash mailboxes.
we can do that, as unpaired near-side messages are ignored anyway.
this mildly changes expiration order, as near-side messages that
existed for a long time but were propagated much later will be expired
later. however, that has no practical relevance.
this is mostly theoretical, as at this point no updates to the message
list can have actually happened. but it's future-proof and consistent
with the near-side loop.
we already didn't propagate messages which would be instantly expunged
from the target, but failed to cancel propagations that were already
scheduled before we got interrupted. this matters a bit when the
resumption happens significantly later than the initial attempt, giving
the user time to mark messages on the source as deleted.
the 'pending' and 'skipped' sync record states are mutually exclusive
with having a complementary message, so there is no point in testing it
explicitly.
amends bd5fb6ff.
we need to pass a different "boundary" UID to driver_t::load_box() for
every OPEN_* flag that queries a partial range:
- OPEN_FIND refers to messages newer than all we know about
- OPEN_OLD_IDS refers to messages which are paired
- OPEN_{OLD,NEW}_SIZE refers to messages (not) above the committed
boundary of already propagated messages
we treated the 3rd like the 2nd, which was just wrong - the actual
boundary may be lower or higher, so we'd produce wrong results when
MaxSize was set and only one of New and ReNew was requested.
the underlying metaphor refers to an inhumane practice, so using it
casually is rightfully offensive to many people. it isn't even a
particularly apt metaphor, as it suggests a strict hierarchy that is
counter to mbsync's highly symmetrical mode of operation.
the far/near terminology has been chosen as the replacement, as it is a
natural fit for the push/pull terminology. on the downside, due to these
not being nouns, a few uses are a bit awkward, and several others had to
be amended to include 'side'. also, it's conceptually quite close to
remote/local, which matches the typical use case, but is maybe a bit too
suggestive of actually non-existing limitations.
the new f/n suffixes of the -C/-R/-X options clash with pre-existing
options, so direct concatenation of short options is even less practical
than before (some suffixes of -D already clashed), but doing that leads
to unreadable command lines anyway.
as with previous deprecations, all pre-existing command line and config
options keep working, but yield a warning. the state files are silently
upgraded.
this is better than using PassCmd, as it allows the keychain manager to
identify the calling process and therefore use a selective whitelist.
unlike in the now removed example, we use an "internet password" for the
imap protocol, rather than a "generic password" - this seems more
appropriate.
based on a patch by Oliver Runge <oliver.runge@gmail.com>
It was already possible to retrieve passwords from arbitrary commands.
But this goes only half the way to allowing automated derivation of
login credentials, as some environments may also have different user
names based on the system. Therefore, add the UserCmd option to
complement PassCmd.
Based on a patch series by Patrick Steinhardt <ps@pks.im>
makes the code less cluttered, and it's harder to introduce leaks.
this has the hypothetical disadvantage that due to freeing being
delayed, the peak memory usage would rise significantly if we chained to
another parse_list() call which produces a big list while already
holding a big list, but that isn't the case anywhere.
... by making a lot of objects unsigned, and some signed.
casts which lose precision and change the sign in one go (ssize_t and
time_t to uint on LP64) are made explicit as well.
this does specifically *not* cover about a bazillion warnings about
size_t being shrunk to uint - these make no sense given the expected
data set size.
mostly ATTR_PRINTFLIKE(*, 0) for functions with a va_list argument.
also, one ATTR_NORETURN and one ATTR_UNUSED, both on functions.
also, an explicit suppression for a format string stored in a variable.
this is mostly to work around the fact that both gcc and clang won't
accept the format string declaration (i.e., will complain with
-Wformat-nonliteral) if the *called* function does not actually take a
va_list.
on the upside, it makes one caller cleaner. yay ...
this is actually potentially counterproductive, as people who have set
SSLVersions and fail to adjust it will _lose_ tls 1.3 support. however,
without the option being there, people (incorrectly) believe that tls
1.3 is not supported.
otherwise the server would interpret it as INBOX contrary to our
expectations, which might lead to moderately surprising effects.
if you really want to sync your ~/maildir/inbox to the IMAP INBOX,
specify it as the Maildir Store's Inbox.
Some distributions (e.g. Fedora) added support for system wide crypto
policies. This is supported in most common crypto libraries including
OpenSSL. Applications can override this policy using their own cipher
string. This commit adds support for specifying the cipher string in
the mbsync configuration.
For example, to exclude Diffie-Hellman, the user can specify
CipherString "DEFAULT:!DH"
in the IMAP Account's configuration.
this is semantically cleaner, and fixes storing the flags in the rare
case that flags are not being synced and the target is not being
expunged, as in this case flags are queried only during the actual
propagation.
the workaround for -Wformat triggered -Wformat-nonliteral in turn.
so instead go back to using pragmas and add a proper gcc version check.
this also works with clang - mostly for qt-creator's code model, which
is clang-based.
amends/reverts 55e65147.
the assumption was that this wouldn't be needed, as maildir_store_msg()
reliably delivers a UID. however, if we crash right before the callback
can record that UID, we'd still use OPEN_FIND in the next run, which
requires the saved next UID.
an effect of 7ce658d is that we can index messages by UID rather than
content (or more specifically, subject). apart from being cleaner, it
allows duplicated subjects.
on modern systems, this makes it likely to end up on tmpfs, which is a
lot faster and ssd-friendlier.
the symlink is not deleted at the end, to minimize fs churn. that means
it will be dangling after a reboot, which gets fixed in the next run.
the operator was exactly inverted. that means that it actually wouldn't
compile with both older versions (that needed the aliases) and
potentially new versions (that will hide the data members - still not
the case as of 3.2).
amends 8a40554f0.
we failed to reset the box list pointer after freeing it, which would
lead to a crash.
we also failed to reset the listing status, which would lead to
malfunction if we hadn't already crashed.
this inlines imap_cleanup_store(), as there isn't much value in keeping
it. the message list is already freed when disowning the store anyway.
we need to deep-copy the struct hostent data, as otherwise the
concurrent connects will overwrite each other's lookup results.
this is a rather hypothetical fix, as the bug currently affects only
channels connecting two IMAP accounts, and only if the first host's
first address asynchronously fails to connect.
this is relevant only when listing an IMAP Store's contents, as that's
the only place where we aren't imposing the spelling ourselves.
we need to be careful not to treat our own canonical (prefix-stripped
and always slash-delimited) box names like that; codify that in
comments.
this reveals that commit 6f2160f1 may be deemed to have been incorrect -
the TODO item was ambiguous, and could quite possibly have meant this
fix. unsurprisingly, 380ccdd4 re-introduced it with more explicit
wording.
the query is untypical enough to have caused problems with davmail (when
we still used *:*) and mailo.com (until it got fixed), so better check
that the result (not) returned by the server makes sense.
we did already set up the timeout when starting to send commands, but so
far we did not reset it when succeeding to send out data. rectify that.
REFFAIL: 87sgy92we3.fsf@jnanam.net
