Skip Menu |

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

Report information
The Basics
Id: 21276
Status: resolved
Priority: 0/
Queue: Mail-GnuPG

People
Owner: Nobody in particular
Requestors: Iikka.Virkkunen [...] trueflaw.com
Cc: ntyni [...] iki.fi
AdminCc:

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



Subject: Problem decoding large messages
Date: Sat, 2 Sep 2006 13:28:37 +0300
To: bug-Mail-GnuPG [...] rt.cpan.org
From: Iikka Virkkunen <Iikka.Virkkunen [...] trueflaw.com>
Dear Sir, Firtly, thank yuo for the very nicely crafted Mail::GnuPG module. While trying to implement an automatic decrypter, I ran into a peculiar problem. When the message size exceeded some 80000 chars, Mail::GnuPG would hang. I traced this as far as I could, but I don't accurately know, what's the reason. Probably the write-buffer of the input-handle fills before reading starts. Anyway, the following work- around worked: First, the original code: Show quoted text
> my ( $input, $output, $error, $passphrase_fh, $status_fh ) > = ( new IO::Handle, new IO::Handle,new IO::Handle, > new IO::Handle,new IO::Handle,); > > my $handles = GnuPG::Handles->new( stdin => $input, > stdout => $output, > stderr => $error, > passphrase => $passphrase_fh, > status => $status_fh, > ); > > # this sets up the communication > my $pid = $gnupg->decrypt( handles => $handles ); > ... > print $input $ciphertext;
And here's the workaround: my ( $input, $output, $error, $passphrase_fh, $status_fh ) = ( IO::File->new_tmpfile(), new IO::Handle,new IO::Handle, new IO::Handle,new IO::Handle,); my $handles = GnuPG::Handles->new( stdin => $input, stdout => $output, stderr => $error, passphrase => undef, # $passphrase_fh, status => $status_fh, ); $handles->options( 'stdin' )->{direct} = 1; $input->print($ciphertext); $input->flush(); $input->seek(0,0); my $pid = $gnupg->decrypt( handles => $handles ); Best regards, Iikka Virkkunen
From: ntyni [...] debian.org
On Sat Sep 02 06:29:51 2006, Iikka.Virkkunen@trueflaw.com wrote: Show quoted text
> While trying to implement an automatic decrypter, I ran into a > peculiar problem. When the message size exceeded some 80000 chars, > Mail::GnuPG would hang. I traced this as far as I could, but I don't > accurately know, what's the reason. Probably the write-buffer of the > input-handle fills before reading starts. Anyway, the following work- > around worked:
Hi, the problem is indeed the pipe buffers filling up, making both Mail::GnuPG and gpg block on write(2). The same problem shows up in most of the Mail::GnuPG methods. It was recently also reported as Debian bug #459693, http://bugs.debian.org/459693 . FWIW, this can be reproduced for me with 0.10 by using a longer message in t/roundtrip.t. Something like Data => ["Line 1","Line 2\n" x 15000]); hangs for me, and this is in the rough 80000 character range that Iikka mentioned above. I'm attaching a proposed patch that implements a select(2) based loop for communication with gpg, interleaving the reads and the writes. It might seem a bit overkill, but I think it's the Right Way to fix this. This also handles SIGPIPE properly, so the patch I filed in ticket #25530 is not needed anymore. Please consider including the patch to fix this long-standing bug. Thanks for your work on Mail::GnuPG, -- Niko Tyni ntyni@debian.org
diff --git a/GnuPG.pm b/GnuPG.pm index afc9d45..d6180ad 100644 --- a/GnuPG.pm +++ b/GnuPG.pm @@ -22,6 +22,7 @@ use strict; use warnings; our $VERSION = '0.10'; +my $DEBUG = 0; use GnuPG::Interface; use File::Spec; @@ -30,6 +31,8 @@ use IO::Handle; use MIME::Entity; use MIME::Parser; use Mail::Address; +use IO::Select; +use Errno qw(EPIPE); =head2 new @@ -160,26 +163,16 @@ sub decrypt { # this sets up the communication my $pid = $gnupg->decrypt( handles => $handles ); - # This passes in the passphrase die "NO PASSPHRASE" unless defined $passphrase_fh; - print $passphrase_fh $self->{passphrase}; - close $passphrase_fh; + my $read = _communicate([$output, $error, $status_fh], + [$input, $passphrase_fh], + { $input => $ciphertext, + $passphrase_fh => $self->{passphrase}} + ); - # this passes in the plaintext - print $input $ciphertext; - - # this closes the communication channel, - # indicating we are done - close $input; - - my @plaintext = <$output>; # reading the output - my @error_output = <$error>; # reading the error - my @status_info = <$status_fh>;# read the status info - - # clean up... - close $output; - close $error; - close $status_fh; + my @plaintext = split(/^/m, $read->{$output}); + my @error_output = split(/^/m, $read->{$error}); + my @status_info = split(/^/m, $read->{$status_fh}); waitpid $pid, 0; my $return = $?; @@ -284,18 +277,10 @@ sub get_decrypt_key { command_args => [ "--batch", "--list-only", "--status-fd", "1" ], ); - # this passes in the ciphertext - print $input $ciphertext; - - # this closes the communication channel, - # indicating we are done - close $input; + my $read = _communicate([$output], [$input], { $input => $ciphertext }); # reading the output - my @result = <$output>; - - # clean up... - close $output; + my @result = split(/^/m, $read->{$output}); # clean up the finished GnuPG process waitpid $pid, 0; @@ -438,11 +423,9 @@ sub verify { "$sigfile" ), ); - # Now we write to the input of GnuPG - # now we read the output - my @result = <$error>; - close $error; - close $input; + my $read = _communicate([$error], [$input], {$input => ''}); + + my @result = split(/^/m, $read->{$error}); unlink $sigfile, $datafile; @@ -561,9 +544,6 @@ sub mime_sign { ); my $pid = $gnupg->detach_sign( handles => $handles ); die "NO PASSPHRASE" unless defined $passphrase_fh; - print $passphrase_fh $self->{passphrase}; - close $passphrase_fh; - # this passes in the plaintext my $plaintext; @@ -577,9 +557,6 @@ sub mime_sign { $plaintext =~ s/\x0A/\x0D\x0A/g; $plaintext =~ s/\x0D+/\x0D/g; - # should we store this back into the body? - print $input $plaintext; - # DEBUG: # print "SIGNING THIS STRING ----->\n"; # $plaintext =~ s/\n/-\n/gs; @@ -587,19 +564,15 @@ sub mime_sign { # warn($entity->as_string); # print STDERR $plaintext; # print "<----\n"; - $input->flush(); - eval { $input->sync() }; # IO::Handle::sync not implemented on - # all systems. - close $input; + my $read = _communicate([$output, $error, $status_fh], + [$input, $passphrase_fh], + { $input => $plaintext, + $passphrase_fh => $self->{passphrase}} + ); - my @signature = <$output>; # reading the output - my @error_output = <$error>; # reading the error - my @status_info = <$status_fh>;# read the status info - - # clean up... - close $output; - close $error; - close $status_fh; + my @signature = split(/^/m, $read->{$output}); + my @error_output = split(/^/m, $read->{$error}); + my @status_info = split(/^/m, $read->{$status_fh}); waitpid $pid, 0; my $return = $?; @@ -675,15 +648,11 @@ sub clear_sign { $plaintext =~ s/\x0A/\x0D\x0A/g; $plaintext =~ s/\x0D+/\x0D/g; - print $input $plaintext; - close $input; + my $read = _communicate([$output, $error], [$input], { $input => $plaintext }); - my @ciphertext = <$output>; - my @error_output = <$error>; + my @ciphertext = split(/^/m, $read->{$output}); + my @error_output = split(/^/m, $read->{$error}); - close $output; - close $error; - waitpid $pid, 0; my $return = $?; $return = 0 if $return == -1; @@ -781,15 +750,11 @@ sub _ascii_encrypt { } }; - print $input $plaintext; - close $input; + my $read = _communicate([$output, $error], [$input], { $input => $plaintext }); - my @ciphertext = <$output>; - my @error_output = <$error>; + my @ciphertext = split(/^/m, $read->{$output}); + my @error_output = split(/^/m, $read->{$error}); - close $output; - close $error; - waitpid $pid, 0; my $return = $?; $return = 0 if $return == -1; @@ -885,10 +850,6 @@ sub _mime_encrypt { } }; - die "NO PASSPHRASE" unless defined $passphrase_fh; - print $passphrase_fh $self->{passphrase}; - close $passphrase_fh; - # this passes in the plaintext my $plaintext; if ($workingentity eq $entity) { @@ -901,23 +862,22 @@ sub _mime_encrypt { # $plaintext =~ s/\n/\x0D\x0A/sg; # should we store this back into the body? - print $input $plaintext; - # DEBUG: #print "ENCRYPTING THIS STRING ----->\n"; # print $plaintext; # print "<----\n"; - close $input; - - my @ciphertext = <$output>; # reading the output - my @error_output = <$error>; # reading the error - my @status_info = <$status_fh>;# read the status info + die "NO PASSPHRASE" unless defined $passphrase_fh; + my $read = _communicate([$output, $error, $status_fh], + [$input, $passphrase_fh], + { $input => $plaintext, + $passphrase_fh => $self->{passphrase}} + ); - # clean up... - close $output; - close $error; - close $status_fh; + my @plaintext = split(/^/m, $read->{$output}); + my @ciphertext = split(/^/m, $read->{$output}); + my @error_output = split(/^/m, $read->{$error}); + my @status_info = split(/^/m, $read->{$status_fh}); waitpid $pid, 0; my $return = $?; @@ -991,6 +951,112 @@ sub is_encrypted { return 0; } +# interleave reads and writes +# input parameters: +# $rhandles - array ref with a list of file handles for reading +# $whandles - array ref with a list of file handles for writing +# $wbuf_of - hash ref indexed by the stringified handles +# containing the data to write +# return value: +# $rbuf_of - hash ref indexed by the stringified handles +# containing the data that has been read +# +# read and write errors due to EPIPE (gpg exit) are skipped silently on the +# assumption that gpg will explain the problem on the error handle +# +# other errors cause a non-fatal warning, processing continues on the rest +# of the file handles +# +# NOTE: all the handles get closed inside this function + +sub _communicate { + my $blocksize = 2048; + my ($rhandles, $whandles, $wbuf_of) = @_; + my $rbuf_of = {}; + + # the current write offsets, again indexed by the stringified handle + my $woffset_of; + + my $reader = IO::Select->new; + for (@$rhandles) { + $reader->add($_); + $rbuf_of->{$_} = ''; + } + + my $writer = IO::Select->new; + for (@$whandles) { + die("no data supplied for handle " . fileno($_)) if !exists $wbuf_of->{$_}; + if ($wbuf_of->{$_}) { + $writer->add($_); + } else { # nothing to write + close $_; + } + } + + # we'll handle EPIPE explicitly below + local $SIG{PIPE} = 'IGNORE'; + + while ($reader->handles || $writer->handles) { + my @ready = IO::Select->select($reader, $writer, undef, undef); + if (!@ready) { + die("error doing select: $!"); + } + my ($rready, $wready, $eready) = @ready; + if (@$eready) { + die("select returned an unexpected exception handle, this shouldn't happen"); + } + for my $rhandle (@$rready) { + my $n = fileno($rhandle); + my $count = sysread($rhandle, $rbuf_of->{$rhandle}, + $blocksize, length($rbuf_of->{$rhandle})); + warn("read $count bytes from handle $n") if $DEBUG; + if (!defined $count) { # read error + if ($!{EPIPE}) { + warn("read failure (gpg exited?) from handle $n: $!") + if $DEBUG; + } else { + warn("read failure from handle $n: $!"); + } + $reader->remove($rhandle); + close $rhandle; + next; + } + if ($count == 0) { # EOF + warn("read done from handle $n") if $DEBUG; + $reader->remove($rhandle); + close $rhandle; + next; + } + } + for my $whandle (@$wready) { + my $n = fileno($whandle); + $woffset_of->{$whandle} = 0 if !exists $woffset_of->{$whandle}; + my $count = syswrite($whandle, $wbuf_of->{$whandle}, + $blocksize, $woffset_of->{$whandle}); + if (!defined $count) { + if ($!{EPIPE}) { # write error + warn("write failure (gpg exited?) from handle $n: $!") + if $DEBUG; + } else { + warn("write failure from handle $n: $!"); + } + $writer->remove($whandle); + close $whandle; + next; + } + warn("wrote $count bytes to handle $n") if $DEBUG; + $woffset_of->{$whandle} += $count; + if ($woffset_of->{$whandle} >= length($wbuf_of->{$whandle})) { + warn("write done to handle $n") if $DEBUG; + $writer->remove($whandle); + close $whandle; + next; + } + } + } + return $rbuf_of; +} + # FIXME: there's no reason why is_signed and is_encrypted couldn't be # static (class) methods, so maybe we should support that.
I'm unable to replicate this by modifying round-trip.t to generate a large message. Can someone send a patch to round-trip.t to create a repeatable failure? (I suspect the select based solution is the right answer long term, just want to be able to prove that it's working.) -R (This does not fail.) $ svn diff Index: t/round-trip.t =================================================================== --- t/round-trip.t (revision 10686) +++ t/round-trip.t (working copy) @@ -31,11 +31,14 @@ isa_ok($mg,"Mail::GnuPG"); +my $line = "x" x 80; +my $string = $line x 100000; + my $copy; my $me = MIME::Entity->build(From => 'me@myhost.com', To => 'you@yourhost.com', Subject => "Hello, nurse!", - Data => ["Line 1","Line 2"]); + Data => [$string]); # Test MIME Signing Round Trip $copy = $me->dup;
From: ntyni [...] iki.fi
On Sun Feb 03 03:35:46 2008, RSPIER wrote: Show quoted text
> I'm unable to replicate this by modifying round-trip.t to generate a > large message. > > Can someone send a patch to round-trip.t to create a repeatable failure?
Show quoted text
> (This does not fail.)
Show quoted text
> +my $line = "x" x 80; > +my $string = $line x 100000;
As seen with strace, gpg is discarding the input here: read(5, "gpg: input line longer than 19995 characters\n", 4096) = 45 The attached patch makes the test block repeatedly for me by embedding newlines in the string. Hope this helps, -- Niko
Download mail-gnupg-test
application/octet-stream 541b

