Hello! On Sun, Jan 19, 2014 at 12:10:55PM +0100, Filipe da Silva wrote:
> # HG changeset patch > # User Filipe da Silva <[email protected]> > # Date 1390129333 -3600 > # Sun Jan 19 12:02:13 2014 +0100 > # Node ID 3ad4498760c6fcd2ba24ae84f6d924b3a1a35a31 > # Parent bb3dc21c89efa8cfd1b9f661fcfd2f155687b99a > Mail: add IMAP ID command support (RFC2971). > > Parse the ID command and its arguments. > Handle the server response to ID command. > > diff -r bb3dc21c89ef -r 3ad4498760c6 src/mail/ngx_mail.h > --- a/src/mail/ngx_mail.h Fri Jan 17 22:06:04 2014 +0400 > +++ b/src/mail/ngx_mail.h Sun Jan 19 12:02:13 2014 +0100 > @@ -215,6 +215,7 @@ typedef struct { > unsigned quoted:1; > unsigned backslash:1; > unsigned no_sync_literal:1; > + unsigned params_list:1; > unsigned starttls:1; > unsigned esmtp:1; > unsigned auth_method:3; > @@ -233,6 +234,7 @@ typedef struct { > ngx_str_t smtp_helo; > ngx_str_t smtp_from; > ngx_str_t smtp_to; > + ngx_str_t imap_id; > > ngx_str_t cmd; > > @@ -279,10 +281,10 @@ typedef struct { > #define NGX_IMAP_CAPABILITY 3 > #define NGX_IMAP_NOOP 4 > #define NGX_IMAP_STARTTLS 5 > +#define NGX_IMAP_AUTHENTICATE 6 > +#define NGX_IMAP_ID 7 > > -#define NGX_IMAP_NEXT 6 > - > -#define NGX_IMAP_AUTHENTICATE 7 > +#define NGX_IMAP_NEXT 8 > > > #define NGX_SMTP_HELO 1 > diff -r bb3dc21c89ef -r 3ad4498760c6 src/mail/ngx_mail_imap_handler.c > --- a/src/mail/ngx_mail_imap_handler.c Fri Jan 17 22:06:04 2014 +0400 > +++ b/src/mail/ngx_mail_imap_handler.c Sun Jan 19 12:02:13 2014 +0100 > @@ -18,6 +18,8 @@ static ngx_int_t ngx_mail_imap_authentic > ngx_connection_t *c); > static ngx_int_t ngx_mail_imap_capability(ngx_mail_session_t *s, > ngx_connection_t *c); > +static ngx_int_t ngx_mail_imap_id(ngx_mail_session_t *s, > + ngx_connection_t *c); > static ngx_int_t ngx_mail_imap_starttls(ngx_mail_session_t *s, > ngx_connection_t *c); > > @@ -31,6 +33,7 @@ static u_char imap_username[] = "+ VXNl > static u_char imap_password[] = "+ UGFzc3dvcmQ6" CRLF; > static u_char imap_bye[] = "* BYE" CRLF; > static u_char imap_invalid_command[] = "BAD invalid command" CRLF; > +static u_char imap_server_id_nil[] = "* ID NIL" CRLF; > > > void > @@ -183,6 +186,10 @@ ngx_mail_imap_auth_state(ngx_event_t *re > rc = ngx_mail_imap_capability(s, c); > break; > > + case NGX_IMAP_ID: > + rc = ngx_mail_imap_id(s, c); > + break; > + > case NGX_IMAP_LOGOUT: > s->quit = 1; > ngx_str_set(&s->text, imap_bye); > @@ -438,6 +445,86 @@ ngx_mail_imap_capability(ngx_mail_sessio > > > static ngx_int_t > +ngx_mail_imap_id(ngx_mail_session_t *s, ngx_connection_t *c) > +{ > + ngx_uint_t i; > + ngx_str_t *arg, cmd; > + > + if (s->args.nelts == 0) { > + return NGX_MAIL_PARSE_INVALID_COMMAND; > + } > + > + arg = s->args.elts; > + cmd.data = s->tag.data + s->tag.len; > + cmd.len = s->arg_end - cmd.data; > + > + /* Client may send ID NIL or ID ( "key" "value" ... ) */ Comment style looks inconsistent (either don't use capitalization, or properly use trailing dots). And the comment itself is misleading as syntax provided is contradicts one from RFC. > + if (s->args.nelts == 1) { > + if (cmd.len != 6 > + || ngx_strncasecmp(cmd.data, (u_char *) "ID NIL", 6) != 0) { Style: the "{" should be on it's own line. > + ngx_log_debug1(NGX_LOG_DEBUG_MAIL, c->log, 0, > + "ID invalid argument:\"%V\"", > + &cmd); The message looks not very in-line with other debug messages, and it probably only needs two lines. > + return NGX_MAIL_PARSE_INVALID_COMMAND; > + } > + > + goto valid; > + } > + > + /* more than one and not an even item count */ > + if (s->args.nelts % 2 != 0) { > + return NGX_MAIL_PARSE_INVALID_COMMAND; > + } The comment seems to be obvious enough to be confusing. > + > + for (i = 0; i < s->args.nelts; i += 2) { > + > + switch (arg[i].len) { > + > + case 0: > + ngx_log_debug1(NGX_LOG_DEBUG_MAIL, c->log, 0, > + "ID empty key #%ui name : \"\"", i ); > + return NGX_MAIL_PARSE_INVALID_COMMAND; > + > + case 3: > + if (ngx_strncasecmp(arg[i].data, (u_char *) "NIL", 3) == 0) { > + ngx_log_debug2(NGX_LOG_DEBUG_MAIL, c->log, 0, > + "ID NIL Key #%ui name \"%V\"", i, > + &arg[i]); > + return NGX_MAIL_PARSE_INVALID_COMMAND; > + } > + break; > + > + default: > + if (arg[i].len > 30) { > + ngx_log_debug2(NGX_LOG_DEBUG_MAIL, c->log, 0, > + "ID Key #%ui name \"%V\" is too long", > + i, &arg[i]); > + return NGX_MAIL_PARSE_INVALID_COMMAND; > + } > + break; > + } > + } This code looks unneeded and incorrect. E.g., it will reject something like 't ID ("nil" "foo")'. > + > +valid: > + s->imap_id.len = cmd.len; > + s->imap_id.data = ngx_pnalloc(c->pool, cmd.len); > + if (s->imap_id.data == NULL) { > + return NGX_ERROR; > + } > + > + ngx_memcpy(s->imap_id.data, cmd.data, cmd.len); > + > + ngx_log_debug2(NGX_LOG_DEBUG_MAIL, c->log, 0, > + "imap client ID:\"%V%V\"", &s->tag, &s->imap_id); > + > + /* Prepare server response to ID command */ > + ngx_str_set(&s->text, imap_server_id_nil); See above. > + > + return NGX_OK; > +} > + > + > +static ngx_int_t > ngx_mail_imap_starttls(ngx_mail_session_t *s, ngx_connection_t *c) > { > #if (NGX_MAIL_SSL) > diff -r bb3dc21c89ef -r 3ad4498760c6 src/mail/ngx_mail_parse.c > --- a/src/mail/ngx_mail_parse.c Fri Jan 17 22:06:04 2014 +0400 > +++ b/src/mail/ngx_mail_parse.c Sun Jan 19 12:02:13 2014 +0100 > @@ -280,6 +280,17 @@ ngx_mail_imap_parse_command(ngx_mail_ses > > switch (p - c) { > > + case 2: > + if ((c[0] == 'I' || c[0] == 'i') > + && (c[1] == 'D'|| c[1] == 'd')) > + { > + s->command = NGX_IMAP_ID; > + > + } else { > + goto invalid; > + } > + break; > + > case 4: > if ((c[0] == 'N' || c[0] == 'n') > && (c[1] == 'O'|| c[1] == 'o') > @@ -409,14 +420,32 @@ ngx_mail_imap_parse_command(ngx_mail_ses > case ' ': > break; > case CR: > + if (s->params_list) > + goto invalid; I believe I already wrote about ifs without curly brackets in previous review. > state = sw_almost_done; > s->arg_end = p; > break; > case LF: > + if (s->params_list) > + goto invalid; > s->arg_end = p; > goto done; > + case '(': // params list begin > + if (!s->params_list && s->args.nelts == 0) { > + s->params_list = 1; > + break; > + } > + goto invalid; As well as about C99-style comments. > + case ')': // params list closing > + if (s->params_list && s->args.nelts > 0) { > + s->params_list = 0; > + state = sw_spaces_before_argument; > + break; > + } > + goto invalid; And about empty parameters lists allowed by RFC 2971. It may be a good idea to pay a bit more attention to comments already got, as it's quickly becomes boring to repeat the same comments again. [... rest of the patch skipped without actual review ...] -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
