On Wed, 2010-09-01 at 13:23 +0200, Len7hir wrote:
And another problem. Why You use safe_memset instead of memset? Now this function have the largest impact in Dovecot performance.
Really? I'd think it would be called very rarely since it should be used only for password-related data. I anyway added that at one point when I saw some people talking about how some compilers optimize away memset() when the compiler realizes that the memory isn't used anymore.
It's of course not super-helpful because if attacker can read some memory it's pretty bad already. But it might help to avoid exposing users' passwords in some buggy situations.
Another on list is t_push.
I don't know how much there is to be done about that. Or you could see what happens if you inline the fast path of that code. Or maybe the whole thing could be redesigned in a way to get the fast code path even smaller.
/* count lines and missing cr (from begining + 1) */last = (unsigned char *)data + 1;while (TRUE) {curr = memchr(last, '\n', block->size - (last - data));if (curr == NULL)break;ctx->part->body_size.lines++;if (*(curr - 1) != '\r')missing_cr_count++;last = curr + 1;}
I did a couple coding style changes here and to other patches to make it more consistent with rest of Dovecot (and also to help me understand the changes with less mental power :)
/* exceptional condition (test begining) */if (*data == '\n' && ctx->last_chr != '\r') {ctx->part->body_size.lines++;missing_cr_count++; }
There's a bug here: lines++ should be done even when last_chr != '\r'.
Also I moved this before the loop. I'd think that optimizes CPU cache behavior better (data is accessed linearly, not data[1] .. data[last], data[0]).
static int parse_next_body_skip_boundary_line(struct message_parser_ctx *ctx, struct message_block *block_r)
This original code was annoyingly complex. Also I think there was a bug in it ("no linefeeds in this block. we can just skip it" block probably shouldn't have been executed when \n was found at the end of the block). After looking at the code long enough I did some other changes besides what you did.
In istream-crlf.c I've changed one thing. When destination buffer is full after '\r' addition (to dest), It didn't skip '\n' in source buffer. I think this was buggy in earlier code, and '\n' was skipped (this piece of code is used very rare).
It looks correct to me .. After buffer is full after '\r', it doesn't skip '\n' because it's not in the buffer. So it comes back there the next time and adds '\n' (but not '\r' again).
if (*(s_ptr - 1) != '\r')*(d_ptr++) = '\r';
If data[0] == '\n' and also cstream->last_cr=TRUE, this code accesses data[-1] which points to random data.
http://hg.dovecot.org/dovecot-2.0/rev/e275c4f02501 http://hg.dovecot.org/dovecot-2.0/rev/e9358064c45e http://hg.dovecot.org/dovecot-2.0/rev/5163d94d4272