Skip Menu |
 

This queue is for tickets about the Mail-IMAPClient CPAN distribution.

Report information
The Basics
Id: 42987
Status: resolved
Priority: 0/
Queue: Mail-IMAPClient

People
Owner: Nobody in particular
Requestors: phil [...] perkpartners.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

Attachments
IMAPClient.pm.message_string-append_string.patch



Subject: message_string() mismatched size handling too strict and verbose
Date: Tue, 03 Feb 2009 02:33:41 -0500
To: bug-Mail-IMAPClient [...] rt.cpan.org
From: Phil Lobbes <phil [...] perkpartners.com>
Download (untitled) / with headers
text/plain 4.5k
Using Mail::IMAPClient v3.13 (and earlier versions) against an IMAP server where RFC822.SIZE does not match the size of LITERAL size via BODY.PEEK some problems can arise. Here's data from a case I ran into: Sending: 19 UID FETCH 4 (RFC822.SIZE) Sent 30 bytes Read: * 4 FETCH (UID 4 RFC822.SIZE 75078) ... Sending: 20 UID FETCH 4 BODY.PEEK[] Sent 28 bytes LITERAL: received literal in line * 4 FETCH (UID 4 BODY[] of length 75076; attempting to retrieve from the 2863 bytes in... This generates these (duplicate) ERROR's: ERROR: message_string() expected 75078 bytes but received 75076 ERROR: message_string() expected 75078 bytes but received 75076 And these warnings: * Use of uninitialized value in substitution (s///) at .../lib/Mail/IMAPClient.pm line 2338. * Use of uninitialized value in length at .../lib/Mail/IMAPClient.pm line 2340. * Use of uninitialized value in concatenation (.) or string at .../lib/Mail/IMAPClient.pm line 2345. - These are the lines in question from append_string(): 2337 my $count = $self->Count($self->Count+1); 2338 $text =~ s/\r?\n/\r\n/g; 2339 2340 my $command = "$count APPEND $folder " . ($flags ? "$flags " : "") . 2341 ($date ? "$date " : "") . "{" . length($text) . "}\r\n"; 2342 2343 $self->_record($count,[$self->_next_index, "INPUT", $command]); 2344 2345 my $bytes = $self->_send_line($command.$text."\r\n"); 2346 unless(defined $bytes) Beyond the undefined warnings, I believe we need to decide what is the correct behavior. Currently we have this... sub message_string { my ($self, $msg) = @_; my $expected_size = $self->size($msg); [ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Note: this is RFC822.SIZE! ] ... $self->fetch($msg, $cmd) or return undef; my $string = $self->_transaction_literals; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unless($self->Ignoresizeerrors) { # Should this return undef if length != expected? # now, attempts are made to salvage parts of the message. if( length($string) != $expected_size ) .... I believe our current behavior of message_string is potentially overly strict. RFC2683 Section 3.4.5 "RFC822.SIZE" seems to address this issue. There's a bit to it, but I believe the most relevant statement is: " ... a client should use the size that's returned in the literal when you fetch the data." Since our code uses _transaction_literals which should (seems to) properly handle the literal size. This extra check of RFC822.SIZE vs the length of the literal string should not be necessary, is not recommended per the RFC, and perhaps more importantly should not be the default behavior. To make matters potentially worse, if the RFC822.SIZE is smaller than the actual message size then the code will truncate a message. This would probably cause valid data to be lost. If desired, I'll submit a patch that makes Ignoresizeerrors default to TRUE, but we the option could still be turned off to be more strict if desired. Here is a patch that: - only records the size mismatch error once - does NOT truncate LITERALS longer than RFC822.SIZE - enforces strictness of RFC822.SIZE vs LITERAL length check, so any mismatch in size (larger or smaller) causes an error and returns undef - eliminates undefined warnings in append_string on empty $text Phil --- IMAPClient.pm.OLD 2009-02-03 02:27:00.706750000 -0500 +++ IMAPClient.pm 2009-02-03 02:28:37.534875000 -0500 @@ -564,20 +564,13 @@ my $string = $self->_transaction_literals; + # Size check may be overly strict, see RFC2683 3.4.5 "RFC822.SIZE" unless($self->Ignoresizeerrors) { # Should this return undef if length != expected? - # now, attempts are made to salvage parts of the message. + # do not truncate the message if more data than RFC822.SIZE returned if( length($string) != $expected_size ) { $self->LastError("message_string() " . "expected $expected_size bytes but received ".length($string)); - } - - $string = substr $string, 0, $expected_size - if length($string) > $expected_size; - - if( length($string) < $expected_size ) - { $self->LastError("message_string() expected ". - "$expected_size bytes but received ".length($string)); return undef; } } @@ -2318,6 +2311,9 @@ my $folder = $self->Massage(shift); my ($text, $flags, $date) = @_; + # an empty message will likely fail but we will try anyway + $text = "" unless(defined $text); + if(defined $flags) { $flags =~ s/^\s+//g; $flags =~ s/\s+$//g;
Subject: Re: [rt.cpan.org #42987] message_string() mismatched size handling too strict and verbose
Date: Tue, 03 Feb 2009 02:44:39 -0500
To: bug-Mail-IMAPClient [...] rt.cpan.org
From: Phil Lobbes <phil [...] perkpartners.com>
Download (untitled) / with headers
text/plain 113b
Attached is an updated version of the patch which eliminates a call to size() if Ignoresizeerrors is set! Phil

Message body is not shown because sender requested not to inline it.

Subject: Re: [rt.cpan.org #42987] message_string() mismatched size handling too strict and verbose
Date: Tue, 3 Feb 2009 12:08:19 +0100
To: "phil [...] perkpartners.com via RT" <bug-Mail-IMAPClient [...] rt.cpan.org>
From: NLnet webmaster <webmaster [...] nlnet.nl>
Download (untitled) / with headers
text/plain 1.4k
* phil@perkpartners.com via RT (bug-Mail-IMAPClient@rt.cpan.org) [090203 07:45]: Show quoted text
> Queue: Mail-IMAPClient > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=42987 > > > Attached is an updated version of the patch which eliminates a call to > size() if Ignoresizeerrors is set!
Very nicely documented patch! And I do understand that servers may be 2 off in their size calculation. I think we need to provide a nice error message when size() is not working. So: unless($self->Ignoresizeerrors) { # Check size with expected size my $expected_size = $self->size($msg); unless(defined $expected_size) { $self->LastError( "message_string() cannot get message size," . " use IgnoreSizeErrors option"); return undef; } # RFC822.SIZE may be wrong # See RFC2683 3.4.5 "RFC822.SIZE" if(length($string) != $expected_size) { $self->LastError("message_string() " . "expected $expected_size bytes but received ".length($string)); return undef; } } Other changes included as well. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
Download (untitled) / with headers
text/plain 435b
I like the idea of referencing Ignoresizeerrors (not IgnoreSizeErrors) unless you've made some other patch/update to use FancyCaps :-). If you want to be a little more wordy, you could change... this: { $self->LastError( "message_string() cannot get message size," . " use IgnoreSizeErrors option"); to this: { $self->LastError( "message_string() cannot get message size," . " consider using Ignoresizeerrors option"); Thanks! Phil
Subject: Re: [rt.cpan.org #42987] message_string() mismatched size handling too strict and verbose
Date: Wed, 4 Feb 2009 08:52:38 +0100
To: Phil Lobbes via RT <bug-Mail-IMAPClient [...] rt.cpan.org>
From: Mark Overmeer <mark [...] overmeer.net>
Download (untitled) / with headers
text/plain 1.2k
* Phil Lobbes via RT (bug-Mail-IMAPClient@rt.cpan.org) [090203 23:20]: Show quoted text
> Queue: Mail-IMAPClient > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=42987 > > > I like the idea of referencing Ignoresizeerrors (not IgnoreSizeErrors) > unless you've made some other patch/update to use FancyCaps :-).
I dislike it, but the options are case-insensitive: (line 221) while(@_) { my $k = ucfirst lc shift; my $v = shift; $self->{$k} = $v if defined $v; } Show quoted text
> If you want to be a little more wordy, you could change... > > this: > { $self->LastError( "message_string() cannot get message size," > . " use IgnoreSizeErrors option"); > > to this: > { $self->LastError( "message_string() cannot get message size," > . " consider using Ignoresizeerrors option");
What about: " you may need the Ignoresizeerrors option" I mean: without that option, it will continue to break for the user... it is not really a choice, is it? -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
Download (untitled) / with headers
text/plain 1.3k
On Wed Feb 04 02:52:56 2009, Mark@Overmeer.net wrote: Show quoted text
> I dislike it, but the options are case-insensitive: (line 221)
Ugh, I dislike the case insensitive options as well, but I was thinking method and not option when writing that last response. It wouldn't mind removing that "feature". In the code I'm dealing with right now, I end up using the Ignoresizeerrors method so case insensitivity doesn't "help" anyway. Show quoted text
> What about: " you may need the Ignoresizeerrors option" > I mean: without that option, it will continue to break for the > user... it is not really a choice, is it?
Sounds good. If we were to consider that people want the code to just DWIM, then I would recommend reversing this option. *However* that certainly would cause problems for things like imapsync because it uses the RFC822.size as part of it's algorithm to uniquely identify messages so leaving the default as-is makes the most sense and I've separately submitted a patch to imapsync 1.267 to add a '--allowsizemismatch' option that would work well with imapsync's --skipsize option. --allowsizemismatch allow RFC822.SIZE != fetched msg size consider --skipsize to avoid duplicate messages when running syncs more than one time per mailbox That info should show up in the archives (http://www.linux-france.org/prj/imapsync_list/) soon probably.
Subject: Re: [rt.cpan.org #42987] message_string() mismatched size handling too strict and verbose
Date: Wed, 4 Feb 2009 22:13:23 +0100
To: Phil Lobbes via RT <bug-Mail-IMAPClient [...] rt.cpan.org>
From: Mark Overmeer <solutions [...] overmeer.net>
Download (untitled) / with headers
text/plain 1.6k
* Phil Lobbes via RT (bug-Mail-IMAPClient@rt.cpan.org) [090204 14:41]: Show quoted text
> Queue: Mail-IMAPClient > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=42987 > > > On Wed Feb 04 02:52:56 2009, Mark@Overmeer.net wrote:
> > I dislike it, but the options are case-insensitive: (line 221)
> > Ugh, I dislike the case insensitive options as well, but I was thinking > method and not option when writing that last response. It wouldn't mind > removing that "feature".
Cannot be done without breaking everyone's applications. You can never recover from such design mistakes. Show quoted text
> > What about: " you may need the Ignoresizeerrors option" > > I mean: without that option, it will continue to break for the > > user... it is not really a choice, is it?
> > Sounds good. If we were to consider that people want the code to just > DWIM, then I would recommend reversing this option.
DWIM is one side, but on the other hand: if people are unaware of problems with the server, then that will never get fixed. Helpful error messages are close to DWIM. Reversing defaults is also impossible in the general case. Show quoted text
> submitted a patch to imapsync 1.267 to add a '--allowsizemismatch' > option that would work well with imapsync's --skipsize option.
Good thinking. I think this ticket can be closed. I'll release a new version tomorrow. Feel invited to open new tickets. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
released in 3.14


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.