Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/9986 )
Change subject: IMPALA-3307: Add support for IANA time-zone db ...................................................................... Patch Set 14: Code-Review+2 (5 comments) Thanks for thinking about Zip-Slip! I have left a few optional comments about the usability of the interfaces. http://gerrit.cloudera.org:8080/#/c/9986/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9986/14//COMMIT_MSG@34 PS14, Line 34: - Introduces a new startup flag (--hdfs_zone_info_zip) to impalad to The Zip slip safe zip-util could be also mentioned in the commit message. http://gerrit.cloudera.org:8080/#/c/9986/14/be/src/util/filesystem-util.h File be/src/util/filesystem-util.h: http://gerrit.cloudera.org:8080/#/c/9986/14/be/src/util/filesystem-util.h@92 PS14, Line 92: Directory(const string& path, bool skip_hidden_entries = true); I thought a bit about usability and I vote for removing this parameter and skip only "." and ".." - I can't imagine any use case when I would be interested in those. http://gerrit.cloudera.org:8080/#/c/9986/14/be/src/util/filesystem-util.h@109 PS14, Line 109: static Status GetEntryNames(const string& path, I would prefer max_result_size to be the last parameter, and give it a default value of 0. http://gerrit.cloudera.org:8080/#/c/9986/14/be/src/util/zip-util-test.cc File be/src/util/zip-util-test.cc: http://gerrit.cloudera.org:8080/#/c/9986/14/be/src/util/zip-util-test.cc@69 PS14, Line 69: EXPECT_FALSE(filesystem::exists(dest_dir3)); I guess that this is only true if zip decoding failed at the start, and some files may be already decompressed before reaching an error in the zip. I am not sure what to do with this, probably nothing. It would be possible add some kind of cleanup logic to the java util, but I am not sure if this worth the effort. http://gerrit.cloudera.org:8080/#/c/9986/14/fe/src/main/java/org/apache/impala/util/ZipUtil.java File fe/src/main/java/org/apache/impala/util/ZipUtil.java: http://gerrit.cloudera.org:8080/#/c/9986/14/fe/src/main/java/org/apache/impala/util/ZipUtil.java@45 PS14, Line 45: try (ZipFile zip = new ZipFile(params.archive_file)) { I would move this block to a similar function with (String archiveFile, String destDir) parameters to make this util usable from Java too. This would be minimal extra effort and I think that it can be handy to have an easily usable Zip-Slip safe extract function. -- To view, visit http://gerrit.cloudera.org:8080/9986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I93c1fbffe81f067919706e30db0a34d0e58e7e77 Gerrit-Change-Number: 9986 Gerrit-PatchSet: 14 Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 08 Jun 2018 16:15:15 +0000 Gerrit-HasComments: Yes
