when parsing base256 encoded sizes, include all 12 bits

mafintosh / tar-stream

tar-stream is a streaming tar parser and generator.

MIT License

413 stars 93 forks source link

when parsing base256 encoded sizes, include all 12 bits #62

Closed bobzoller closed 7 years ago

bobzoller commented 7 years ago

Turns out it was a simple typo -- the size field is 12 bits long, not 8.

fixes #61

piranna commented 7 years ago

Tests are failling... also, could you be able to add a test for your use case?

bobzoller commented 7 years ago

tests should pass this time -- more complete fix too. I'll see about adding a test, but offhand it seems how they're written it would take me committing a 10GB fixture(?)

bobzoller commented 7 years ago

yeah... test wise I'm stuck unless you want to suggest something:

a small file with a header forced into base256 encoding wouldn't have caught this bug
I could commit/reference a 10GB fixture
I could try to trim the header off a large tar, leaving a fixture we could parse but an invalid tarfile

mafintosh commented 7 years ago

Nice catch. I'll take a look at this tmw so we can get it merged.

piranna commented 7 years ago

a small file with a header forced into base256 encoding wouldn't have caught this bug

Why not? According to the code it would be feasable...

I could commit/reference a 10GB fixture

Overkill, but maybe you could craft it by replacing the file stream for one that generated on the fly 10GB of data, so there's no need to store it.

I could try to trim the header off a large tar, leaving a fixture we could parse but an invalid tarfile

That would be feasable too, we are only interested on testing the header and not the tarfile itself, so the test can be able to catch and swallow the exception. I would implement the three tests, seems they can show different use cases :-)

kanongil commented 7 years ago

Can this get merged? I need large file support for my project.

Also, it is perfectly possible to craft a small and valid test fixture for this, where eg. the file size is encoded as base-256.

piranna commented 7 years ago

@kanongil, could you be able to provide the test?

kanongil commented 7 years ago

@piranna I have made a PR #67 with a test containing a handcrafted file with a base-256 file size.

I have verified that it decompresses correctly using macOS tar client.

bobzoller commented 7 years ago

thanks @kanongil

kanongil commented 7 years ago

Ping?

kanongil commented 7 years ago

Anything else I can do to speed this along?

mafintosh commented 7 years ago

1.5.3 (including the test)