Eliminate resource leaks from zip::ZipFiles and

Eliminate resource leaks from zip::ZipFiles and
zip::ZipReader::OpenFromPlatformFile.

zip::ZipFiles and zip::ZipReader::OpenFromPlatformFile do not want to
take ownership of the passed-in file. On POSIX, dup the file descriptor
to pass to fopen, so that that we can safely fclose the result. On
Windows, stop closing the file handle. Fix consumers of these functions
that assumed ownership was being passed to instead close the file
themselves.

BUG=430959
Committed: https://crrev.com/b2eae6046c3aa484cc5019615f925a7bc178fa1b
Cr-Commit-Position: refs/heads/master@{#304930}

[jeremyspiegel, 2014-11-08 23:34:04]

jeremysspiegel@gmail.com changed reviewers:
+ jochen@chromium.org, noelutz@google.com, satorux@chromium.org

[jeremyspiegel, 2014-11-08 23:35:14]

jeremysspiegel@gmail.com changed reviewers:
+ jhawkins@chromium.org
- jochen@chromium.org

[jeremyspiegel, 2014-11-08 23:40:45]

Hi, this is my first contribution, so please forgive me for any mistakes in the
process. I have a test case that exposes one the issues by running the
problematic code in a loop, but that test takes a couple hundred milliseconds to
run so I wasn't sure if it made sense to include.

[satorux1, 2014-11-10 09:58:44]

Thank you for sending us the patch.I was out of the office today. I'll have a
look tomorrow.

[satorux1, 2014-11-12 07:04:44]

Thank you for sending us the fix!

> I have a test case that exposes one the issues by running the problematic code
in a loop, but that test takes a couple hundred milliseconds to
run so I wasn't sure if it made sense to include.

Could you add it but name it something like DISABLED_LeakDetectionTest so it
won't be run in the bots but developers can run manually?

https://codereview.chromium.org/683913009/diff/1/chrome/utility/chrome_conten...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/683913009/diff/1/chrome/utility/chrome_conten...
chrome/utility/chrome_content_utility_client.cc:252: base::File
dest_file(dest_fd);
This is needed to close the file descriptor, right?

maybe base::ScopedFD is clearer as its sole purpose is to close the file
descriptor, which we want here. :)

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
File third_party/zlib/google/zip_internal.cc (right):

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
third_party/zlib/google/zip_internal.cc:136: free(stream);
Good catch!

maybe add a comment:  // malloc'ed in HandleOpenFileFunc()

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
third_party/zlib/google/zip_reader_unittest.cc:183: FileWrapper
zip_fd_wrapper(test_zip_file_, FileWrapper::READ_ONLY);
does this have to be moved? otherwise, keep it-as-is?

[satorux1, 2014-11-12 07:05:09]

BTW, sorry for the late response. I was swamped yesterday.

[jeremyspiegel, 2014-11-12 08:51:46]

On 2014/11/12 07:04:44, satorux1 wrote:
> Thank you for sending us the fix!
> BTW, sorry for the late response. I was swamped yesterday.
No worries, I'm happy you were able to get to it today!
 
> Could you add it but name it something like DISABLED_LeakDetectionTest so it
> won't be run in the bots but developers can run manually?
Sure thing. I was also unsure of whether the iteration count needed would be
correct for other systems. On my OS X 10.10 system, it took 2557 iterations to
reproduce the issue. I'll include that and resubmit.

[jeremyspiegel, 2014-11-12 08:52:52]

https://codereview.chromium.org/683913009/diff/1/chrome/utility/chrome_conten...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/683913009/diff/1/chrome/utility/chrome_conten...
chrome/utility/chrome_content_utility_client.cc:252: base::File
dest_file(dest_fd);
On 2014/11/12 07:04:43, satorux1 wrote:
> This is needed to close the file descriptor, right?
> 
> maybe base::ScopedFD is clearer as its sole purpose is to close the file
> descriptor, which we want here. :)

Yes thanks, that is what I was looking for.

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
File third_party/zlib/google/zip_internal.cc (right):

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
third_party/zlib/google/zip_internal.cc:136: free(stream);
On 2014/11/12 07:04:43, satorux1 wrote:
> Good catch!
> 
> maybe add a comment:  // malloc'ed in HandleOpenFileFunc()

Sounds good.

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
third_party/zlib/google/zip_reader_unittest.cc:183: FileWrapper
zip_fd_wrapper(test_zip_file_, FileWrapper::READ_ONLY);
On 2014/11/12 07:04:43, satorux1 wrote:
> does this have to be moved? otherwise, keep it-as-is?

