[PATCH 1 of 3] Mail: add IMAP ID command support (RFC2971)
Maxim Dounin
mdounin at mdounin.ru
Wed Jan 22 18:10:58 UTC 2014
Hello!
On Sun, Jan 19, 2014 at 12:10:55PM +0100, Filipe da Silva wrote:
> # HG changeset patch
> # User Filipe da Silva <fdasilvayy at gmail.com>
> # 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/
More information about the nginx-devel
mailing list