Browse Source

*** various workarounds for exchange being braindead

among other things, this contains a possible fix for
https://sourceforge.net/p/isync/bugs/22/ and a lot of related reports.

patch by Florian Lombard <f.lombard@montmirail.com>:

Common cfg section:

  * Either skip or fix messages with lines more than xxx bytes
    (typically no more than 9900 bytes with exchange)
    MaxLineLength xxx (in bytes)
    CutLongLines yes|no (fix or skip message)
  * Allow to rescan all mails from a folder, ignoring the last sync
    latest message pulled (usefull when playing with my new settings)
    IgnoreMaxPulledUid yes|no
  * Skip messages with raw binary content (bytes < 0x20 except CR/LF/TAB)
    SkipBinaryContent yes|no
  * Allow to delete non empty folders on slave (when you are sure about
    what you're doing)
    DeleteNonEmpty yes|no

Drivers cfg section (imap only):

  * Suppress Keyword not supported warnings
    IgnoreKeywordWarnings yes|no

The only missing part is long lines cutting when there's CR/LF
convertion (I don't use maildir++)

============

my response:

> Common cfg section:
>
>   * Either skip or fix messages with lines more than xxx bytes
>     (typically no more than 9900 bytes with exchange)
>     MaxLineLength xxx (in bytes)
>     CutLongLines yes|no (fix or skip message)
>
as mentioned before, i'm concerned about the "sledge hammer" approach of
hard-cutting the lines, because that falsifies the messages' content,
which may very well render them unreadable (if it's not plain text).

meanwhile i found that this should at least not invalidate possibly
present signatures, simply because the respective standards require
complete normalization of the contents before signing - specifically to
avoid the problem.

still, a cleaner approach would be encapsulating the message in a MIME
structure. i found in the imapsync FAQ that "reformime -r7" would do
that (i'm not suggesting to use that, but it should serve as a good
example).

i'd be interested in samples of such messages with excessively long
lines to assess what the "target audience" actually is. i would expect
that messages which already are MIME-encoded would not have this
problem. but then, a sloppily encoded multipart text+html mail could
very well be broken as well.

>   * Allow to rescan all mails from a folder, ignoring the last sync
>     latest message pulled (usefull when playing with my new settings)
>     IgnoreMaxPulledUid yes|no
>
that seems to be overkill to me given that it's a workaround and can be
easily achieved by hacking the sync state files, for example by sed'ing
them.
i suppose you implemented this to resume syncing after implementing the
line length workaround?

>   * Skip messages with raw binary content (bytes < 0x20 except CR/LF/TAB)
>     SkipBinaryContent yes|no
>
i know that i suggested that this might be a problem, but i don't
remember whether you reported actual instances of that.
anyway, the treatment should be the same as for messages with excesively
long lines - MIME-encoding (presumably as quoted-printable).

>   * Allow to delete non empty folders on slave (when you are sure about
>     what you're doing)
>     DeleteNonEmpty yes|no
>
i'll consider this.
my biggest concern is that some transient error would falsify the
mailbox list and thus cause the folders to be nuked. similary, a
permanent change in the server configuration would have that effect.
arguably, either wouldn't be so bad as such, as it would destroy only
the replica. however, it would be important to verify that the replica
does not contain any unpropagated mails (as opposed to any mails at all,
as is done currently).

> Drivers cfg section (imap only):
>
>   * Suppress Keyword not supported warnings
>     IgnoreKeywordWarnings yes|no
>
i wonder why a server would bleat about not supporting an optional
feature when it can (and probably does) announce that in a "civilized"
way, too. did these responses appear to be correlated with specific
messages, or did they always come when opening any mailbox?

> diff --git a/src/drv_imap.c b/src/drv_imap.c
> index e24c7d8..10da0cb 100644
> --- a/src/drv_imap.c
> +++ b/src/drv_imap.c
> @@ -1416,6 +1419,16 @@ imap_socket_read( void *aux )
>                                         resp = RESP_NO;
>                                         if (cmdp->param.failok)
>                                                 goto doresp;
> +                               } else if (!strcmp( "BAD", arg )) {
> +                                       resp = RESP_NO;
> +                               warn( "Warning: IMAP command '%s' returned an error: %s %s\n",
> +                                      starts_with( cmdp->cmd, -1, "LOGIN", 5 ) ?
> +                                          "LOGIN <user> <pass>" :
> +                                          starts_with( cmdp->cmd, -1, "AUTHENTICATE PLAIN", 18 ) ?
> +                                              "AUTHENTICATE PLAIN <authdata>" :
> +                                               cmdp->cmd,
> +                                      arg, cmd ? cmd : "" );
> +                                       goto doresp;
>                                 } else /*if (!strcmp( "BAD", arg ))*/
>                                         resp = RESP_CANCEL;
>
this hunk downgrades tagged BAD responses to warnings and suppresses the
subsequent client-side connection drop.
this doesn't seem like a terribly good idea to me - this server response
indicates that the client (allegedly) did something wrong. that may mean
that the subsequent command stream will be interpreted as garbage, which
may have unpredictable effects. it just isn't safe to continue at this
point.
i suppose you implemented this as a workaround before you identified the
line length issue?

============

and a last retour:

>> Common cfg section:
>>
>>    * Either skip or fix messages with lines more than xxx bytes
>>      (typically no more than 9900 bytes with exchange)
>>      MaxLineLength xxx (in bytes)
>>      CutLongLines yes|no (fix or skip message)
> as mentioned before, i'm concerned about the "sledge hammer" approach of
> hard-cutting the lines, because that falsifies the messages' content,
> which may very well render them unreadable (if it's not plain text).

Well you have the choice of just skipping them to allow the sync to
complete if you're concerned about the messages integrity

> meanwhile i found that this should at least not invalidate possibly
> present signatures, simply because the respective standards require
> complete normalization of the contents before signing - specifically to
> avoid the problem.
>
> still, a cleaner approach would be encapsulating the message in a MIME
> structure. i found in the imapsync FAQ that "reformime -r7" would do
> that (i'm not suggesting to use that, but it should serve as a good
> example).

I had a look at that, and found that completely overkill for my usage
(see below)

> i'd be interested in samples of such messages with excessively long
> lines to assess what the "target audience" actually is. i would expect
> that messages which already are MIME-encoded would not have this
> problem. but then, a sloppily encoded multipart text+html mail could
> very well be broken as well.

100% of those messages where having bad html code without line breaks
Non binary attachments where always correctly line wrapped.
It was either poorly done html signatures or even javascript (yeah,
inside an email !)
So I wasn't worried about the integrity of those messages, which where
already breaking the rules, but I needed the contents (messages from
customers we needed to keep)

>>    * Allow to rescan all mails from a folder, ignoring the last sync
>>      latest message pulled (usefull when playing with my new settings)
>>      IgnoreMaxPulledUid yes|no
> that seems to be overkill to me given that it's a workaround and can be
> easily achieved by hacking the sync state files, for example by sed'ing
> them.
> i suppose you implemented this to resume syncing after implementing the
> line length workaround?

Yes it was mainly a flag I used for debugging (editing hundreds of sync
state files wasn't an option)

>>    * Skip messages with raw binary content (bytes < 0x20 except CR/LF/TAB)
>>      SkipBinaryContent yes|no
> i know that i suggested that this might be a problem, but i don't
> remember whether you reported actual instances of that.
> anyway, the treatment should be the same as for messages with excesively
> long lines - MIME-encoding (presumably as quoted-printable).

Those where bogus messages with the raw attachment in binary but with
base 64 headers correctly set.
Near 100% (if not 100%) of those where in the sent folder and are
probably the result of gmail + buggy email client (but you can still
open the attachment with gmail !)

>>    * Allow to delete non empty folders on slave (when you are sure about
>>      what you're doing)
>>      DeleteNonEmpty yes|no
> i'll consider this.
> my biggest concern is that some transient error would falsify the
> mailbox list and thus cause the folders to be nuked. similary, a
> permanent change in the server configuration would have that effect.
> arguably, either wouldn't be so bad as such, as it would destroy only
> the replica. however, it would be important to verify that the replica
> does not contain any unpropagated mails (as opposed to any mails at all,
> as is done currently).

Well, when you are sure about your settings, this can be usefull, as my
users where renaming folders while I was working on the sync
At start I was logging to the mailbox, deleted the folder, and syncing
again.

>> Drivers cfg section (imap only):
>>
>>    * Suppress Keyword not supported warnings
>>      IgnoreKeywordWarnings yes|no
>>
> i wonder why a server would bleat about not supporting an optional
> feature when it can (and probably does) announce that in a "civilized"
> way, too. did these responses appear to be correlated with specific
> messages, or did they always come when opening any mailbox?

Well, "exchange online", that sums it all ...
Tied to specific messages, I guess it happened when there was a word
between bracket in the message subject (no debug log of that)
Happends only one time, when the message is synced.
A rather ugly hack, but I needed clean logs to spot errors.

> i suppose you implemented this as a workaround before you identified the
> line length issue?

I implemented that before the binary content issue
It's exchange which is breaking all the rules that "forced" me to do
that to sync most of the messages
Cutting the connexion instead of reporting the right error is not the
right thing to do, but that's what exchange does (with Error 10 or 11,
but with BAD reponse)
wip/exchange-workarounds-1.2
Oswald Buddenhagen 7 years ago
parent
commit
0ba4d6e8a9
  1. 10
      src/config.c
  2. 17
      src/drv_imap.c
  3. 118
      src/sync.c
  4. 16
      src/sync.h

10
src/config.c

@ -202,6 +202,16 @@ getopt_helper( conffile_t *cfile, int *cops, channel_conf_t *conf )
conf->sync_state = expand_strdup( cfile->val );
else if (!strcasecmp( "CopyArrivalDate", cfile->cmd ))
conf->use_internal_date = parse_bool( cfile );
else if (!strcasecmp( "CutLongLines", cfile->cmd ))
conf->cut_lines = parse_bool( cfile );
else if (!strcasecmp( "IgnoreMaxPulledUid", cfile->cmd ))
conf->ignore_max_pulled_uid = parse_bool( cfile );
else if (!strcasecmp( "SkipBinaryContent", cfile->cmd ))
conf->skip_binary_content = parse_bool( cfile );
else if (!strcasecmp( "DeleteNonEmpty", cfile->cmd ))
conf->delete_nonempty = parse_bool( cfile );
else if (!strcasecmp( "MaxLineLength", cfile->cmd ))
conf->max_line_len = parse_int( cfile );
else if (!strcasecmp( "MaxMessages", cfile->cmd ))
conf->max_messages = parse_int( cfile );
else if (!strcasecmp( "ExpireUnread", cfile->cmd ))

17
src/drv_imap.c

@ -66,6 +66,7 @@ typedef struct imap_store_conf {
imap_server_conf_t *server;
char delimiter;
char use_namespace;
char ignore_keyword_warnings;
} imap_store_conf_t;
typedef struct imap_message {
@ -1259,6 +1260,7 @@ static void
imap_socket_read( void *aux )
{
imap_store_t *ctx = (imap_store_t *)aux;
imap_store_conf_t *cfg = (imap_store_conf_t *)ctx->gen.conf;
struct imap_cmd *cmdp, **pcmdp;
char *cmd, *arg, *arg1, *p;
int resp, resp2, tag;
@ -1326,7 +1328,8 @@ imap_socket_read( void *aux )
error( "IMAP error: bogus greeting response %s\n", arg );
break;
} else if (!strcmp( "NO", arg )) {
warn( "Warning from IMAP server: %s\n", cmd );
if (!strcmp( "Keywords are not supported", arg) && !cfg->ignore_keyword_warnings)
warn( "Warning from IMAP server: %s\n", cmd );
} else if (!strcmp( "BAD", arg )) {
error( "Error from IMAP server: %s\n", cmd );
} else if (!strcmp( "CAPABILITY", arg )) {
@ -1416,6 +1419,16 @@ imap_socket_read( void *aux )
resp = RESP_NO;
if (cmdp->param.failok)
goto doresp;
} else if (!strcmp( "BAD", arg )) {
resp = RESP_NO;
warn( "Warning: IMAP command '%s' returned an error: %s %s\n",
starts_with( cmdp->cmd, -1, "LOGIN", 5 ) ?
"LOGIN <user> <pass>" :
starts_with( cmdp->cmd, -1, "AUTHENTICATE PLAIN", 18 ) ?
"AUTHENTICATE PLAIN <authdata>" :
cmdp->cmd,
arg, cmd ? cmd : "" );
goto doresp;
} else /*if (!strcmp( "BAD", arg ))*/
resp = RESP_CANCEL;
error( "IMAP command '%s' returned an error: %s %s\n",
@ -2968,6 +2981,8 @@ imap_parse_store( conffile_t *cfg, store_conf_t **storep )
store->server = srv;
} else if (!strcasecmp( "UseNamespace", cfg->cmd ))
store->use_namespace = parse_bool( cfg );
else if (!strcasecmp( "IgnoreKeywordWarnings", cfg->cmd ))
store->ignore_keyword_warnings = parse_bool( cfg );
else if (!strcasecmp( "Path", cfg->cmd ))
store->gen.path = nfstrdup( cfg->val );
else if (!strcasecmp( "PathDelimiter", cfg->cmd )) {

118
src/sync.c

@ -302,13 +302,15 @@ copy_msg( copy_vars_t *vars )
static void msg_stored( int sts, int uid, void *aux );
static void
copy_msg_bytes( char **out_ptr, const char *in_buf, int *in_idx, int in_len, int in_cr, int out_cr )
copy_msg_bytes( char **out_ptr, const char *in_buf, int *in_idx, int in_len, int in_cr, int out_cr, int max_line_len )
{
char *out = *out_ptr;
int idx = *in_idx;
if (out_cr != in_cr) {
/* message needs to be converted */
char c;
if (out_cr) {
/* adding CR */
for (; idx < in_len; idx++) {
if ((c = in_buf[idx]) != '\r') {
if (c == '\n')
@ -317,15 +319,52 @@ copy_msg_bytes( char **out_ptr, const char *in_buf, int *in_idx, int in_len, int
}
}
} else {
/* removing CR */
for (; idx < in_len; idx++) {
if ((c = in_buf[idx]) != '\r')
*out++ = c;
}
}
} else {
memcpy( out, in_buf + idx, in_len - idx );
out += in_len - idx;
idx = in_len;
/* no CRLF change */
if (max_line_len > 0) {
/* there are too long lines in the message */
const char * curLine = in_buf + *in_idx;
int lines = 0;
while(curLine) {
char * nextLine = strchr(curLine, '\n');
int curLineLen = nextLine ? (unsigned int)(nextLine-curLine) + 1 : strlen(curLine);
if (curLineLen > max_line_len) {
/* this line need to be cut into smaller lines */
int line_idx = 0;
while(line_idx < curLineLen) {
memcpy(out, in_buf + idx + line_idx, ((curLineLen - line_idx) < max_line_len) ? (curLineLen - line_idx) : max_line_len);
out += (((curLineLen - line_idx) < max_line_len) ? (curLineLen - line_idx) : max_line_len);
line_idx += ((curLineLen - line_idx) < max_line_len) ? (curLineLen - line_idx) : max_line_len;
if( line_idx < curLineLen) {
/* add (CR)LF except for the last line */
if (out_cr)
*out++ = '\r';
*out++ = '\n';
}
}
idx += curLineLen;
} else {
/* simple copy */
memcpy (out , in_buf + idx, curLineLen);
out += curLineLen;
idx += curLineLen;
}
curLine = nextLine ? (nextLine+1) : NULL;
lines++;
}
//debug("End index %d (message size %d), message size should be %d\n", idx, in_len, *in_idx + out - *out_ptr);
} else {
/* simple copy */
memcpy( out, in_buf + idx, in_len - idx );
out += in_len - idx;
idx = in_len;
}
}
*out_ptr = out;
*in_idx = idx;
@ -338,7 +377,41 @@ copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars )
int in_len = vars->data.len;
int idx = 0, sbreak = 0, ebreak = 0;
int lines = 0, hdr_crs = 0, bdy_crs = 0, app_cr = 0, extra = 0;
int extra_bytes = 0;
if (vars->srec) {
/* check if the message has too long lines if enabled */
if(global_conf.max_line_len) {
char * curLine = in_buf;
while(curLine) {
char * nextLine = strchr(curLine, '\n');
uint curLineLen = nextLine ? (unsigned int)(nextLine-curLine) + 1 : strlen(curLine);
if (curLineLen > global_conf.max_line_len) {
if (global_conf.cut_lines) {
/* compute the addded lines as we are going to cut them */
if (out_cr)
extra_bytes += curLineLen / global_conf.max_line_len; // CR
extra_bytes += curLineLen / global_conf.max_line_len; // LF
} else {
/* stop here with too long line error */
free( in_buf );
return 2;
}
}
curLine = nextLine ? (nextLine+1) : NULL;
}
}
if(global_conf.skip_binary_content) {
while (idx < in_len) {
unsigned char c = in_buf[idx++];
if (c < 0x20 && c != '\r' && c != '\n' && c != '\t') {
/* binary content, skip */
debug("Incorrect byte %d at offset %d/%d\n", c, idx, in_len);
free( in_buf );
return 3;
}
}
idx = 0;
}
nloop: ;
int start = idx;
int line_crs = 0;
@ -380,11 +453,12 @@ copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars )
extra += lines;
}
vars->data.len = in_len + extra;
vars->data.len = in_len + extra + extra_bytes;
char *out_buf = vars->data.data = nfmalloc( vars->data.len );
idx = 0;
if (vars->srec) {
copy_msg_bytes( &out_buf, in_buf, &idx, sbreak, in_cr, out_cr );
//debug ("Calling copy_msg_bytes for the header (0 to %d) with %d extra bytes\n", sbreak, extra);
copy_msg_bytes( &out_buf, in_buf, &idx, sbreak, in_cr, out_cr, 0 );
memcpy( out_buf, "X-TUID: ", 8 );
out_buf += 8;
@ -395,8 +469,10 @@ copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars )
*out_buf++ = '\n';
idx = ebreak;
}
copy_msg_bytes( &out_buf, in_buf, &idx, in_len, in_cr, out_cr );
//debug ("Calling copy_msg_bytes for the body (at %d) with %d extra byte(s), limit is %d \n", ebreak, extra_bytes, extra_bytes > 0 ? global_conf.max_line_len : 0);
copy_msg_bytes( &out_buf, in_buf, &idx, in_len, in_cr, out_cr, extra_bytes > 0 ? global_conf.max_line_len : 0 );
//debug("Message after %s\n", vars->data.data);
//debug("Good message size should be %d + %d\n",vars->data.len-extra, extra);
free( in_buf );
return 1;
}
@ -422,11 +498,24 @@ msg_fetched( int sts, void *aux )
scr = (svars->drv[1-t]->flags / DRV_CRLF) & 1;
tcr = (svars->drv[t]->flags / DRV_CRLF) & 1;
if (vars->srec || scr != tcr) {
if (!copy_msg_convert( scr, tcr, vars )) {
switch (copy_msg_convert( scr, tcr, vars )) {
case 0:
warn( "Warning: message %d from %s has incomplete header.\n",
vars->msg->uid, str_ms[1-t] );
vars->cb( SYNC_NOGOOD, 0, vars );
return;
case 2:
warn( "Warning: message %d from %s has too long line(s).\n",
vars->msg->uid, str_ms[1-t] );
vars->cb( SYNC_MALFORMED, 0, vars );
return;
case 3:
warn( "Warning: message %d from %s has raw binary content.\n",
vars->msg->uid, str_ms[1-t] );
vars->cb( SYNC_MALFORMED, 0, vars );
return;
default:
break;
}
}
@ -779,7 +868,11 @@ load_state( sync_vars_t *svars )
}
svars->existing = 0;
}
svars->newmaxuid[M] = svars->maxuid[M];
if (global_conf.ignore_max_pulled_uid) {
svars->newmaxuid[M] = 0;
} else {
svars->newmaxuid[M] = svars->maxuid[M];
}
svars->newmaxuid[S] = svars->maxuid[S];
svars->mmaxxuid = INT_MAX;
line = 0;
@ -1076,7 +1169,7 @@ box_confirmed2( sync_vars_t *svars, int t )
svars->chan->name, str_ms[t], svars->orig_name[t] );
goto bail;
}
if (svars->drv[1-t]->confirm_box_empty( svars->ctx[1-t] ) != DRV_OK) {
if ((svars->drv[1-t]->confirm_box_empty( svars->ctx[1-t] ) != DRV_OK) && !global_conf.delete_nonempty) {
warn( "Warning: channel %s: %s %s cannot be opened and %s %s not empty.\n",
svars->chan->name, str_ms[t], svars->orig_name[t], str_ms[1-t], svars->orig_name[1-t] );
goto done;
@ -1821,6 +1914,9 @@ msg_copied( int sts, int uid, copy_vars_t *vars )
svars->state[t] |= ST_FIND_NEW;
msg_copied_p2( svars, vars->srec, t, uid );
break;
case SYNC_MALFORMED:
msg_copied_p2( svars, vars->srec, t, -2 );
break;
case SYNC_NOGOOD:
debug( " -> killing (%d,%d)\n", vars->srec->uid[M], vars->srec->uid[S] );
vars->srec->status = S_DEAD;

16
src/sync.h

@ -55,6 +55,11 @@ typedef struct channel_conf {
uint max_messages; /* for slave only */
signed char expire_unread;
char use_internal_date;
uint max_line_len;
char cut_lines;
char ignore_max_pulled_uid; /* for master only */
char skip_binary_content; /* for master only */
char delete_nonempty;
} channel_conf_t;
typedef struct group_conf {
@ -69,11 +74,12 @@ extern group_conf_t *groups;
extern const char *str_ms[2], *str_hl[2];
#define SYNC_OK 0 /* assumed to be 0 */
#define SYNC_FAIL 1
#define SYNC_BAD(ms) (4<<(ms))
#define SYNC_NOGOOD 16 /* internal */
#define SYNC_CANCELED 32 /* internal */
#define SYNC_OK 0 /* assumed to be 0 */
#define SYNC_FAIL 1
#define SYNC_BAD(ms) (4<<(ms))
#define SYNC_NOGOOD 16 /* internal */
#define SYNC_CANCELED 32 /* internal */
#define SYNC_MALFORMED 64 /* internal */
#define BOX_POSSIBLE -1
#define BOX_ABSENT 0

Loading…
Cancel
Save