Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for 0.18.1 #3

Closed
wants to merge 1 commit into from
Closed

Fix for 0.18.1 #3

wants to merge 1 commit into from

Conversation

genotrance
Copy link
Contributor

Using #head, needed this change along with minor changes to readStr() and peekStr() to work around ""[0] issue.

Tested with 0.18.0 as well as #head.

@dom96
Copy link
Owner

dom96 commented May 24, 2018

What problem does this solve? Ignoring errors like this usually leads to trouble.

@genotrance
Copy link
Contributor Author

When the filename field is blank - 101 blank characters, the filesize field = 0000000000000000000000 which fails the parseOctInt() function. This just catches that error.

I don't know why 0.18.0 handles this though.

@genotrance
Copy link
Contributor Author

Reason it fails is due to this commit:-

nim-lang/Nim@9b8603a#diff-d48a4a282bdbb4254c40b51e199e0a2cL1763

of '\0': break is no longer present and since header[124 .. 134] filesize is all nulled out for a blank filename, parseOctInt() is raising a ValueError.

@data-man
Copy link

I haven't checked in detail what kind of tar supports this library. And how packed a files.
Maybe this will help:
https://github.com/libarchive/libarchive/blob/master/libarchive/archive_read_support_format_tar.c#L54

@dom96
Copy link
Owner

dom96 commented May 25, 2018

So maybe the stdlib should be fixed, not untar?

@genotrance
Copy link
Contributor Author

@Araq explicitly removed null checks a month ago, I'll let him decide.

@data-man
Copy link

@genotrance genotrance closed this May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants