Matthew Reimer wrote:
There is a bug in partial IMAP fetches where the following happens:

fetch ... <0.10240>
 correctly returns bytes 0-10239
fetch ... <10240.10240>
 incorrectly returns bytes 20479...
fetch ... <10240.10240> (a 2nd time)
 correctly returns bytes 10240-20479

While the second fetch fails, the third fetch works correctly. I tracked this down to a problem with partial_cache. There seems to be a bug in lib-mail/message-send.c:39 (CVS):

    ret = o_stream_send_istream(output, input) > 0;

This causes message_send() to return 0 or 1 (note the "> 0"), not the number of bytes sent. This in turn causes partial.pos.virtual_size to get a value of 1 rather than 10240 in fetch_body(), and this in turn results in the wrong portion of the message being returned for the subsequent fetch.

But then when I remove the "> 0" from the above line of code, I get off-by-one errors:

fetch ... <0.10240>
 correctly returns bytes 0-10240
fetch ... <10240.10240>
 incorrectly returns bytes 10241-20480
fetch ... <10240.10240> (a 2nd time)
 correctly returns bytes 10240-20479

I'm guessing there is a bug in the interaction with message_skip_virtual(), but I haven't been able to find the problem yet.

Any ideas? I've attached the offending message.

Matt
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.

Matt