From 4f383a8074b33c31e5e941f446419f5e5a708a20 Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Sat, 4 Oct 2014 17:07:17 +0200 Subject: [PATCH] stop abusing memcmp() memcmp() is unfortunately not guaranteed to read forward byte-by-byte, which means that the clever use as a strncmp() without the pointless strlen()s is not permitted, and can actually misbehave with SSE-optimized string functions. so implement proper equals() and starts_with() functions. as a bonus, the calls are less cryptic. --- configure.ac | 2 +- src/common.h | 3 +++ src/compat/config.c | 12 ++++++------ src/compat/isync.h | 3 +++ src/compat/util.c | 29 +++++++++++++++++++++++++++++ src/drv_imap.c | 34 ++++++++++++++++++---------------- src/drv_maildir.c | 19 ++++++++++--------- src/main.c | 16 ++++++++-------- src/sync.c | 4 ++-- src/util.c | 29 +++++++++++++++++++++++++++++ 10 files changed, 109 insertions(+), 42 deletions(-) diff --git a/configure.ac b/configure.ac index 5bccacf..f6d5d64 100644 --- a/configure.ac +++ b/configure.ac @@ -29,7 +29,7 @@ if test "x$ob_cv_strftime_z" = x"no"; then fi AC_CHECK_HEADERS(sys/poll.h sys/select.h) -AC_CHECK_FUNCS(vasprintf memrchr timegm) +AC_CHECK_FUNCS(vasprintf strnlen memrchr timegm) AC_CHECK_LIB(socket, socket, [SOCK_LIBS="-lsocket"]) AC_CHECK_LIB(nsl, inet_ntoa, [SOCK_LIBS="$SOCK_LIBS -lnsl"]) diff --git a/src/common.h b/src/common.h index fb868b3..e90f0ea 100644 --- a/src/common.h +++ b/src/common.h @@ -94,6 +94,9 @@ void free_string_list( string_list_t *list ); void *memrchr( const void *s, int c, size_t n ); #endif +int starts_with( const char *str, int strl, const char *cmp, int cmpl ); +int equals( const char *str, int strl, const char *cmp, int cmpl ); + #ifndef HAVE_TIMEGM # include time_t timegm( struct tm *tm ); diff --git a/src/compat/config.c b/src/compat/config.c index 70360fc..746b46a 100644 --- a/src/compat/config.c +++ b/src/compat/config.c @@ -144,7 +144,7 @@ load_config( const char *path, config_t ***stor ) goto forbid; inbox = nfstrdup( val ); } else if (!strcasecmp( "Host", cmd )) { - if (!memcmp( "imaps:", val, 6 )) { + if (starts_with( val, -1, "imaps:", 6 )) { val += 6; cfg->use_imaps = 1; cfg->port = 993; @@ -268,7 +268,7 @@ write_imap_server( FILE *fp, config_t *cfg ) } if (boxes) /* !o2o */ for (pbox = boxes; pbox != cfg; pbox = pbox->next) - if (!memcmp( pbox->old_server_name, buf, hl + 1 )) { + if (equals( pbox->old_server_name, -1, buf, hl )) { nfasprintf( (char **)&cfg->old_server_name, "%s-%d", buf, ++pbox->old_servers ); goto gotsrv; } @@ -293,7 +293,7 @@ write_imap_server( FILE *fp, config_t *cfg ) } if (boxes) /* !o2o */ for (pbox = boxes; pbox != cfg; pbox = pbox->next) - if (!memcmp( pbox->server_name, buf, hl + 1 )) { + if (equals( pbox->server_name, -1, buf, hl )) { nfasprintf( (char **)&cfg->server_name, "%s-%d", buf, ++pbox->servers ); goto ngotsrv; } @@ -444,13 +444,13 @@ write_config( int fd ) gotall: path = expand_strdup( box->path ); - if (!memcmp( path, Home, HomeLen ) && path[HomeLen] == '/') + if (starts_with( path, -1, Home, HomeLen ) && path[HomeLen] == '/') nfasprintf( &path, "~%s", path + HomeLen ); local_store = local_box = strrchr( path, '/' ) + 1; pl = local_store - path; /* try to re-use existing store */ for (pbox = boxes; pbox != box; pbox = pbox->next) - if (pbox->local_store_path && !memcmp( pbox->local_store_path, path, pl ) && !pbox->local_store_path[pl]) + if (pbox->local_store_path && equals( pbox->local_store_path, -1, path, pl )) goto gotstor; box->local_store_path = my_strndup( path, pl ); /* derive a suitable name */ @@ -535,7 +535,7 @@ find_box( const char *s ) config_t *p; char *t; - if (!memcmp( s, Home, HomeLen ) && s[HomeLen] == '/') + if (starts_with( s, -1, Home, HomeLen ) && s[HomeLen] == '/') s += HomeLen + 1; for (p = boxes; p; p = p->next) { if (!strcmp( s, p->path ) || (p->alias && !strcmp( s, p->alias ))) diff --git a/src/compat/isync.h b/src/compat/isync.h index b8c7cc2..0473900 100644 --- a/src/compat/isync.h +++ b/src/compat/isync.h @@ -106,3 +106,6 @@ void ATTR_NORETURN oob( void ); #ifndef HAVE_MEMRCHR void *memrchr( const void *s, int c, size_t n ); #endif + +int starts_with( const char *str, int strl, const char *cmp, int cmpl ); +int equals( const char *str, int strl, const char *cmp, int cmpl ); diff --git a/src/compat/util.c b/src/compat/util.c index f7389a2..518a5a7 100644 --- a/src/compat/util.c +++ b/src/compat/util.c @@ -100,6 +100,35 @@ memrchr( const void *s, int c, size_t n ) } #endif +#ifndef HAVE_STRNLEN +int +strnlen( const char *str, size_t maxlen ) +{ + size_t len; + + /* It's tempting to use memchr(), but it's allowed to read past the end of the actual string. */ + for (len = 0; len < maxlen && str[len]; len++) {} + return len; +} + +#endif + +int +starts_with( const char *str, int strl, const char *cmp, int cmpl ) +{ + if (strl < 0) + strl = strnlen( str, cmpl + 1 ); + return (strl >= cmpl) && !memcmp( str, cmp, cmpl ); +} + +int +equals( const char *str, int strl, const char *cmp, int cmpl ) +{ + if (strl < 0) + strl = strnlen( str, cmpl + 1 ); + return (strl == cmpl) && !memcmp( str, cmp, cmpl ); +} + void oob( void ) { diff --git a/src/drv_imap.c b/src/drv_imap.c index 70118e2..32b6708 100644 --- a/src/drv_imap.c +++ b/src/drv_imap.c @@ -257,7 +257,7 @@ send_imap_cmd( imap_store_t *ctx, struct imap_cmd *cmd ) if (DFlags & VERBOSE) { if (ctx->num_in_progress) printf( "(%d in progress) ", ctx->num_in_progress ); - if (memcmp( cmd->cmd, "LOGIN", 5 )) + if (!starts_with( cmd->cmd, -1, "LOGIN", 5 )) printf( "%s>>> %s", ctx->label, buf ); else printf( "%s>>> %d LOGIN \n", ctx->label, cmd->tag ); @@ -732,7 +732,7 @@ parse_imap_list( imap_store_t *ctx, char **sp, parse_list_state_t *sts ) if (sts->level && *s == ')') break; cur->len = s - p; - if (cur->len == 3 && !memcmp ("NIL", p, 3)) + if (equals( p, cur->len, "NIL", 3 )) cur->val = NIL; else { cur->val = nfmalloc( cur->len + 1 ); @@ -926,7 +926,7 @@ parse_fetch_rsp( imap_store_t *ctx, list_t *list, char *s ATTR_UNUSED ) tmp = tmp->next; if (!is_atom( tmp )) goto bfail; - if (!memcmp( tmp->val, "X-TUID: ", 8 )) + if (starts_with( tmp->val, tmp->len, "X-TUID: ", 8 )) tuid = tmp->val + 8; } else { bfail: @@ -1063,12 +1063,12 @@ parse_list_rsp( imap_store_t *ctx, list_t *list, char *cmd ) } static int -is_inbox( imap_store_t *ctx, const char *arg ) +is_inbox( imap_store_t *ctx, const char *arg, int argl ) { int i; char c; - if (memcmp( arg, "INBOX", 5 )) + if (!starts_with( arg, argl, "INBOX", 5 )) return 0; if (arg[5]) for (i = 0; (c = ctx->delimiter[i]); i++) @@ -1082,7 +1082,7 @@ parse_list_rsp_p2( imap_store_t *ctx, list_t *list, char *cmd ATTR_UNUSED ) { string_list_t *narg; char *arg; - int l; + int argl, l; if (!is_atom( list )) { error( "IMAP error: malformed LIST response\n" ); @@ -1090,17 +1090,19 @@ parse_list_rsp_p2( imap_store_t *ctx, list_t *list, char *cmd ATTR_UNUSED ) return LIST_BAD; } arg = list->val; - if (!is_inbox( ctx, arg ) && (l = strlen( ctx->prefix ))) { - if (memcmp( arg, ctx->prefix, l )) + argl = list->len; + if (!is_inbox( ctx, arg, argl ) && (l = strlen( ctx->prefix ))) { + if (!starts_with( arg, argl, ctx->prefix, l )) goto skip; arg += l; - if (is_inbox( ctx, arg )) { + argl -= l; + if (is_inbox( ctx, arg, argl )) { if (!arg[5]) warn( "IMAP warning: ignoring INBOX in %s\n", ctx->prefix ); goto skip; } } - if ((l = strlen( arg )) >= 5 && !memcmp( arg + l - 5, ".lock", 5 )) /* workaround broken servers */ + if (argl >= 5 && !memcmp( arg + argl - 5, ".lock", 5 )) /* workaround broken servers */ goto skip; if (map_name( arg, (char **)&narg, offsetof(string_list_t, string), ctx->delimiter, "/") < 0) { warn( "IMAP warning: ignoring mailbox %s (reserved character '/' in name)\n", arg ); @@ -1137,7 +1139,7 @@ prepare_box( char **buf, const imap_store_t *ctx ) const char *name = ctx->gen.name; return prepare_name( buf, ctx, - (!memcmp( name, "INBOX", 5 ) && (!name[5] || name[5] == '/')) ? "" : ctx->prefix, name ); + (starts_with( name, -1, "INBOX", 5 ) && (!name[5] || name[5] == '/')) ? "" : ctx->prefix, name ); } static int @@ -1276,7 +1278,7 @@ imap_socket_read( void *aux ) if (!strcmp( "NO", arg )) { if (cmdp->param.create && (cmdp->param.trycreate || - (cmd && !memcmp( cmd, "[TRYCREATE]", 11 )))) + (cmd && starts_with( cmd, -1, "[TRYCREATE]", 11 )))) { /* SELECT, APPEND or UID COPY */ struct imap_cmd_trycreate *cmd2 = (struct imap_cmd_trycreate *)new_imap_cmd( sizeof(*cmd2) ); @@ -1292,7 +1294,7 @@ imap_socket_read( void *aux ) } else /*if (!strcmp( "BAD", arg ))*/ resp = RESP_CANCEL; error( "IMAP command '%s' returned an error: %s %s\n", - memcmp( cmdp->cmd, "LOGIN", 5 ) ? cmdp->cmd : "LOGIN ", + !starts_with( cmdp->cmd, -1, "LOGIN", 5 ) ? cmdp->cmd : "LOGIN ", arg, cmd ? cmd : "" ); } if ((resp2 = parse_response_code( ctx, cmdp, cmd )) > resp) @@ -2266,7 +2268,7 @@ imap_parse_store( conffile_t *cfg, store_conf_t **storep ) if (!strcasecmp( "Host", cfg->cmd )) { /* The imap[s]: syntax is just a backwards compat hack. */ #ifdef HAVE_LIBSSL - if (!memcmp( "imaps:", cfg->val, 6 )) { + if (starts_with( cfg->val, -1, "imaps:", 6 )) { cfg->val += 6; server->sconf.use_imaps = 1; server->sconf.use_sslv2 = 1; @@ -2274,10 +2276,10 @@ imap_parse_store( conffile_t *cfg, store_conf_t **storep ) } else #endif { - if (!memcmp( "imap:", cfg->val, 5 )) + if (starts_with( cfg->val, -1, "imap:", 5 )) cfg->val += 5; } - if (!memcmp( "//", cfg->val, 2 )) + if (starts_with( cfg->val, -1, "//", 2 )) cfg->val += 2; server->sconf.host = nfstrdup( cfg->val ); } diff --git a/src/drv_maildir.c b/src/drv_maildir.c index 6f5a7ea..2b30c90 100644 --- a/src/drv_maildir.c +++ b/src/drv_maildir.c @@ -208,7 +208,7 @@ maildir_invoke_bad_callback( store_t *ctx ) static int maildir_list_inbox( store_t *gctx, int *flags ); static int -maildir_list_recurse( store_t *gctx, int isBox, int *flags, const char *inbox, +maildir_list_recurse( store_t *gctx, int isBox, int *flags, const char *inbox, int inboxLen, char *path, int pathLen, char *name, int nameLen ) { DIR *dir; @@ -234,7 +234,7 @@ maildir_list_recurse( store_t *gctx, int isBox, int *flags, const char *inbox, while ((de = readdir( dir ))) { const char *ent = de->d_name; pl = pathLen + nfsnprintf( path + pathLen, _POSIX_PATH_MAX - pathLen, "%s", ent ); - if (inbox && !memcmp( path, inbox, pl ) && !inbox[pl]) { + if (inbox && equals( path, pl, inbox, inboxLen )) { if (maildir_list_inbox( gctx, flags ) < 0) { closedir( dir ); return -1; @@ -249,14 +249,14 @@ maildir_list_recurse( store_t *gctx, int isBox, int *flags, const char *inbox, } else { if (isBox) continue; - if (!nameLen && !memcmp( ent, "INBOX", 6 )) { + if (!nameLen && equals( ent, -1, "INBOX", 5 )) { path[pathLen] = 0; warn( "Maildir warning: ignoring INBOX in %s\n", path ); continue; } } nl = nameLen + nfsnprintf( name + nameLen, _POSIX_PATH_MAX - nameLen, "%s", ent ); - if (maildir_list_recurse( gctx, 1, flags, inbox, path, pl, name, nl ) < 0) { + if (maildir_list_recurse( gctx, 1, flags, inbox, inboxLen, path, pl, name, nl ) < 0) { closedir( dir ); return -1; } @@ -273,7 +273,7 @@ maildir_list_inbox( store_t *gctx, int *flags ) *flags &= ~LIST_INBOX; return maildir_list_recurse( - gctx, 2, flags, 0, + gctx, 2, flags, 0, 0, path, nfsnprintf( path, _POSIX_PATH_MAX, "%s", ((maildir_store_conf_t *)gctx->conf)->inbox ), name, nfsnprintf( name, _POSIX_PATH_MAX, "INBOX" ) ); } @@ -281,12 +281,13 @@ maildir_list_inbox( store_t *gctx, int *flags ) static int maildir_list_path( store_t *gctx, int *flags ) { + const char *inbox = ((maildir_store_conf_t *)gctx->conf)->inbox; char path[_POSIX_PATH_MAX], name[_POSIX_PATH_MAX]; if (maildir_validate_path( gctx->conf ) < 0) return -1; return maildir_list_recurse( - gctx, 0, flags, ((maildir_store_conf_t *)gctx->conf)->inbox, + gctx, 0, flags, inbox, strlen( inbox ), path, nfsnprintf( path, _POSIX_PATH_MAX, "%s", gctx->conf->path ), name, 0 ); } @@ -767,7 +768,7 @@ maildir_scan( maildir_store_t *ctx, msglist_t *msglist ) ctx->db->err( ctx->db, ret, "Maildir error: db->c_get()" ); break; } - if ((key.size != 11 || memcmp( key.data, "UIDVALIDITY", 11 )) && + if (!equals( key.data, key.size, "UIDVALIDITY", 11 ) && (ret = tdb->get( tdb, 0, &key, &value, 0 ))) { if (ret != DB_NOTFOUND) { tdb->err( tdb, ret, "Maildir error: tdb->get()" ); @@ -876,7 +877,7 @@ maildir_scan( maildir_store_t *ctx, msglist_t *msglist ) while (fgets( nbuf, sizeof(nbuf), f )) { if (!nbuf[0] || nbuf[0] == '\n') break; - if (!memcmp( nbuf, "X-TUID: ", 8 ) && nbuf[8 + TUIDL] == '\n') { + if (starts_with( nbuf, -1, "X-TUID: ", 8 ) && nbuf[8 + TUIDL] == '\n') { memcpy( entry->tuid, nbuf + 8, TUIDL ); break; } @@ -940,7 +941,7 @@ maildir_select( store_t *gctx, int create, #ifdef USE_DB ctx->db = 0; #endif /* USE_DB */ - if (!memcmp( gctx->name, "INBOX", 5 ) && (!gctx->name[5] || gctx->name[5] == '/')) { + if (starts_with( gctx->name, -1, "INBOX", 5 ) && (!gctx->name[5] || gctx->name[5] == '/')) { gctx->path = maildir_join_path( ((maildir_store_conf_t *)gctx->conf)->inbox, gctx->name + 5 ); } else { if (maildir_validate_path( gctx->conf ) < 0) { diff --git a/src/main.c b/src/main.c index 8c3afda..3efd179 100644 --- a/src/main.c +++ b/src/main.c @@ -155,7 +155,7 @@ filter_boxes( string_list_t *boxes, const char *prefix, string_list_t *patterns pfxl = prefix ? strlen( prefix ) : 0; for (; boxes; boxes = boxes->next) { - if (memcmp( boxes->string, prefix, pfxl )) + if (!starts_with( boxes->string, -1, prefix, pfxl )) continue; fnot = 1; for (cpat = patterns; cpat; cpat = cpat->next) { @@ -256,7 +256,7 @@ main( int argc, char **argv ) return 1; } config = argv[mvars->oind++]; - } else if (!memcmp( opt, "config=", 7 )) + } else if (starts_with( opt, -1, "config=", 7 )) config = opt + 7; else if (!strcmp( opt, "all" )) mvars->all = 1; @@ -282,7 +282,7 @@ main( int argc, char **argv ) cops |= XOP_PULL, mvars->ops[M] |= XOP_HAVE_TYPE; else if (!strcmp( opt, "push" )) cops |= XOP_PUSH, mvars->ops[M] |= XOP_HAVE_TYPE; - else if (!memcmp( opt, "create", 6 )) { + else if (starts_with( opt, -1, "create", 6 )) { opt += 6; op = OP_CREATE|XOP_HAVE_CREATE; lcop: @@ -295,7 +295,7 @@ main( int argc, char **argv ) else goto badopt; mvars->ops[M] |= op & (XOP_HAVE_CREATE|XOP_HAVE_EXPUNGE); - } else if (!memcmp( opt, "expunge", 7 )) { + } else if (starts_with( opt, -1, "expunge", 7 )) { opt += 7; op = OP_EXPUNGE|XOP_HAVE_EXPUNGE; goto lcop; @@ -307,7 +307,7 @@ main( int argc, char **argv ) mvars->ops[M] |= XOP_HAVE_TYPE|XOP_PULL|XOP_PUSH; else if (!strcmp( opt, "noop" )) mvars->ops[M] |= XOP_HAVE_TYPE; - else if (!memcmp( opt, "pull", 4 )) { + else if (starts_with( opt, -1, "pull", 4 )) { op = XOP_PULL; lcac: opt += 4; @@ -318,7 +318,7 @@ main( int argc, char **argv ) goto rlcac; } else goto badopt; - } else if (!memcmp( opt, "push", 4 )) { + } else if (starts_with( opt, -1, "push", 4 )) { op = XOP_PUSH; goto lcac; } else { @@ -719,10 +719,10 @@ store_opened( store_t *ctx, void *aux ) const char *pat = cpat->string; if (*pat != '!') { char buf[8]; - snprintf( buf, sizeof(buf), "%s%s", mvars->chan->boxes[t], pat ); + int bufl = snprintf( buf, sizeof(buf), "%s%s", mvars->chan->boxes[t], pat ); /* Partial matches like "INB*" or even "*" are not considered, * except implicity when the INBOX lives under Path. */ - if (!memcmp( buf, "INBOX", 5 )) { + if (starts_with( buf, bufl, "INBOX", 5 )) { char c = buf[5]; if (!c) { /* User really wants the INBOX. */ diff --git a/src/sync.c b/src/sync.c index 51c64a9..35eaf38 100644 --- a/src/sync.c +++ b/src/sync.c @@ -315,7 +315,7 @@ msg_fetched( int sts, void *aux ) if (c == '\r') lcrs++; else if (c == '\n') { - if (!memcmp( fmap + start, "X-TUID: ", 8 )) { + if (starts_with( fmap + start, len - start, "X-TUID: ", 8 )) { extra = (sbreak = start) - (ebreak = i); goto oke; } @@ -803,7 +803,7 @@ box_selected( int sts, void *aux ) error( "Error: incomplete journal header in %s\n", svars->jname ); goto jbail; } - if (memcmp( buf, JOURNAL_VERSION "\n", strlen(JOURNAL_VERSION) + 1 )) { + if (!equals( buf, t, JOURNAL_VERSION "\n", strlen(JOURNAL_VERSION) + 1 )) { error( "Error: incompatible journal version " "(got %.*s, expected " JOURNAL_VERSION ")\n", t - 1, buf ); goto jbail; diff --git a/src/util.c b/src/util.c index c76916b..2c017c8 100644 --- a/src/util.c +++ b/src/util.c @@ -203,6 +203,35 @@ memrchr( const void *s, int c, size_t n ) } #endif +#ifndef HAVE_STRNLEN +int +strnlen( const char *str, size_t maxlen ) +{ + size_t len; + + /* It's tempting to use memchr(), but it's allowed to read past the end of the actual string. */ + for (len = 0; len < maxlen && str[len]; len++) {} + return len; +} + +#endif + +int +starts_with( const char *str, int strl, const char *cmp, int cmpl ) +{ + if (strl < 0) + strl = strnlen( str, cmpl + 1 ); + return (strl >= cmpl) && !memcmp( str, cmp, cmpl ); +} + +int +equals( const char *str, int strl, const char *cmp, int cmpl ) +{ + if (strl < 0) + strl = strnlen( str, cmpl + 1 ); + return (strl == cmpl) && !memcmp( str, cmp, cmpl ); +} + #ifndef HAVE_TIMEGM /* Converts struct tm to time_t, assuming the data in tm is UTC rather