Mail::Message::Body::Multipart adds extraneous blank line between end-delimiters

[jbalazerpfpt]

The fix to issue #16 introduced a bug. When a multipart body is nested inside another multipart, if the inner part had no epilogue (no blank lines between the end-delimiter at the end of the child multipart and the separator that follows) the fixed code would incorrectly add a blank line after the end-delimiter. That earlier fix was correct for a multipart at the message level, but incorrect for this multipart. See the attached test message multipart_close-delimiter_test_nested_no_epilogue_between_close-delimiters.eml.txt.

Intertwined with this issue, we've had a problem of unbounded recursion, and so we've made a change to track recursion depth. The epilogue fix relies on that depth counter to know if the multipart body is nested or not. Here's the combined set of changes:

--- Multipart.pm-3.015  2024-11-11 16:40:37.879470200 -0800
+++ Multipart.pm       2024-11-11 17:07:11.149641000 -0800
@@ -21,6 +21,7 @@
 use Mail::Box::FastScalar;
 use Carp;

+our $max_depth = 100;

 sub init($)
 {   my ($self, $args) = @_;
@@ -122,6 +123,8 @@
 {   my $self   = shift;
     my $bbytes = length($self->boundary) +4;  # \n--$b\n

+    no warnings 'recursion';
+
     my $bytes  = $bbytes +2;   # last boundary, \n--$b--\n
     if(my $preamble = $self->preamble)
          { $bytes += $preamble->size }
@@ -179,6 +182,8 @@
 {   my $self = shift;
     my $out  = shift || select;

+    no warnings 'recursion';
+
     my $boundary = $self->boundary;
     my $count    = 0;
     if(my $preamble = $self->preamble)
@@ -268,6 +273,7 @@
     # Get the parts.

     my ($has_epilogue, @parts);
+    croak "Deep nesting" if ($self->{MMB_message}->{MMP_depth} || 0) >= $max_depth;
     while(my $sep = $parser->readSeparator)
     {   if($sep =~ m/^--\Q$boundary\E--[ \t]*\n?/)
         {   # Per RFC 2046, a CRLF after the close-delimiter marks the presence
@@ -280,7 +286,7 @@

         my $part = Mail::Message::Part->new
          ( @msgopts
-         , container => $self
+         , container => $self, depth => ($self->{MMB_message}->{MMP_depth} || 0)+ 1
          );

         last unless $part->readFromParser($parser, $bodytype);
@@ -301,8 +307,15 @@
             :                      $begin;
     $self->fileLocation($begin, $end);

-    $has_epilogue || $epilogue->nrLines
-        or undef $epilogue;
+    if ($self->{MMB_message}->{MMP_depth}) {
+        # Nested case: another separator follows the end of this Multipart body,
+        # so if the epilogue has no lines, then there's actually no epilogue
+        # because the EOL char at the end of this Multipart body's end-delimiter
+        # logically goes with the separator that follows.
+        undef $epilogue unless ($epilogue->nrLines);
+    } else {
+        undef $epilogue unless ($has_epilogue);
+    }

     $self->{MMBM_epilogue} = $epilogue
         if defined $epilogue;

[markov2]

Thanks for the patch. I have a few remarks:

1. apparently, you want to make the recursion fatal. This implementation can live without a depth counter

use warnings FATAL => 'recursion';

2. There is no 'depth' attribute in ::Part. Probably, you missed that change in this patch.
3. 'croak' is not the way that runtime errors are handled in MailBox, only for coding errors.
4. $max_depth is an outside configurable, so Perl's convention is to use CAPS in that case.
5. I am in doubt for the name depth... is $part->depth clear enough? Or should it be $part->nesting?
6. I would never, ever accept access to object fields unless the attribute is implemented in the same file. So: no MMB or MMP in MMBM. That kind of coding makes applications unmaintainable.

I think the depth can be implemented like this:

package Mail::Message;
sub depth() { 0 }

package Mail::Message::Path;
sub depth() { $_[0]->container->message->depth + 1 }    # or:
sub depth() { $_[0]->{MMP_depth} //= $_[0]->container->message->depth + 1 }

package ::Multipart;
$self->message->depth < $max_depth or croak;
undef $epilogue if $self->message->isPart ? $epilogue->nrLines : !$has_epilogue;

How much do you agree with me?