Re: [Dovecot] Bug with partial IMAP fetches
--- message-send.c.orig Sun Jun 22 18:09:48 2003 +++ message-send.c Tue Nov 4 12:53:04 2003 @@ -31,12 +31,11 @@ if (msg_size->physical_size == msg_size->virtual_size && !fix_nuls) { /* no need to kludge with CRs, we can use sendfile() */ - i_stream_skip(input, virtual_skip); old_limit = input->v_limit; limit = input->v_offset + max_virtual_size; i_stream_set_read_limit(input, I_MIN(limit, old_limit)); - ret = o_stream_send_istream(output, input) > 0; + ret = o_stream_send_istream(output, input); i_stream_set_read_limit(input, old_limit); return ret;
On Tuesday, Nov 4, 2003, at 21:10 Europe/Helsinki, Matthew Reimer wrote:
I think I've found the bug. The attached patch fixes the problem for me. The first problem is that the wrong value is being returned by message_send() because ret is getting set to the wrong value (see above or patch). The second problem is that in the special case where physical_size == virtual_size (i.e., sendfile can be used) i_stream_skip(input, virtual_skip) is being called on the input stream. This is wrong because virtual_skip is supposed to indicate that \r was the last character seen and therefore \n needs to be sent before any of the input stream; but in the sendfile case this is not necessary.
I think a little refactoring of seek_partial(), message_send() and message_skip_virtual() might be helpful to clarify the code. The virtual_skip argument to message_send() is an off_t but is only used to indicate that \r was the last character read, so it would probably be more clear to add another argument cr_skipped (like message_skip_virtual()). Better might be to put all the partial cache stuff in message-send.c.
It actually was supposed to be skip byte count, not just CR-indicator. But you're right, it was used wrong in sendfile code path. There was one piece of code anymore which used the virtual_skip more than just for CR (BODY[HEADER]<..>) and it was easy to change. I think the attached patch should fix it properly.
Timo Sirainen wrote:
On Tuesday, Nov 4, 2003, at 21:10 Europe/Helsinki, Matthew Reimer wrote:
I think I've found the bug. The attached patch fixes the problem for me. The first problem is that the wrong value is being returned by message_send() because ret is getting set to the wrong value (see above or patch). The second problem is that in the special case where physical_size == virtual_size (i.e., sendfile can be used) i_stream_skip(input, virtual_skip) is being called on the input stream. This is wrong because virtual_skip is supposed to indicate that \r was the last character seen and therefore \n needs to be sent before any of the input stream; but in the sendfile case this is not necessary.
I think a little refactoring of seek_partial(), message_send() and message_skip_virtual() might be helpful to clarify the code. The virtual_skip argument to message_send() is an off_t but is only used to indicate that \r was the last character read, so it would probably be more clear to add another argument cr_skipped (like message_skip_virtual()). Better might be to put all the partial cache stuff in message-send.c.
It actually was supposed to be skip byte count, not just CR-indicator. But you're right, it was used wrong in sendfile code path. There was one piece of code anymore which used the virtual_skip more than just for CR (BODY[HEADER]<..>) and it was easy to change. I think the attached patch should fix it properly.
I tested 0.99.10.2 and it works fine. Thanks!
Matt
participants (2)
-
Matthew Reimer
-
Timo Sirainen