[PATCH 1 of 7] Mail: add IMAP ID command support

[Filipe Da Silva]

Hi !

Many thanks for the reviews.
I will rework on my patch quickly.

I've just one question.

2014/1/17  <nginx-devel-request at nginx.org>:
> Message: 1
> Date: Fri, 17 Jan 2014 07:54:27 +0400
> From: Maxim Dounin <mdounin at mdounin.ru>
> To: nginx-devel at nginx.org
> Subject: Re: [PATCH 1 of 7] Mail: add IMAP ID command support
> Message-ID: <20140117035427.GM1835 at mdounin.ru>
> Content-Type: text/plain; charset=us-ascii
>
> Hello!
>
> On Tue, Jan 14, 2014 at 12:54:18PM +0100, Filipe da Silva wrote:
>
>> # HG changeset patch

>
> Please use
>
> [diff]
> showfunc=1
>
> in your hgrc.  Having function names in diffs greatly simplifies
> reading patches.
>

As I read it, i remember seeing it in some previous threads.
You better add this point to http://nginx.org/en/docs/contributing_changes.html


>> +
>> +        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.
>

I think I better add
@@ -249,6 +249,8 @@ typedef struct {
     u_char                 *cmd_start;
     u_char                 *arg_start;
     u_char                 *arg_end;
+    u_char                 *list_start;
+    u_char                 *list_end;
     ngx_uint_t              literal_len;
 } ngx_mail_session_t;

And use this point pointers to extract the whole parameter list at once .

But I'm worried about the fact that a very long client command could
be split/sparse in separated buffers ?
Am I wrong ? ' cause I haven't dig enough into nginx internals, to
answer to this point.


>
>> +            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.
>

I'm agree, I have to extend a bit more the IMAP parsing,
as I made some tests on gmail, and these commands are valid :
dd ID ( KEY VALUE )
dd ID ( KEY NIL )
dd ID ("KEY" NIL )
dd ID ("KEY" NIL)

but these are'nt :
dd ID ( NIL NIL )
dd ID ( NIL NIL)
tag ID ( KEY VALUE NIL )
neither :
ddd ID ( )
-> ddd BAD Invalid argument supplied to ID. zzzzzzzzzzzzzz.25


>>  }
>
>
> --
> Maxim Dounin
> http://nginx.org/
>

---
Filipe da Silva