Archive::Zip 1.66 and 1.67 break a test in strip-nondeterminism

gregoa commented 5 years ago

As seen in https://bugs.debian.org/940973 Archive::Zip since 1.66 fails to read a test zip archive included in strip-nondeterminism. The test file is t/fixtures/zip/bug_803503.zip.in in the repo at https://salsa.debian.org/reproducible-builds/strip-nondeterminism (or directly: https://salsa.debian.org/reproducible-builds/strip-nondeterminism/blob/master/t/fixtures/zip/bug_803503.zip.in)

A minimal test case is:

#!/usr/bin/perl

use strict;
use warnings;

use Archive::Zip qw/:CONSTANTS :ERROR_CODES/;

my $zip_filename = 't/fixtures/zip/bug_803503.zip.in';
my $zip = Archive::Zip->new();

$zip->read($zip_filename);

which dies with

# ./test.pl 
IO error: reading local extra field :  
 at /usr/share/perl5/Archive/Zip/ZipFileMember.pm line 257.
    Archive::Zip::ZipFileMember::_readLocalFileHeader(Archive::Zip::ZipFileMember=HASH(0x55ff7f4e65a0)) called at /usr/share/perl5/Archive/Zip/ZipFileMember.pm line 106
    Archive::Zip::ZipFileMember::_become(Archive::Zip::ZipFileMember=HASH(0x55ff7f4e65a0), "Archive::Zip::DirectoryMember") called at /usr/share/perl5/Archive/Zip/Archive.pm line 798
    Archive::Zip::Archive::readFromFileHandle(Archive::Zip::Archive=HASH(0x55ff7ef57710), IO::File=GLOB(0x55ff7ef82328), "t/fixtures/zip/bug_803503.zip.in") called at /usr/share/perl5/Archive/Zip/Archive.pm line 729
    Archive::Zip::Archive::read(Archive::Zip::Archive=HASH(0x55ff7ef57710), "t/fixtures/zip/bug_803503.zip.in") called at ./test.pl line 11
error: becoming Archive::Zip::DirectoryMember 
 at /usr/share/perl5/Archive/Zip/Archive.pm line 800.
    Archive::Zip::Archive::readFromFileHandle(Archive::Zip::Archive=HASH(0x55ff7ef57710), IO::File=GLOB(0x55ff7ef82328), "t/fixtures/zip/bug_803503.zip.in") called at /usr/share/perl5/Archive/Zip/Archive.pm line 729
    Archive::Zip::Archive::read(Archive::Zip::Archive=HASH(0x55ff7ef57710), "t/fixtures/zip/bug_803503.zip.in") called at ./test.pl line 11

Cheers, gregor, Debian Perl Team

farblos commented 5 years ago

Thanks for the succinct test case.

Now the zip file is broken, as "unzip -t" shows:

[test]$ unzip -t bug_803503.zip.in 
Archive:  bug_803503.zip.in
foo/:  mismatching "local" filename (���/UT),
         continuing with "central" filename version
    testing: foo/                     invalid compressed data for EAs
error: invalid zip file with overlapped components (possible zip bomb)

I've tried Archive::Zip 1.63 and 1.65 on that zip file, both error out (1.65 shown below):

[test]$ perl -I../perl-Archive-Zip-1.65/lib test.pl 
IO error: reading local extra field :  
 at ../perl-Archive-Zip-1.65/lib/Archive/Zip/ZipFileMember.pm line 242.
    Archive::Zip::ZipFileMember::_readLocalFileHeader(Archive::Zip::ZipFileMember=HASH(0x564eb7be4200)) called at ../perl-Archive-Zip-1.65/lib/Archive/Zip/ZipFileMember.pm line 104
    Archive::Zip::ZipFileMember::_become(Archive::Zip::ZipFileMember=HASH(0x564eb7be4200), "Archive::Zip::DirectoryMember") called at ../perl-Archive-Zip-1.65/lib/Archive/Zip/Member.pm line 131
    Archive::Zip::Member::_becomeDirectoryIfNecessary(Archive::Zip::ZipFileMember=HASH(0x564eb7be4200)) called at ../perl-Archive-Zip-1.65/lib/Archive/Zip/Archive.pm line 625
    Archive::Zip::Archive::readFromFileHandle(Archive::Zip::Archive=HASH(0x564eb7bc6188), IO::File=GLOB(0x564eb7be4158), "bug_803503.zip.in") called at ../perl-Archive-Zip-1.65/lib/Archive/Zip/Archive.pm line 564
    Archive::Zip::Archive::read(Archive::Zip::Archive=HASH(0x564eb7bc6188), "bug_803503.zip.in") called at test.pl line 11

So what is the expected result in that case? A particular error being thrown?

