[PATCH 1 of 7] Mail: add IMAP ID command support
Maxim Dounin
mdounin at mdounin.ru
Fri Jan 17 03:54:27 UTC 2014
Hello!
On Tue, Jan 14, 2014 at 12:54:18PM +0100, Filipe da Silva wrote:
> # HG changeset patch
> # User Filipe da Silva <fdasilvayy at gmail.com>
> # Date 1389700210 -3600
> # Tue Jan 14 12:50:10 2014 +0100
> # Node ID 0ff28c3c519125db11ae3c56fbf34a7a5975a452
> # Parent d049b0ea00a388c142627f10a0ee01c5b1bedc43
> Mail: add IMAP ID command support.
> add parsing of IMAP ID command and his parameter list, see RFC2971
Summary line.
<empty line>
Detailed description, if needed, with proper capitalization and
punctuation, please.
>
> diff -r d049b0ea00a3 -r 0ff28c3c5191 src/mail/ngx_mail.h
> --- a/src/mail/ngx_mail.h Fri Jan 10 16:12:40 2014 +0100
> +++ b/src/mail/ngx_mail.h Tue Jan 14 12:50:10 2014 +0100
> @@ -215,6 +215,7 @@
> 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 @@
> ngx_str_t smtp_helo;
> ngx_str_t smtp_from;
> ngx_str_t smtp_to;
> + ngx_str_t imap_client_id;
Shouldn't it be "imap_id" instead?
>
> ngx_str_t cmd;
>
> @@ -284,6 +286,7 @@
>
> #define NGX_IMAP_AUTHENTICATE 7
>
> +#define NGX_IMAP_ID 8
>
Probably both NGX_IMAP_AUTHENTICATE and NGX_IMAP_ID should be
moved to other IMAP commands, with only NGX_IMAP_NEXT preserved
distinct - as it's a special case of fake command to handle
literals.
> #define NGX_SMTP_HELO 1
> #define NGX_SMTP_EHLO 2
> diff -r d049b0ea00a3 -r 0ff28c3c5191 src/mail/ngx_mail_imap_handler.c
> --- a/src/mail/ngx_mail_imap_handler.c Fri Jan 10 16:12:40 2014 +0100
> +++ b/src/mail/ngx_mail_imap_handler.c Tue Jan 14 12:50:10 2014 +0100
> @@ -16,6 +16,8 @@
> ngx_connection_t *c);
> static ngx_int_t ngx_mail_imap_authenticate(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_capability(ngx_mail_session_t *s,
> ngx_connection_t *c);
> static ngx_int_t ngx_mail_imap_starttls(ngx_mail_session_t *s,
> @@ -32,6 +34,9 @@
> static u_char imap_bye[] = "* BYE" CRLF;
> static u_char imap_invalid_command[] = "BAD invalid command" CRLF;
>
> +static ngx_str_t ngx_mail_imap_client_id_nil = ngx_string("ID NIL");
> +static ngx_str_t ngx_mail_imap_server_id_nil = ngx_string("* ID NIL" CRLF);
> +
>
Using strings in a same way as the other code here may be a good
idea.
> void
> ngx_mail_imap_init_session(ngx_mail_session_t *s, ngx_connection_t *c)
> @@ -179,6 +184,10 @@
Please use
[diff]
showfunc=1
in your hgrc. Having function names in diffs greatly simplifies
reading patches.
> tag = (rc != NGX_OK);
> break;
>
> + case NGX_IMAP_ID:
> + rc = ngx_mail_imap_id(s, c);
> + break;
> +
> case NGX_IMAP_CAPABILITY:
> rc = ngx_mail_imap_capability(s, c);
> break;
I would rather recommend moving this after the CAPABILITY command
handling.
> @@ -292,6 +301,60 @@
> ngx_mail_send(c->write);
> }
>
> +static ngx_int_t
Two empty lines between functions, please.
http://nginx.org/en/docs/contributing_changes.html
> +ngx_mail_imap_id(ngx_mail_session_t *s, ngx_connection_t *c)
> +{
This is certainly a wrong place to put this function. Both
prototype and switch suggests it should be after
ngx_mail_imap_authenticate(), but for some reason you've placed
the function body in an arbitrary place.
> + ngx_str_t *arg;
> + size_t size, i;
> + u_char *p, *data;
Proper order would be:
u_char *p, *data;
size_t size, i;
ngx_str_t *arg;
> +
> + arg = s->args.elts;
> + if (s->args.nelts < 1 || arg[0].len == 0) {
> + return NGX_MAIL_PARSE_INVALID_COMMAND;
> + }
The arg[0].len == 0 check seems unneeded.
> +
> + // Client sends ID NIL or ID ( ... )
No C99 comments, please.
> + if (s->args.nelts == 1) {
> +
> + if (ngx_strncasecmp(arg[0].data, (u_char *) "NIL", 3) != 0)
> + return NGX_MAIL_PARSE_INVALID_COMMAND;
Never ever use if() without curly brackets, please. It's not
only a style issue but also may easily result in wrong code as
there are macros which aren't single statements.
> +
> + s->imap_client_id = ngx_mail_imap_client_id_nil;
> +
> + } else {
> + size = sizeof("ID (") - 1;
> + for (i = 0; i < s->args.nelts; i++) {
> + size += 1 + arg[i].len + 2; // 1 space plus 2 quotes
> + }
> +
> + data = ngx_pnalloc(c->pool, size);
> + if (data == NULL) {
> + return NGX_ERROR;
> + }
> +
> + p = ngx_cpymem(data, "ID (", sizeof("ID (") - 1);
> + for (i = 0; i < s->args.nelts; i++) {
> + *p++ = '"';
> + p = ngx_cpymem(p, arg[i].data, arg[i].len);
> + *p++ = '"';
> + *p++ = ' ';
> + }
> + *--p = ')'; // replace last space
> +
> + s->imap_client_id.len = size;
> + s->imap_client_id.data = data;
This doesn't look correct. E.g., arguments may contain quotes.
> + }
> +
> + ngx_log_debug2(NGX_LOG_DEBUG_MAIL, c->log, 0,
> + "imap client ID:\"%V%V\"",
> + &s->tag, &s->imap_client_id);
> +
> + // Prepare server response to ID command
> + s->text = ngx_mail_imap_server_id_nil;
> +
> + return NGX_OK;
> +}
> +
>
> static ngx_int_t
> ngx_mail_imap_login(ngx_mail_session_t *s, ngx_connection_t *c)
> diff -r d049b0ea00a3 -r 0ff28c3c5191 src/mail/ngx_mail_parse.c
> --- a/src/mail/ngx_mail_parse.c Fri Jan 10 16:12:40 2014 +0100
> +++ b/src/mail/ngx_mail_parse.c Tue Jan 14 12:50:10 2014 +0100
> @@ -279,6 +279,16 @@
> c = s->cmd_start;
>
> 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')
The empty line between "switch" and "case" was here intentionally.
> @@ -409,14 +419,31 @@
> case ' ':
> break;
> case CR:
> + if (s->params_list == 1)
> + goto invalid;
> state = sw_almost_done;
> s->arg_end = p;
> break;
> case LF:
> + if (s->params_list == 1)
> + 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;
It's good idea to use one form of tests, either "s->params_list /
!s->params_list", or "s->params_list == 0 / s->params_list == 1".
> + case ')': // params list closing
> + if (s->params_list == 1 && s->args.nelts > 0) {
The ID command allows an empty list as per it's formal syntax.
> + s->params_list = 0;
> + state = sw_spaces_before_argument;
> + break;
> + }
> + goto invalid;
> case '"':
> - if (s->args.nelts <= 2) {
> + if (s->args.nelts <= 2 || s->params_list) {
Completely unlimited number of arguments looks wrong, especially
considering "Implementations MUST NOT send more than 30
field-value pairs" clause in RFC 2971.
> s->quoted = 1;
> s->arg_start = p + 1;
> state = sw_argument;
> @@ -430,7 +457,7 @@
> }
> goto invalid;
> default:
> - if (s->args.nelts <= 2) {
> + if (s->args.nelts <= 2 && !s->params_list) {
This effectively forbids atoms within lists, and also forbids NIL
as there is no special handling. The NIL is perfectly allowed
even in ID command parameters list, not even talking about lists
in general.
> s->arg_start = p;
> state = sw_argument;
> break;
> @@ -602,6 +629,7 @@
> s->quoted = 0;
> s->no_sync_literal = 0;
> s->literal_len = 0;
> + s->params_list = 0;
> }
>
> s->state = (s->command != NGX_IMAP_AUTHENTICATE) ? sw_start : sw_argument;
> @@ -614,6 +642,7 @@
> s->quoted = 0;
> s->no_sync_literal = 0;
> s->literal_len = 0;
> + s->params_list = 0;
>
> return NGX_MAIL_PARSE_INVALID_COMMAND;
> }
--
Maxim Dounin
http://nginx.org/
More information about the nginx-devel
mailing list