Message body not shown because it is not plain text.

CC: undisclosed-recipients:;
Subject: Re: [rt.cpan.org #21276] Problem decoding large messages
Date: Sat, 23 Feb 2008 22:13:06 -0800
To: "ntyni [...] iki.fi via RT" <bug-Mail-GnuPG [...] rt.cpan.org>
From: Robert Spier <rspier [...] pobox.com>
Thanks Niko, I've confirmed that your patch to round-trip.t causes a repeatable failure. I've started looking at your previous patch to replace the communication method with a select based loop. I like the logic and control flow. It looks like there's been a slight API change as a side-effect: -is_deeply($mg->{decrypted}->body,$me->body); +is_deeply([$mg->{decrypted}->body],[$me->body]); body returns a list now, instead of a reference. Was that intentional? -R
From: ntyni [...] iki.fi
On Sun Feb 24 01:13:37 2008, rspier@pobox.com wrote: Show quoted text
> It looks like there's been a slight API change as a side-effect: > > -is_deeply($mg->{decrypted}->body,$me->body); > +is_deeply([$mg->{decrypted}->body],[$me->body]); > > body returns a list now, instead of a reference. > > Was that intentional?
Um, I don't think I have touched anything to that effect. Maybe you're using a buggy version of MIME-Tools? See CPAN #29643, fixed in 5.423. (Note that I never received your question, because I'm not the submitter of this ticket. Please add me to the CC list of this ticket if you have the privileges to do that.) Cheers, -- Niko Tyni ntyni@debian.org
CC: undisclosed-recipients:;
Subject: Re: [rt.cpan.org #21276] Problem decoding large messages
Date: Sat, 08 Mar 2008 20:00:49 -0800
To: "ntyni [...] iki.fi via RT" <bug-Mail-GnuPG [...] rt.cpan.org>
From: Robert Spier <rspier [...] pobox.com>
ntyni@iki.fi via RT wrote: Show quoted text
> > > Queue: Mail-GnuPG > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=21276 > > > On Sun Feb 24 01:13:37 2008, rspier@pobox.com wrote: >
> > It looks like there's been a slight API change as a side-effect: > > > > -is_deeply($mg->{decrypted}->body,$me->body); > > +is_deeply([$mg->{decrypted}->body],[$me->body]); > > > > body returns a list now, instead of a reference. > > > > Was that intentional?
> > Um, I don't think I have touched anything to that effect. Maybe you're > using a buggy version of MIME-Tools? See CPAN #29643, fixed in > 5.423.
Correct. Patch applied. Mail::GnuPG 0.15 uploaded to CPAN, it should show up on your local mirror within a day. Show quoted text
> (Note that I never received your question, because I'm not the submitter > of this ticket. Please add me to the CC list of this ticket if you have > the privileges to do that.)
Done. Thanks!