Bug #88297 for PAR-Packer: -M crashes pp if option contains backslashes

Fri Aug 30 20:11:35 2013 NGLENN [...] cpan.org - Ticket created

CC:	garfieldnate [...] gmail.com
Subject:	-M crashes pp if option contains backslashes

Doing `pp -M path\to\my\Lib.pm` causes a failure: Can't call method "desiredCompressionMethod" on an undefined value at C:/[my path]/PAR/Packer.pm line 1139. The problem is that Archive::Zip stores members with Unixy names, but Packer.pm was keeping the name with backslashes in it. Then when it called $zip->member($in), the member with the given name didn't exist. I'm including a patch to fix this (generated with diff -u Packer.pm PackerNew.pm > file).

Subject:

Packer.pm.patch

--- /var/tmp/.diff790528876 2013-08-31 04:06:21.000000000 +0400 +++ /var/tmp/.diff1747176174 2013-08-31 04:06:21.000000000 +0400 @@ -1135,7 +1135,10 @@ $oldsize += length($str); $self->_vprint(1, "... adding <string> as $in"); - $zip->addString($str => $in)->unixFileAttributes(0644); + my $member = $zip->addString($str => $in); + $member->unixFileAttributes(0644); + # Archive::Zip might have renamed member to something more Unixy + $in = $member->fileName; $zip->memberNamed($in)->desiredCompressionMethod($method); $zip->memberNamed($in)->desiredCompressionLevel($level); }

Sun Sep 01 15:22:41 2013 RSCHUPP [...] cpan.org - Taken

Mon Sep 02 09:04:14 2013 RSCHUPP [...] cpan.org - Correspondence added

On 2013-08-30 20:11:35, NGLENN wrote: Show quoted text

> Doing `pp -M path\to\my\Lib.pm` causes a failure:

I can reproduce the failure, but this command is kinda pointless: it would result in path\to\my\Lib.pm being packed as module path::to::my::Lib which is probably not want you want. And if you wanted it, why not say -M path::to::my::Lib? Show quoted text

> ... Then when it > called $zip->member($in), the member with the given name didn't exist. > I'm including a patch to fix this

Your patch only pampers over the problem. Also it's very bad style to modify the parameters ($in in this case) of a function (except when it's to provide a default value). The formulation in the original code is also lacking, though: addString and addFile already return an Archive::Zip::Member object, so why not use this to set compression options etc. I applied the attached fix instead, will be in the next release of PAR::Packer. Cheers, Roderich

Subject:

88297.patch

Index: lib/PAR/Packer.pm =================================================================== --- lib/PAR/Packer.pm (revision 1414) +++ lib/PAR/Packer.pm (working copy) @@ -1090,9 +1090,9 @@ $manifest->{ $alias } = [ file => $file ]; $oldsize += -s $file; - $zip->addFile($file, $alias); - $zip->memberNamed($alias)->desiredCompressionMethod($method); - $zip->memberNamed($alias)->desiredCompressionLevel($level); + my $member = $zip->addFile($file, $alias); + $member->desiredCompressionMethod($method); + $member->desiredCompressionLevel($level); } } elsif (-e $fn and -r $fn) { @@ -1103,9 +1103,9 @@ $self->_vprint(1, "... adding $fn as $in\n"); $oldsize += -s $fn; - $zip->addFile($fn => $in); - $zip->memberNamed($in)->desiredCompressionMethod($method); - $zip->memberNamed($in)->desiredCompressionLevel($level); + my $member = $zip->addFile($fn => $in); + $member->desiredCompressionMethod($method); + $member->desiredCompressionLevel($level); } } else { @@ -1117,9 +1117,10 @@ $oldsize += length($str); $self->_vprint(1, "... adding <string> as $in"); - $zip->addString($str => $in)->unixFileAttributes(0644); - $zip->memberNamed($in)->desiredCompressionMethod($method); - $zip->memberNamed($in)->desiredCompressionLevel($level); + my $member = $zip->addString($str => $in); + $member->unixFileAttributes(0644); + $member->desiredCompressionMethod($method); + $member->desiredCompressionLevel($level); } $self->{pack_attrib}{old_size} = $oldsize;

Mon Sep 02 09:04:14 2013 The RT System itself - Status changed from 'new' to 'open'

Mon Sep 02 14:12:38 2013 NGLENN [...] cpan.org - Correspondence added

RT-Send-CC:

garfieldnate [...] gmail.com

Thanks for the tips, and for the quick fix!

Mon Sep 02 15:26:20 2013 NGLENN [...] cpan.org - Correspondence added

RT-Send-CC:

garfieldnate [...] gmail.com

Sorry to bring this up again. Just in the way of documentation, executing pp the way I did still results in an error later on. If somebody gets the following error the first time they run their executable (and not subsequent times), it is because they used the -M option incorrectly (the way I did): mkdir C:\Users\...\AppData\Local\Temp\par-xyz\cache-abc/inc/lib/C:/: Invalid argument; The filename, directory name, or volume label syntax is incorrect at C:/strawberry/perl/vendor/lib/Archive/Zip/Archive.pm line 192. This is purely due to user error, but a fail-fast mechanism might be useful. Otherwise, this will at least be a web search result.

Tue Sep 03 02:44:48 2013 RSCHUPP [...] cpan.org - Correspondence added

On 2013-09-02 15:26:20, NGLENN wrote: Show quoted text

> This is purely due to user error, but a fail-fast mechanism might be useful.

Maybe we could check whether -M was given an absolute pathname and at least print a warning. Even on *nix, an absolute pathname would probably not do what you wanted (though it would not result in an outright error). Cheers, Roderich

Mon Sep 16 16:49:13 2013 NGLENN [...] cpan.org - Correspondence added

Show quoted text

> Maybe we could check whether -M was given an absolute pathname > and at least print a warning.

This seems to do the trick: --- lib/PAR/PackerOLD.pm 2013-09-17 00:33:16.000000000 +0400 +++ lib/PAR/Packer.pm 2013-09-17 00:33:16.000000000 +0400 @@ -675,6 +675,9 @@ my (@modules, @data, @exclude); foreach my $name (@{ $opt->{M} || [] }) { + if(File::Spec->file_name_is_absolute($name)){ + $self->_warn('Absolute path used in -M option'); + } $self->_name2moddata($name, \@modules, \@data); } I don't know if $self->_warn is a good thing to do here, since it prints the line number in Packer.pm, potentially making the user think that there's something wrong with the module. Don't know if the message is the best either.

Wed Oct 09 07:43:00 2013 RSCHUPP [...] cpan.org - Correspondence added

Fixed in PAR::Packer 1.015

Wed Oct 09 07:43:01 2013 RSCHUPP [...] cpan.org - Status changed from 'open' to 'resolved'