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