Hi ! Many thanks for the reviews. I will rework on my patch quickly.
I've just one question. 2014/1/17 <[email protected]>: > Message: 1 > Date: Fri, 17 Jan 2014 07:54:27 +0400 > From: Maxim Dounin <[email protected]> > To: [email protected] > Subject: Re: [PATCH 1 of 7] Mail: add IMAP ID command support > Message-ID: <[email protected]> > 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 _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
