Hello! On Fri, Sep 20, 2013 at 10:48:12PM +0300, Markus Linnala wrote:
> # HG changeset patch > # User Markus Linnala <[email protected]> > # Date 1379691757 -10800 > # Fri Sep 20 18:42:37 2013 +0300 > # Node ID 79cea900573997a74400dcef925de41ec6c150e7 > # Parent 4e7279d4c9c4181683373df3947749a7727b89a4 > Mail: fix STARTTLS misalloc Trailing dot, please. > Found by mail_imap.t from mdounin nginx-tests when running under valgrind. > > ==10647== Invalid write of size 1 > ==10647== at 0x4B1493: ngx_mail_smtp_merge_srv_conf > (ngx_mail_smtp_module.c:280) > ==10647== by 0x4AB363: ngx_mail_block (ngx_mail.c:209) > ==10647== by 0x4303BE: ngx_conf_parse (ngx_conf_file.c:391) > ==10647== by 0x42DF03: ngx_init_cycle (ngx_cycle.c:265) > ==10647== by 0x4206A9: main (nginx.c:333) > ==10647== Address 0x550fb84 is 0 bytes after a block of size 68 alloc'd > ==10647== at 0x4A074CD: malloc (vg_replace_malloc.c:236) > ==10647== by 0x43B251: ngx_alloc (ngx_alloc.c:22) > ==10647== by 0x421B0D: ngx_malloc (ngx_palloc.c:119) > ==10647== by 0x421B65: ngx_pnalloc (ngx_palloc.c:147) > ==10647== by 0x4B1447: ngx_mail_smtp_merge_srv_conf > (ngx_mail_smtp_module.c:269) > ==10647== by 0x4AB363: ngx_mail_block (ngx_mail.c:209) > ==10647== by 0x4303BE: ngx_conf_parse (ngx_conf_file.c:391) > ==10647== by 0x42DF03: ngx_init_cycle (ngx_cycle.c:265) > ==10647== by 0x4206A9: main (nginx.c:333) > ==10647== > > I choose to retain extra CRLF as I could not test protocol change easily. > As per RFC 2487 there is no extra CRLF. But it was not obvious why it was > there from history. As I already said in the ticket, you patch looks wrong for me. It doesn't retain extra CRLF but rather adds one to the output - previously, size (and hence conf->starttls_capability.len) was correct, and the problem was unneeded overrun of unallocated memory. With your patch, size becomes wrong - and the output changes. > > This fixes ticket #411 Just a "... (ticket #411)." in a summary line, please. > > diff -r 4e7279d4c9c4 -r 79cea9005739 src/mail/ngx_mail_smtp_module.c > --- a/src/mail/ngx_mail_smtp_module.c Fri Sep 20 17:57:21 2013 +0300 > +++ b/src/mail/ngx_mail_smtp_module.c Fri Sep 20 18:42:37 2013 +0300 > @@ -264,7 +264,7 @@ > last[3] = ' '; > } > > - size += sizeof("250 STARTTLS" CRLF) - 1; > + size += sizeof("250 STARTTLS" CRLF CRLF) - 1; > > p = ngx_pnalloc(cf->pool, size); > if (p == NULL) { > @@ -276,8 +276,7 @@ > > p = ngx_cpymem(p, conf->capability.data, conf->capability.len); > > - p = ngx_cpymem(p, "250 STARTTLS" CRLF, sizeof("250 STARTTLS" CRLF) - 1); > - *p++ = CR; *p = LF; > + p = ngx_cpymem(p, "250 STARTTLS" CRLF CRLF, sizeof("250 STARTTLS" CRLF > CRLF) - 1); > > p = conf->starttls_capability.data > + (last - conf->capability.data) + 3; See above. As I already suggested, correct patch seems to be: --- a/src/mail/ngx_mail_smtp_module.c +++ b/src/mail/ngx_mail_smtp_module.c @@ -277,7 +277,6 @@ ngx_mail_smtp_merge_srv_conf(ngx_conf_t p = ngx_cpymem(p, conf->capability.data, conf->capability.len); p = ngx_cpymem(p, "250 STARTTLS" CRLF, sizeof("250 STARTTLS" CRLF) - 1); - *p++ = CR; *p = LF; p = conf->starttls_capability.data + (last - conf->capability.data) + 3; -- Maxim Dounin http://nginx.org/en/donation.html _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
