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

Change TarArchive.ExtractContent() to dispose of output streams in case of an exception #326

Merged
merged 2 commits into from Jun 18, 2019

Conversation

Projects
None yet
2 participants
@Numpsy
Copy link
Contributor

commented Feb 21, 2019

refs #321 - change TarArchive.ExtractEntry to dispose of output file streams in a finally block, in case an exception occurs.
(pretty simple change, but it looks like changing the indentation makes the diff viewer make the change look greater than it is).

Also adds a unit test that failed previously and passes now.

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@piksel
Copy link
Collaborator

left a comment

I would prefer if using() { } blocks where used instead. Is there any reason for not using them?

@Numpsy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

I think at the time I was just trying to make the minimal change to fix the issue.

Possibly the using approach has issues with the way the StreamWriter is only optionally created, but when it is it takes ownership of the output stream such that you don't have to dispose both?
Would have to have another look, perhaps it could do with being refactored to improve that.

@piksel

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2019

StreamWriter has a leaveOpen parameter, but you have to specify all four parameters:

public StreamWriter (System.IO.Stream stream, System.Text.Encoding encoding, int bufferSize, bool leaveOpen);

Even if the change is small, it's hard to tell what has changed due to the scope change (as you said).

@Numpsy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

Hmm, I didn't pay too much attention to what this function was actually doing when I looked at it before, but on a closer look it seems a bit wrong in general. I mean,

  1. The IsBinary check @

    asciiTrans = !IsBinary(destFile);
    looks like it's trying to check if the output file is binary, rather than the input data?

  2. What's the loop @

    for (int off = 0, b = 0; b < numRead; ++b)
    going to do if there aren't any new lines in the buffer it's going through?

@piksel

This comment has been minimized.

Copy link
Collaborator

commented Jun 16, 2019

I think what it's trying to do is mimic fopen() without the b mode flag. Basically translating line endings to the target operating systems preferred ones.
They probably copied the isBinary from the archive creation code where it makes much more sense. This test should always return false since it tries to find "binary" data in truncated file. Either way it's wrong.

The loop would just not write anything to the output if no newlines are encountered. Which also means that the last line would always be omitted.
Instead of this code, simply using StreamReader.ReadLine() followed by StreamWriter.WriteLine() would replace the line endings.

@Numpsy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

that seems more reasonable (was also thinking that the existing code will have issues if the input already has Windows line endings, and that would fix that as well).

This is going on a bit of a tangent though - i'll refactor the stream disposal to use usings, and then see about a seperate issue for the rest.

@Numpsy Numpsy force-pushed the Numpsy:partial_tar_gzip_handling branch from 84e7ea4 to d151908 Jun 16, 2019

@Numpsy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

I've changed it to use using blocks and also to just use TarInputStream.CopyEntryContents in the non-translate cases.
I haven't made any changes to the binary detection or line handling in this set - will open a seprate issue about that.

@Numpsy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Actually, I wonder if the usage of IsBinary there is actually the cause of #27, where trying to open the file for read while it's already open for create/write caues the exception?

@piksel

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

Good catch! That actually also answers the question "how did this ever work?". It's probably never even turned on.
Since the write file handle is always acquired (without Read sharing permitted) this would always fail (on windows).

@piksel

piksel approved these changes Jun 18, 2019

Copy link
Collaborator

left a comment

This does have some quirks, but it's a lot better than before. The whole ASCII translation needs to be revisited in another issue anyway.

@piksel piksel merged commit 6f245b3 into icsharpcode:master Jun 18, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@Numpsy Numpsy deleted the Numpsy:partial_tar_gzip_handling branch Jun 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.