data_start is incorrect when reading an aligned file

[danielocfb]

I tried upgrading to version 1.1 and it seems as if handling of alignment is broken.

use std::env;
use std::fs::read as read_file;
use std::fs::File;
use std::io::Write as _;
use std::path::Path;

use zip::write::SimpleFileOptions;
use zip::CompressionMethod;
use zip::ZipArchive;
use zip::ZipWriter;

fn main() {
    let page_size = 4096;
    let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap();
    let dst_dir = Path::new(&crate_dir);
    let dst_file = dst_dir.join("test.zip");

    {
        let dst_file = File::options()
            .create(true)
            .truncate(true)
            .read(false)
            .write(true)
            .open(&dst_file)
            .unwrap();

        let options = SimpleFileOptions::default()
            .compression_method(CompressionMethod::Stored)
            .with_alignment(page_size.try_into().unwrap());
        let mut zip = ZipWriter::new(dst_file);
        let contents = read_file("/usr/bin/sleep").unwrap();
        let () = zip.start_file("sleep", options).unwrap();
        let _count = zip.write(&contents).unwrap();
    }

    {
        let dst_file = File::open(dst_file).unwrap();
        let mut zip = ZipArchive::new(dst_file).unwrap();
        let file = zip.by_index(0).unwrap();
        assert_eq!(file.name(), "sleep");
        assert_eq!(file.data_start(), page_size);
    }
}

fails with:

thread 'main' panicked at src/main.rs:41:9:
assertion `left == right` failed
  left: 4092
 right: 4096
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Similar code on 0.6.4 works fine:

use std::env;
use std::fs::read as read_file;
use std::fs::File;
use std::io::Write as _;
use std::path::Path;

use zip::write::FileOptions;
use zip::CompressionMethod;
use zip::ZipArchive;
use zip::ZipWriter;

fn main() {
    let page_size = 4096;
    let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap();
    let dst_dir = Path::new(&crate_dir);
    let dst_file = dst_dir.join("test.zip");

    {
        let dst_file = File::options()
            .create(true)
            .truncate(true)
            .read(false)
            .write(true)
            .open(&dst_file)
            .unwrap();

        let options = FileOptions::default().compression_method(CompressionMethod::Stored);
        let mut zip = ZipWriter::new(dst_file);
        let contents = read_file("/usr/bin/sleep").unwrap();
        let _align = zip
            .start_file_aligned(
                "sleep",
                options,
                page_size.try_into().unwrap(),
            )
            .unwrap();
        let _count = zip.write(&contents).unwrap();
    }

    {
        let dst_file = File::open(dst_file).unwrap();
        let mut zip = ZipArchive::new(dst_file).unwrap();
        let file = zip.by_index(0).unwrap();
        assert_eq!(file.name(), "sleep");
        assert_eq!(file.data_start(), page_size);
    }
}

Am I misunderstanding what is being aligned?

[d-e-s-o]

This exact test case was broken again with 2.1.4. Please reopen.

thread 'main' panicked at src/main.rs:41:9:
assertion `left == right` failed
  left: 35
 right: 4096
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I would bisect it, but your history is utterly broken.

The first bad commit could be any of:
3436a569ab1f76944f45f2dfa0eab5c8aaeec6c1
aa896ce9927f591850ab77fbc9146aa42fc550ec
7b2e31ca7fcff012d77f79bb3ada3a06bfa07d0c
bf3e7510a6a39dcafd5d99df5d954b8df72b2460
0d9fd0da0b25898c1f995e811bdedd8ed2025d79
7f641d23a32da92b0fc5203cff935baf45591785
0f0ef1dd960a5410f013160a946ac576b282fabb
9e12b6db0cb7958fb5abc4169101ff5757ee073c
0ad79e9ab3dccbcfc6112c0e4948f665478edc44
40d2da84f753be243795d2d03340aac6ddc1990b
19e5af84064b959f19377b88a0b7e6e768d81c7c
60f7c637e6b820b023fefa24442e67cb743b8a5a
32f946e78851615ac445579db427241cefdf5e7f

Would it be possible to have some kind of quality assurance by any chance?

[Pr0methean]

I added the method test_alignment() to write.rs in c3d9123, and a debug_assert to

zip2/src/write.rs

Line 784 in c3d9123

debug_assert_eq!(header_end % align, 0);

specifically so that they'd catch any regressions of this bug. (Unit regression tests are my go-to solution for ensuring that fixed bugs stay fixed.) I'll have to investigate how test_alignment() is different from your code, besides operating on memory rather than on a filesystem.

[d-e-s-o]

The following test reproduces it:

    #[test]
    fn test_alignment() {
        let page_size = 4096;
        let mut data = Vec::new();
        {
            let options = SimpleFileOptions::default()
                .compression_method(CompressionMethod::Stored)
                .with_alignment(page_size);
            let mut zip = ZipWriter::new(io::Cursor::new(&mut data));
            let contents = b"sleeping";
            let () = zip.start_file("sleep", options).unwrap();
            let _count = zip.write(&contents[..]).unwrap();
        }

        {
            let mut zip = ZipArchive::new(io::Cursor::new(&mut data)).unwrap();
            let file = zip.by_index(0).unwrap();
            assert_eq!(file.name(), "sleep");
            assert_eq!(file.data_start(), page_size.into());
        }
    }

The difference to the existing one is that it doesn't use finish_into_readable method. I don't know why, with that method, the bug isn't hit.

[Pr0methean]

Ah, this is actually a different bug: the data starts at byte 4096 as expected (I confirmed this by adding assert_eq!(data[4096..4104], b"sleeping"[..]); to the test case you provided), but data_start() incorrectly returns 35 when the archive is read back in. I'll look into adding tests that specifically compare writer.finish_into_readable()? with ZipArchive::new_append(writer.finish()?)?

[d-e-s-o]

Makes sense. But irrespective of that the zip archive created with 2.1.4 or more recent is still broken compared to previous versions. That is without any involvement of data_start().

Here are two archives created using zip with basically the above logic (no compression, page alignment etc.), using different versions of zip:
test-2.1.3.zip
test-2.1.5.zip

test-2.1.5.zip can't be unpacked properly:

$ unzip test-2.1.5.zip
Archive:  test-2.1.5.zip
 extracting: test-stable-addrs-stripped-elf-with-dwarf.bin   bad CRC 2886939d  (should be d9becfd9)
 extracting: zip-dir/test-no-debug.bin   bad CRC 239d2efa  (should be 0e0e8dc5)
 extracting: libtest-so.so            bad CRC ac70132d  (should be f886cd80)
 extracting: libtest-so-no-separate-code.so   bad CRC 82120180  (should be 33423096)

[Pr0methean]

That's yet another different issue: #195.

Sorry that the overall state of zip isn't better; I've had to work mostly alone providing bug fixes and regression tests. The long-term solution will be to increase the coverage of the fuzz tests, but I can't merge the PRs that do that (such as #220 and #221) until the bugs they uncover are fixed, without also indefinitely delaying the release of valuable PRs to users who aren't affected by those bugs. Even once those PRs are merged, it may take weeks of generating fuzz corpora from scratch with different length limits, merging them with the current ones, and iterating cargo fuzz run and cargo fuzz cmin to convergence (see the shell scripts in the root folder for how I'm doing that), before we have reliably good coverage; with changes in the code under test and without blank-slate restarts, the corpora have so far tended to get stuck on local optima with poor coverage. None of this is an excuse, but it's the cause of the problem I'm fighting and not always winning against.

The code also has some known quality issues, such as how data_start isn't consistently defined in the presence of certain extra fields in the header; but I inherited most of them when I took over the project.

[d-e-s-o]

Yeah, I understand. I am not blocked on this getting fixed as I've blacklisted 2.1.4 and higher and I don't really use the crate in production. That said, it is very annoying to have to deal with regressions like this on a 1.x or 2.x crate. Almost equally bad is not having a buildable commit history. Root causing issues like this should be trivial and instead we are running in circles for nothing.

That's yet another different issue: #195.

It's possible, but I kind of doubt it. The issue may manifest as checksum mismatches in my paste, but I am not convinced that is necessarily the actual issue. For one, #195 mentions version 2.1.3. This version is definitely behaving properly for my use case. Based on my limited testing, data offsets in the local file headers seemed off (though again, that may not be the actual root cause either, it is hard for me to tell).

If we compare the hex dumps there are a bunch of differences between the two, most notably towards the end. I am not fluent enough to judge validity of those differences at this point.

[Pr0methean]

I've mentioned my need for a co-admin on the relevant Slack channels at work; hopefully someone can either recommend one, or step up to the plate themselves and be at least as qualified as me.

[danielocfb]

That's yet another different issue: #195.

It's possible, but I kind of doubt it.

Also, I am not setting large_file(true).

[Pr0methean]

On my machine #221 fixes this, and #228 will be able to verify the fix after being rebased.

[d-e-s-o]

On my machine #221 fixes this, and #228 will be able to verify the fix after being rebased.

I can confirm that I no longer see issues at 6d8ab62. Thanks!