This doesn't have to be moved to work correctly, but since the ZipReader does
not take ownership of the file, I think it would be clearer to ensure the
FileWrapper outlives it. I think the unclear ownership semantics of
ZipReader::OpenFromPlatformFile may have led to one of the resource leaks, so in
addition to documenting it in the comment at the function declaration, I think
that changing the test case to be consistent would also help. If you'd prefer to
leave it as-is, that's fine too.

[satorux1, 2014-11-14 02:27:51]

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
third_party/zlib/google/zip_reader_unittest.cc:183: FileWrapper
zip_fd_wrapper(test_zip_file_, FileWrapper::READ_ONLY);
On 2014/11/12 08:52:51, jeremyspiegel wrote:
> On 2014/11/12 07:04:43, satorux1 wrote:
> > does this have to be moved? otherwise, keep it-as-is?
> 
> This doesn't have to be moved to work correctly, but since the ZipReader does
> not take ownership of the file, I think it would be clearer to ensure the
> FileWrapper outlives it. I think the unclear ownership semantics of
> ZipReader::OpenFromPlatformFile may have led to one of the resource leaks, so
in
> addition to documenting it in the comment at the function declaration, I think
> that changing the test case to be consistent would also help. If you'd prefer
to
> leave it as-is, that's fine too.

I see. There are many places in the file where ZipReader is created before
FileWrapper (ex. PlatformFileIteration). We can fix everywhere but I guess it's
fine to leave them as-is

https://codereview.chromium.org/683913009/diff/20001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/683913009/diff/20001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:252: base::ScopedFD
scoped_fd(dest_fd.fd);
Maybe name it fd_closer to make it even clearer?

BTW, I wanted to make sure that closing here was the right thing to do, and
found the documentation in ipc/ipc_message_util.h:

// The received file descriptor will have the |auto_close| flag set to true. The
      
// code which handles the message is responsible for taking ownership of it.    
      
// File descriptors are OS resources and must be closed when no longer needed.  
      


Maybe add a comment like

// dest_fd should be closed in the function. See ipc/ipc_message_util.h for
details.

[satorux1, 2014-11-14 02:28:44]

On 2014/11/12 08:51:46, jeremyspiegel wrote:
> On 2014/11/12 07:04:44, satorux1 wrote:
> > Thank you for sending us the fix!
> > BTW, sorry for the late response. I was swamped yesterday.
> No worries, I'm happy you were able to get to it today!
>  
> > Could you add it but name it something like DISABLED_LeakDetectionTest so it
> > won't be run in the bots but developers can run manually?
> Sure thing. I was also unsure of whether the iteration count needed would be
> correct for other systems. On my OS X 10.10 system, it took 2557 iterations to
> reproduce the issue. I'll include that and resubmit.

I think the number of iterations does not have to be precise. Maybe 100000 is
big enough for all platforms? :)

[jeremyspiegel, 2014-11-14 23:49:26]

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
File third_party/zlib/google/zip_reader_unittest.cc (right):

https://codereview.chromium.org/683913009/diff/1/third_party/zlib/google/zip_...
third_party/zlib/google/zip_reader_unittest.cc:183: FileWrapper
zip_fd_wrapper(test_zip_file_, FileWrapper::READ_ONLY);
> I see. There are many places in the file where ZipReader is created before
> FileWrapper (ex. PlatformFileIteration). We can fix everywhere but I guess
it's
> fine to leave them as-is

Ah I missed those. Sounds good, I'll leave this as-is.

https://codereview.chromium.org/683913009/diff/20001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/683913009/diff/20001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:252: base::ScopedFD
scoped_fd(dest_fd.fd);
On 2014/11/14 02:27:51, satorux1 wrote:
> Maybe name it fd_closer to make it even clearer?
> ...
> Maybe add a comment like
> 
> // dest_fd should be closed in the function. See ipc/ipc_message_util.h for
> details.

Sure, sounds good.

[jeremyspiegel, 2014-11-14 23:49:46]

> I think the number of iterations does not have to be precise. Maybe 100000 is
> big enough for all platforms? :)
OK, sounds good.

[satorux1, 2014-11-17 00:55:50]

third_party/zlib/google LGTM

noelutz@ and jhawkins@ please take a look at other files.

[James Hawkins, 2014-11-17 23:10:38]

https://codereview.chromium.org/683913009/diff/40001/third_party/zlib/google/...
File third_party/zlib/google/zip_internal.cc (right):

https://codereview.chromium.org/683913009/diff/40001/third_party/zlib/google/...
third_party/zlib/google/zip_internal.cc:135: int HandleCloseFileFunc(void*
opaque, void* stream) {
Can we provide better clarity about what the difference between CloseFileFunc
and HandleCloseFileFunc are?

[jeremyspiegel, 2014-11-17 23:13:50]

https://codereview.chromium.org/683913009/diff/40001/third_party/zlib/google/...
File third_party/zlib/google/zip_internal.cc (right):

https://codereview.chromium.org/683913009/diff/40001/third_party/zlib/google/...
third_party/zlib/google/zip_internal.cc:135: int HandleCloseFileFunc(void*
opaque, void* stream) {
On 2014/11/17 23:10:38, James Hawkins wrote:
> Can we provide better clarity about what the difference between CloseFileFunc
> and HandleCloseFileFunc are?

Would renaming CloseFileFunc to FdCloseFileFunc help? (The difference is that
CloseFileFunc is used on POSIX while HandleCloseFileFunc is used on Windows)

[James Hawkins, 2014-11-17 23:28:39]

https://codereview.chromium.org/683913009/diff/40001/third_party/zlib/google/...
File third_party/zlib/google/zip_internal.cc (right):

https://codereview.chromium.org/683913009/diff/40001/third_party/zlib/google/...
third_party/zlib/google/zip_internal.cc:135: int HandleCloseFileFunc(void*
opaque, void* stream) {
On 2014/11/17 23:13:50, jeremyspiegel wrote:
> On 2014/11/17 23:10:38, James Hawkins wrote:
> > Can we provide better clarity about what the difference between
CloseFileFunc
> > and HandleCloseFileFunc are?
> 
> Would renaming CloseFileFunc to FdCloseFileFunc help? (The difference is that
> CloseFileFunc is used on POSIX while HandleCloseFileFunc is used on Windows)

Yeah, I think so.  It'd also be good to document the ownership and lifecycles of
these objects (that you're changing).

[jeremyspiegel, 2014-11-17 23:34:58]

> It'd also be good to document the ownership and lifecycles of
> these objects (that you're changing).

I tried to do that in the declarations of zip::ZipFiles and
zip::ZipReader::OpenFromPlatformFile. I can also add that to zip_internal.cc.

[James Hawkins, 2014-11-18 00:02:13]

lgtm

[jeremyspiegel, 2014-11-18 00:10:19]

The CQ bit was checked by jeremysspiegel@gmail.com

[jeremyspiegel, 2014-11-18 00:10:23]

The CQ bit was unchecked by jeremysspiegel@gmail.com

[jeremyspiegel, 2014-11-19 06:35:12]

The CQ bit was checked by jeremysspiegel@gmail.com

[jeremyspiegel, 2014-11-19 06:35:15]

The CQ bit was unchecked by jeremysspiegel@gmail.com

[jeremyspiegel, 2014-11-19 06:35:16]

The CQ bit was checked by jeremysspiegel@gmail.com

[jeremyspiegel, 2014-11-19 06:35:16]

The CQ bit was unchecked by jeremysspiegel@gmail.com

[satorux1, 2014-11-19 07:40:12]

satorux@chromium.org changed reviewers:
+ shess@chromium.org

[satorux1, 2014-11-19 07:40:12]

+shess@ for chrome/common/safe_browsing/zip_analyzer.cc in case noelutz@ is out
of town

[satorux1, 2014-11-19 07:41:11]

satorux@chromium.org changed reviewers:
+ mattm@chromium.org

[satorux1, 2014-11-19 07:41:11]

oops. shess@ seems to be on leave. adding +mattm@ for
	chrome/common/safe_browsing/zip_analyzer.cc

[mattm, 2014-11-19 22:45:20]

safe_browsing lgtm

[jeremyspiegel, 2014-11-19 22:56:26]

The CQ bit was checked by jeremysspiegel@gmail.com

[commit-bot: I haz the power, 2014-11-19 22:58:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/683913009/100001

[commit-bot: I haz the power, 2014-11-19 23:53:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-19 23:54:14]

Patchset 6 (id:??) landed as
https://crrev.com/b2eae6046c3aa484cc5019615f925a7bc178fa1b
Cr-Commit-Position: refs/heads/master@{#304930}