Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(87)

Issue 222243003: Fixed uncompressing files with wrong uncompressed size set. (Closed)

Created:
6 years, 8 months ago by João Eiras
Modified:
6 years, 7 months ago
Reviewers:
satorux1, gavinp, agl, hshi1
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Fixed uncompressing files with wrong uncompressed size set. A zip file carries some metadata for each archived file, including the total uncompressed size. If that size was incorrect, therefore the compressed file being different in size when unpacking, the minizip code would fail with a CRC error. Every other zip utility handles these files, so should the minizip code for safety sake. BUG=359516 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268940

Patch Set 1 #

Total comments: 2

Patch Set 2 : review comments #

Total comments: 6

Patch Set 3 : review issues #

Total comments: 3

Patch Set 4 : white line #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -5 lines) Patch
M third_party/zlib/contrib/minizip/unzip.c View 1 chunk +0 lines, -5 lines 0 comments Download
A third_party/zlib/google/test/data/test_mismatch_size.zip View Binary file 0 comments Download
M third_party/zlib/google/zip_reader.h View 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/zlib/google/zip_unittest.cc View 1 2 3 2 chunks +29 lines, -0 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
João Eiras
So, first fix here (for reference, original review https://codereview.chromium.org/179963002/ )
6 years, 8 months ago (2014-04-02 15:04:48 UTC) #1
satorux1
Could you file a bug on crbug.com and add BUG=<bugid> ? A bug fix should ...
6 years, 8 months ago (2014-04-03 04:59:03 UTC) #2
João Eiras
Hi agl and gavinp. I hope this commit message is enough content for you to ...
6 years, 8 months ago (2014-04-03 11:19:39 UTC) #3
agl
https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/zip_unittest.cc File third_party/zlib/google/zip_unittest.cc (right): https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/zip_unittest.cc#newcode326 third_party/zlib/google/zip_unittest.cc:326: SCOPED_TRACE(base::StringPrintf("<loop:%d>", static_cast<int>(i))); |i| is an int, but you're static_casting ...
6 years, 8 months ago (2014-04-03 14:34:56 UTC) #4
satorux1
I just recalled that allowing wrong uncompressed size was dangerous. As described in crbug.com/268243 a ...
6 years, 8 months ago (2014-04-04 05:31:30 UTC) #5
João Eiras
On 2014/04/04 05:31:30, satorux1 wrote: > I just recalled that allowing wrong uncompressed size was ...
6 years, 8 months ago (2014-04-04 11:51:58 UTC) #6
satorux1
https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/zip_unittest.cc File third_party/zlib/google/zip_unittest.cc (right): https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/zip_unittest.cc#newcode326 third_party/zlib/google/zip_unittest.cc:326: SCOPED_TRACE(base::StringPrintf("<loop:%d>", static_cast<int>(i))); On 2014/04/03 14:34:56, agl wrote: > |i| ...
6 years, 7 months ago (2014-04-30 00:18:19 UTC) #7
satorux1
On 2014/04/04 11:51:58, João Eiras wrote: > On 2014/04/04 05:31:30, satorux1 wrote: > > I ...
6 years, 7 months ago (2014-04-30 00:20:09 UTC) #8
João Eiras
https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/zip_unittest.cc File third_party/zlib/google/zip_unittest.cc (right): https://codereview.chromium.org/222243003/diff/20001/third_party/zlib/google/zip_unittest.cc#newcode326 third_party/zlib/google/zip_unittest.cc:326: SCOPED_TRACE(base::StringPrintf("<loop:%d>", static_cast<int>(i))); On 2014/04/30 00:18:19, satorux1 wrote: > On ...
6 years, 7 months ago (2014-05-07 12:03:50 UTC) #9
gavinp
Overall this lgtm. This bug exists in zlib 1.2.8, the currently released version. Shouldn't we ...
6 years, 7 months ago (2014-05-07 14:01:00 UTC) #10
João Eiras
On 2014/05/07 14:01:00, gavinp wrote: > Overall this lgtm. > > This bug exists in ...
6 years, 7 months ago (2014-05-07 15:31:10 UTC) #11
João Eiras
https://codereview.chromium.org/222243003/diff/40001/third_party/zlib/google/zip_unittest.cc File third_party/zlib/google/zip_unittest.cc (right): https://codereview.chromium.org/222243003/diff/40001/third_party/zlib/google/zip_unittest.cc#newcode319 third_party/zlib/google/zip_unittest.cc:319: const base::FilePath& temp_dir = scoped_temp_dir.path(); On 2014/05/07 14:01:01, gavinp ...
6 years, 7 months ago (2014-05-07 15:31:20 UTC) #12
João Eiras
The CQ bit was checked by joaoe@opera.com
6 years, 7 months ago (2014-05-07 15:36:17 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaoe@opera.com/222243003/60001
6 years, 7 months ago (2014-05-07 15:41:03 UTC) #14
commit-bot: I haz the power
6 years, 7 months ago (2014-05-07 20:53:07 UTC) #15
Message was sent while issue was closed.
Change committed as 268940

Powered by Google App Engine
This is Rietveld 408576698