zip: Add ZipReader and rework Unzip() using the new class.

zip: Add ZipReader and rework Unzip() using the new class.

ZipReader implementation is based on ExtractCurrentFile() in zip.cc.
The basic logic is not changed, but the code is simplified.

BUG=chromium-os:22351
TEST=out/Release/unit_tests --gtest_filter=Zip*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109655

[Aaron Boodman, 2011-11-09 08:00:39]

nice

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc
File chrome/common/zip_reader.cc (right):

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:31: // Check the file name here for directory
traversal issues. In the name of
Nit: add blank line before this one.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:38: &file_name_utf16))
If a conditional wraps, it should use curly braces.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:55: // Don't forget to update Close() when you add
new code in the constructor.
Instead of comments like this, it is better to have a Reset() private method
that both Close() and the constructor call that centralizes member
initialization.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:77: unz_global_info zip_info = {};  // Zero-clear.
Unnecessary comment.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h
File chrome/common/zip_reader.h (right):

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:36: class ZipReader {
Nice API.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:44: // Returns the file path.
It would be nice to indicate whether they are prefixed with '/' for those who
are unfamiliar with the intricacies of zip.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:112: bool ExtractCurrentFileToFile(const FilePath&
output_file_path);
Throughout the API, consider using the term 'entry' instead of 'file' for the
items inside the zip. I think that makes it more clear that it might represent a
directory or a file.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:122: bool ExtractCurrentFileToDirectory(const
FilePath& output_directory_path);
Maybe "ExtractCurrentEntryIntoDirectory" ("into" instead of "to") to try and
make clear that you're not transforming the current entry into a directory ...
but just placing it into one.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:138: scoped_ptr<FileInfo> current_file_info_;
Call the DISALLOW_COPY_AND_ASSIGN macro from basictypes.h here.

[satorux1, 2011-11-10 08:00:56]

Thank you for the great comments!

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc
File chrome/common/zip_reader.cc (right):

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:31: // Check the file name here for directory
traversal issues. In the name of
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> Nit: add blank line before this one.

Done.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:31: // Check the file name here for directory
traversal issues. In the name of
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> Nit: add blank line before this one.

Done.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:38: &file_name_utf16))
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> If a conditional wraps, it should use curly braces.

Done.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:38: &file_name_utf16))
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> If a conditional wraps, it should use curly braces.

Done.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:55: // Don't forget to update Close() when you add
new code in the constructor.
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> Instead of comments like this, it is better to have a Reset() private method
> that both Close() and the constructor call that centralizes member
> initialization.

You are right. Done.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.cc#new...
chrome/common/zip_reader.cc:77: unz_global_info zip_info = {};  // Zero-clear.
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> Unnecessary comment.

Done.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h
File chrome/common/zip_reader.h (right):

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:36: class ZipReader {
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> Nice API.

Thanks!

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:44: // Returns the file path.
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> It would be nice to indicate whether they are prefixed with '/' for those who
> are unfamiliar with the intricacies of zip.

Done. This comment made me to add a new evil zip file that contains file names
starting with /, and a test for it.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:112: bool ExtractCurrentFileToFile(const FilePath&
output_file_path);
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> Throughout the API, consider using the term 'entry' instead of 'file' for the
> items inside the zip. I think that makes it more clear that it might represent
a
> directory or a file.

Good idea. Changed accordingly.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:122: bool ExtractCurrentFileToDirectory(const
FilePath& output_directory_path);
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> Maybe "ExtractCurrentEntryIntoDirectory" ("into" instead of "to") to try and
> make clear that you're not transforming the current entry into a directory ...
> but just placing it into one.

Done.

http://codereview.chromium.org/8508003/diff/1/chrome/common/zip_reader.h#newc...
chrome/common/zip_reader.h:138: scoped_ptr<FileInfo> current_file_info_;
On 2011/11/09 08:00:40, Aaron Boodman wrote:
> Call the DISALLOW_COPY_AND_ASSIGN macro from basictypes.h here.

Good catch.  Added.

[Aaron Boodman, 2011-11-10 08:13:05]

LGTM w/ one question (you don't have to wait for another review from me)

http://codereview.chromium.org/8508003/diff/2002/chrome/common/zip_reader.h
File chrome/common/zip_reader.h (right):

http://codereview.chromium.org/8508003/diff/2002/chrome/common/zip_reader.h#n...
chrome/common/zip_reader.h:47: // "foo/bar.txt", but if it's absolute,
is_unsafe() returns true.
Is it legit for zip files to have paths starting with '/'? If so, can you just
strip the leading '/' if it is present so that callers don't have a chance to
mess it up?

[satorux1, 2011-11-10 17:42:26]

http://codereview.chromium.org/8508003/diff/2002/chrome/common/zip_reader.h
File chrome/common/zip_reader.h (right):

http://codereview.chromium.org/8508003/diff/2002/chrome/common/zip_reader.h#n...
chrome/common/zip_reader.h:47: // "foo/bar.txt", but if it's absolute,
is_unsafe() returns true.
On 2011/11/10 08:13:05, Aaron Boodman wrote:
> Is it legit for zip files to have paths starting with '/'? If so, can you just
> strip the leading '/' if it is present so that callers don't have a chance to
> mess it up?

Good question. Per the spec, it seems to legitimate (at least not prohibited).
unzip command on Linux strips the leading '/' automatically. 

I thought about stripping the leading '/' but chose not to for now, as it would
lose the original file names.

Maybe we could return the sanitized path name from file_path(), and provide
original_file_path() that returns the original unmodified file path.

I'll rethink about the file path API, when adding support for non-UTF-8 file
names, and Unicode Path Extra field.

[Aaron Boodman, 2011-11-10 19:37:38]

On 2011/11/10 17:42:26, satorux1 wrote:
> Maybe we could return the sanitized path name from file_path(), and provide
> original_file_path() that returns the original unmodified file path.

That makes sense.

> I'll rethink about the file path API, when adding support for non-UTF-8 file
> names, and Unicode Path Extra field.

sgtm

[Aaron Boodman, 2011-11-10 19:38:06]

I think I need to say "LGTM" again, for the commit queue.

[satorux1, 2011-11-10 23:24:21]

On 2011/11/10 19:38:06, Aaron Boodman wrote:
> I think I need to say "LGTM" again, for the commit queue.

In the patch set 4, I've fixed test failures on Linux and Windows.

On Linux, ZipReaderTest.current_entry_info_InvalidUTF8File was failing as
FilePath internally did some conversion over the file name string, which
contained an invalid UTF-8 character.

On Windows, ExtensionServiceTest was crashing as good.crx contained invalid time
stamp data as follows (1980-00-00), and base::Time::FromLocalExploded() aborted
by a check failure.


% unzip -l chrome/test/data/extensions/good.crx
Archive:  chrome/test/data/extensions/good.crx
warning [chrome/test/data/extensions/good.crx]:  306 extra bytes at beginning or
within zipfile
  (attempting to process anyway)
  Length      Date    Time    Name
---------  ---------- -----   ----
      365  1980-00-00 00:00   manifest.json
       11  1980-00-00 00:00   script1.js
       11  1980-00-00 00:00   script2.js
---------                     -------
      387                     3 files

[Aaron Boodman, 2011-11-11 17:12:35]

lgtm

[commit-bot: I haz the power, 2011-11-11 17:20:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satorux@chromium.org/8508003/11018

[commit-bot: I haz the power, 2011-11-11 18:50:04]

Change committed as 109655