(And BTW, the both issues in Debian bug #940973 are related only insofar as they have been introduced by the new version. Otherwise they are unrelated...)

gregoa commented 5 years ago

I've tried Archive::Zip 1.63 and 1.65 on that zip file, both error out (1.65 shown below):

[test]$ perl -I../perl-Archive-Zip-1.65/lib test.pl 
IO error: reading local extra field :  
 at ../perl-Archive-Zip-1.65/lib/Archive/Zip/ZipFileMember.pm line 242.
  Archive::Zip::ZipFileMember::_readLocalFileHeader(Archive::Zip::ZipFileMember=HASH(0x564eb7be4200)) called at ../perl-Archive-Zip-1.65/lib/Archive/Zip/ZipFileMember.pm line 104
  Archive::Zip::ZipFileMember::_become(Archive::Zip::ZipFileMember=HASH(0x564eb7be4200), "Archive::Zip::DirectoryMember") called at ../perl-Archive-Zip-1.65/lib/Archive/Zip/Member.pm line 131
  Archive::Zip::Member::_becomeDirectoryIfNecessary(Archive::Zip::ZipFileMember=HASH(0x564eb7be4200)) called at ../perl-Archive-Zip-1.65/lib/Archive/Zip/Archive.pm line 625
  Archive::Zip::Archive::readFromFileHandle(Archive::Zip::Archive=HASH(0x564eb7bc6188), IO::File=GLOB(0x564eb7be4158), "bug_803503.zip.in") called at ../perl-Archive-Zip-1.65/lib/Archive/Zip/Archive.pm line 564
  Archive::Zip::Archive::read(Archive::Zip::Archive=HASH(0x564eb7bc6188), "bug_803503.zip.in") called at test.pl line 11

So what is the expected result in that case? A particular error being thrown?

Now that's interesting and a bit embarassing, I was quite sure it worked as @lamby (well, actually @paulgevers) filed the bug after the update to 1.66.

(And BTW, the both issues in Debian bug #940973 are related only insofar as they have been introduced by the new version. Otherwise they are unrelated...)

Yes, the bug is a bit of a mess, sorry; that's why I waited a bit until I thought I saw one clear issue …

@lamby, do you know when the test actually started to fail and/or if the test file changed at some point?

Cheers, gregor

lamby commented 5 years ago

I'm.. not quite sure what you mean here. Do you mean in strip-determinism or in archive::zip? If the latter, then I am afraid I really haven't touched it. :)

paulgevers commented 5 years ago

Hi gregor,

On 14-10-2019 21:22, gregor herrmann wrote:

Now that's interesting and a bit embarassing, I was quite sure it worked as @lamby https://github.com/lamby (well, actually @paulgevers https://github.com/paulgevers) filed the bug after the update to 1.66.

In testing, strip-nondeterminism succeeds systematically with the older version of perl-archive-zip, while it fails systematically with the newer versions. So yes, it has all appearances of a regression in perl-archive-zip as it has been tried quite a few times now.

Paul

farblos commented 5 years ago

Hi @paulgevers,

I'm afraid that Archive::Zip has changed a lot from 1.65 => 1.66, and it can behave differently, but not necessarily in the wrong way.

For example, I redid the ...->contents methods to consistently behave as documented, that is, return a pair ( $contents, $status ) when called in list context. They did not always do so previously, and it might be very well that a call (as quoted from the Debian bug)

($con, $stat) = $zip->contents( 'META-INF/MANIFEST.MF' );

now (correctly) detects an error where it previously did not. As also @gregoa has noticed in his update, if I understand that particular update correctly.

Regarding the issue that Gregor has mentioned in this issue on bug_803503.zip.in, I can only guess that your tests check for return code AZ_IO_ERROR and now return code AZ_ERROR is returned. That I could change, in particular since the AZ_IO_ERROR is more appropriate.

It would be great if you could sort out your different failing tests a bit better, then I could see what could be done about them.

Thanks

Jens

farblos commented 5 years ago

Seems that there are actually three different issues mentioned in Debian bug #940973:

The one on t/fixtures/jar/jdependency.jar.out. This should have been fixed in 1.67.
The one on absent META-INF/MANIFEST.MF really being absent. Seems to be different but correct behavior.
Finally, the failure on t/fixtures/zip/bug_803503.zip.in.

Could you please provide the original source code for the last failure? Including all that error handling etc.?

gregoa commented 5 years ago

On Mon, 14 Oct 2019 14:55:09 -0700, farblos wrote:

Finally, the failure on t/fixtures/zip/bug_803503.zip.in.

Ack, this seems to be the last and only one left.

Recent test logs are at https://ci.debian.net/packages/s/strip-nondeterminism/unstable/amd64/

(And the still passing tests with Archive::Zip 1.65 are at https://ci.debian.net/packages/s/strip-nondeterminism/testing/amd64/ )

Could you please provide the original source code for the last failure? Including all that error handling etc.?

That's all at https://salsa.debian.org/reproducible-builds/strip-nondeterminism with the failing test being t/fixtures.t.

Thanks for looking into this issue!

Cheers, gregor

farblos commented 5 years ago

If you change your test case to print the return value of $zip->read:

my $status = $zip->read($zip_filename);

print "$status\n";

it gets clearer what's going on: Archive::Zip pre-1.66 only warns about errors in bug_803503.zip.in but happily returns AZ_OK when done reading. What your test suite interprets as "zip file successfully read".

In contrast to that, Archive::Zip 1.66 and newer not only warn, but also return an error (AZ_ERROR).

Not sure how I can help here ...

gregoa commented 5 years ago

On Tue, 15 Oct 2019 01:10:00 -0700, farblos wrote:

If you change your test case to print the return value of $zip->read:
my $status = $zip->read($zip_filename);

print "$status\n";
it gets clearer what's going on: Archive::Zip pre-1.66 only warns about errors in bug_803503.zip.in but happily returns AZ_OK when done reading. What your test suite interprets as "zip file successfully read". In contrast to that, Archive::Zip 1.66 and newer not only warn, but also return an error (AZ_ERROR).

Right, that was just my simplified test case.

strip-nondeterminism's code does check the return code, and if I output the value, I indeed get 2 (aka AZ_ERROR).

Not sure how I can help here ...

You helped a lot :)

But yes, it looks like

all good from the Archive::Zip side
bug_803503.zip.in was always broken
Archive::Zip < 1.66 nevertheless returned AZ_OK, so strip-nondeterminism continued [0]
Archive::Zip >= 1.66 fixed the bug and now correctly returns AZ_ERROR which makes strip-nondeterminism's test fail [1]

@lamby, it looks like we have to hand this back to you :)

Cheers, gregor

[0] and the warning is indeed in the logs:

# Subtest: t/fixtures/zip/bug_803503.zip.in
    ok 1 - Normalizer found for /tmp/VVDt2wY2NG/bug_803503.zip
IO error: reading local extra field :  
 at t/fixtures.t line 86.
Can't write to /tmp/z2U8UmJv1c.zip 
 at /usr/share/perl/5.30/Test/Builder.pm line 333.
    ok 2 - Test output /tmp/VVDt2wY2NG/bug_803503.zip matched expected t/fixtures/zip/bug_803503.zip.out
    ok 3 - t/fixtures/zip/bug_803503.zip.in: dev (device number of filesystem)
    ok 4 - t/fixtures/zip/bug_803503.zip.in: mode (file mode (type and permissions))
    ok 5 - t/fixtures/zip/bug_803503.zip.in: nlink (number of hard links to the file)
    ok 6 - t/fixtures/zip/bug_803503.zip.in: uid (numeric user ID of file's owner)
    ok 7 - t/fixtures/zip/bug_803503.zip.in: gid (numeric group ID of file's owner)
    ok 8 - t/fixtures/zip/bug_803503.zip.in: rdev (the device identifier; special files only)
    ok 9 - Unexpected files leftover: /tmp/VVDt2wY2NG/bug_803503.zip
    1..9
ok 20 - t/fixtures/zip/bug_803503.zip.in

at https://ci.debian.net/data/autopkgtest/testing/amd64/s/strip-nondeterminism/3170873/log.gz

[1] with the same warning as before:

# Subtest: t/fixtures/zip/bug_803503.zip.in
    ok 1 - Normalizer found for /tmp/SQn9GWpPvM/bug_803503.zip
    1..1
ok 20 - t/fixtures/zip/bug_803503.zip.in
Reading ZIP archive failed: IO error: reading local extra field :  

error: becoming Archive::Zip::DirectoryMember 
# Looks like your test exited with 29 just after 20.

("Reading ZIP archive failed: " is from strip-nondeterminism, "IO …" is the output of Archive::Zip)

-- .''. https://info.comodo.priv.at -- Debian Developer https://www.debian.org : :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D 85FA BB3A 6801 8649 AA06 . ' Member VIBE!AT & SPI Inc. -- Supporter Free Software Foundation Europe - NP: Steppenwolf: Wild Thing

lamby commented 5 years ago

Archive::Zip >= 1.66 fixed the bug and now correctly returns AZ_ERROR which makes strip-nondeterminism's test fail [1]

So I think what we want/can to do here is simply drop this fixture test, right? :)

gregoa commented 5 years ago

On Tue, 15 Oct 2019 11:40:07 -0700, Chris Lamb wrote:

Archive::Zip >= 1.66 fixed the bug and now correctly returns AZ_ERROR which makes strip-nondeterminism's test fail [1] So I think what we want/can to do here is simply drop this fixture test, right? :)

Yup, simple git-rm'ing t/fixtures/zip/bug_803503.zip* seems the way to go :)

Thanks again, @farblos, and feel free to close this issue.

Cheers, gregor

-- .''. https://info.comodo.priv.at -- Debian Developer https://www.debian.org : :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D 85FA BB3A 6801 8649 AA06 . ' Member VIBE!AT & SPI Inc. -- Supporter Free Software Foundation Europe - NP: Steppenwolf: Wild Thing

redhotpenguin / perl-Archive-Zip

Archive::Zip 1.66 and 1.67 break a test in strip-nondeterminism #68