Page MenuHomeWildfire Games

Patch to solve ticket #5165 (Zip files with comments trip assertion on game startup)
Needs ReviewPublic

Authored by Teiresias on May 20 2018, 5:48 PM.

Details

Reviewers
vladislavbelov
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5165
Summary

The problem described in the ticket seems to be caused by the way archive_zip.cpp scans for the zip header in function LocateCentralDirectory():

  • The function first tests if the zip main directory is at EOF (normal case).
  • If it is not there due to a comment, it starts scanning the last 64Kb max of the file for the main directory using ScanforEcdr(). However, the latter is calling FindRecord(), which apparently expects to find the "structure to find" at the start of the area to search (see line 500: ENSURE(p == start);). If the comment is "too small" so FindRecord() won't find the central directory at the start of the scan area, the assertion will trip.

In this revision, I created a test case using the test.zip from the ticket to demonstrate the problem.

Manual work required:
Since SVN does not accept binary files in patch files, manually create the directory binaries/data/mods/_test.lib and place the test.zip file from the ticket into there - the test case is designed to work with that archive.

The patch also provides a modified archive_zip.cpp with a bugfix, so applying it will not show an issue in the test suite, but after applying the patch and reverting archive_zip.cpp it should trip the assertion.

Test Plan
  1. Apply the patch to a current 0AD checkout.
  2. Add to svn path binaries/data/mods/_test.lib/test.zip (to be committed)
  3. Revert archive_zip.cpp
  4. Build & run tests subproject: ./test TestArchiveZip Assertion should trip.
  5. Redo the former actions but leave archive_zip.cpp
  6. Build & run tests subproject ./test All tests should succeed.

Unit TestsFailed

TimeTest
0 msJenkins > cxxtest-debug.xml::[failed-to-read]
Failed to read test report file /Users/wfg/Jenkins/workspace/macos-differential/cxxtest-debug.xml org.dom4j.DocumentException: Error on line 1 of document : Content is not allowed in prolog. at org.dom4j.io.SAXReader.read(SAXReader.java:462)
0 msJenkins > TestAllocators::test_da
0 msJenkins > TestAllocators::test_da
0 msJenkins > TestAllocators::test_da
0 msJenkins > TestArchiveZip::test_scan_suspiciousZipFile
View Full Test Results (1 Failed · 994 Passed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Teiresias added a reviewer: Restricted Project.May 20 2018, 5:51 PM
Vulcan added a subscriber: Vulcan.May 20 2018, 5:52 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/532/display/redirect

Stan added a subscriber: Stan.May 20 2018, 6:19 PM

Hey thanks for working on this :)

Here are some comments. I can't review the whole thing but you can look at the coding conventions wiki page :) I'll try to lint the code to my limits ;)

source/lib/file/archive/archive_zip.cpp
537

++commentSize if you can't use a range for loop.

539

Static cast maybe unless you can refactor the thing not to have casts

546

Nullptr :)

564

Anyway to avoid the cast here ? :)

source/lib/file/archive/tests/test_archive_zip.h
2

Wow that's old :P bump the year

vladislavbelov requested changes to this revision.May 20 2018, 6:26 PM
vladislavbelov added a subscriber: vladislavbelov.

Tests should work automatically, without manual setup. So you need to create the archive by the test. And don't forget to remove it.

Thank you for the patch!

source/lib/file/archive/archive_zip.cpp
533

nullptr instead of NULL.

535

It looks like a code duplication, we have FindRecord function. And you just continue the search, if the commentSize != ecdr->getCommentLength() condition is invalid.

537

By CC the brace should be on new line, below too.

546

nullptr instead of NULL.

source/lib/file/archive/tests/test_archive_zip.h
2

Bump the year.

24

Empty line between system and our headers.

31

By CC the brace should be on new line, below too.

37

nullptr instead of NULL.

42

You don't have to mention delete if you don't have new, especially for shared_ptr.

This revision now requires changes to proceed.May 20 2018, 6:26 PM
vladislavbelov edited reviewers, added: Restricted Owners Package; removed: Restricted Project.May 20 2018, 7:46 PM
Teiresias edited the test plan for this revision. (Show Details)May 21 2018, 10:54 PM
vladislavbelov added a comment.EditedMay 21 2018, 11:35 PM

The file is too big for tests, I think we don't need to have the size bigger than 1KiB. Or you may generate it in the test. We have to avoid the storing of a big binary file in the version control systems.

Teiresias edited the test plan for this revision. (Show Details)May 26 2018, 9:22 PM
Teiresias updated this revision to Diff 6654.May 26 2018, 9:38 PM
Teiresias marked 11 inline comments as done.

Fixed most review issues:

  • Updated year in copyright declaration.
  • Adapted braces to CC
  • Changed NULL to nullptr
  • Added missing separator line between includes in test_archive_zip.h
  • Removed references to delete in test case
  • Reduced size of test.zip from 13Kbytes to 0,3KBytes

I have updated the patch to match the review items as far as I could to. Refactoring to get rid of the casts has been intentionally skipped for now since the remainder of the file uses the same style and the code has been working that ways for many years. I don't think it is worth to put this at stake.

I worry a bit about the FindRecord() function mentioned by vladislavbelov (see my comment there), since I do not fully understand the ENSURE bit - see my comment there.

source/lib/file/archive/archive_zip.cpp
535

I don't completely understand how the current FindRecord() works:
The for-loop runs up pointer p from start to (buf+size-recordSize), testing each position for the magic-id.
But the ENSURE(p == start) will fire an assertion if p != start, i.e. the magic-id is not at the very start of the buffer.

Regarding "continue the search": I will provide an update for this.

539

I tried to go for static_cast<const u32*>(), but at least gcc keeps giving me errors "invalid static_cast from type const u8* {aka const unsigned char*} to type const u32* {aka const unsigned int*}

I hesitate from refactoring the thing as this would go out of style of the rest of the file

564

I tried to, but found no easy way. UniqueRange.get() returns a "pointer" which is just a void* (see source/lib/allocators/unique_range.h).

Maybe one can rewrite this to get rid of the casts, but I don't dare at the moment, since most of the code has proven to work for a long time.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/579/display/redirect

Stan added a comment.May 27 2018, 11:11 AM

Thanks for fixing the comments. If you don't know we are currently investigating a few annoying bugs before a re release so don't worry if reviews are slow :) You can always fix something else in the meantime... Like the bugs with the archive builder ? :P Or the fact .deleted files do not work in packaged releases :)

source/lib/file/archive/archive_zip.cpp
539

Might be nice to figure out why we need to cast to such a smaller size and if everything cannot be changed so that cast is not needed :)

Maybe another diff :)

Could you create a patch with context: https://trac.wildfiregames.com/wiki/SubmittingPatches#Makingsomechanges? It helps a lot when you're reviewing, thank you. Currently it's harder to me to review a logic without context.

source/lib/file/archive/archive_zip.cpp
1

2019.

source/lib/file/archive/tests/test_archive_zip.h
2

2019.

34

std::string isn't required to be initialised - it has default constructor. Also we use lowerCamelCase: resultBuffer.

50
vladislavbelov requested changes to this revision.Apr 2 2019, 4:38 PM
This revision now requires changes to proceed.Apr 2 2019, 4:38 PM
Teiresias updated this revision to Diff 8277.Jun 1 2019, 10:18 PM

Updated diff file to contain full context (using command svn diff --diff-cmd diff -x "-U 99999")

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/1582/display/redirect

I uploaded a new .patch file which - checked using local text editor - contains the full context of archive_zip.cpp.
However, using the "Download Raw Diff" button on the topright menu bar it seems that still only the "changed areas" are exported, although Phabricator accepted the new .patch file (Copyright changed from 2018 to 2019).

Stan added a comment.Jun 1 2019, 10:30 PM

Hey @Teiresias, long time no see.
Some style comments. I guess you can ignore the static_cast though it's part of our coding conventions.

source/lib/file/archive/archive_zip.cpp
274

static_cast<off_t>(expr)

536

Any reason for the new lines ?

source/lib/file/archive/tests/test_archive_zip.h
50

Why is there no parameter name ?

Stan added a comment.Sep 24 2019, 4:30 PM

@Teiresias Will you finish the patch ?

I am sorry, I lost track of this ticket for health reasons. Regarding your questions, it seems I was so occupied by the zip archive problem that I neglected styleguide and common appearance. Here are attempts to answer your observations:

  1. Regarding static_cast<off_t>(expr): I admit I never got around with the various kinds of casts offered by C++ (when I design SW on my own, I try and avoid casting alltogether). When updating the patch, I can still put it in.
  2. Assuming the newlines refer to the for-loop whose three parts have been put on three lines, I created the patch on a BSD machine which usually runs in text mode (80x25). For me it was easier to write the code in this way so I did not encounter line-wrapping. I normally use vim for editing.
  3. "Why is there no parameter name ?": I am used to name only those parameters whose values are actually used in the function body, and leave those parameters anonymous which are unused and supplied only to match a given interface. This usually avoids an unused parameter warning as well. Since the ArchiveEntryCallback function needs the first parameter (path) only but has to provide all four parameters to comply with the callback interface, I left the other three parameters anonymous. Is this a nuisance?

I can and will update the patch but would like a comment from yours on behalf of 2) and 3) first, so I can design the patch in proper style. I will then put the amphersands to the correct place too, as pers vladislavbelovs comment in the test file (line 50).

Stan added a comment.Oct 5 2019, 10:26 PM

Regarding static_cast<off_t>(expr): I admit I never got around with the various kinds of casts offered by C++ (when I design SW on my own, I try and avoid casting alltogether). When updating the patch, I can still put it in.

Basically using C casts run all the C++ casts until it finds something that works. So it's better to use the good type directly. dynamic_cast can slow a lot the code. See the recent removal of virtual inheritance.

Assuming the newlines refer to the for-loop whose three parts have been put on three lines, I created the patch on a BSD machine which usually runs in text mode (80x25). For me it was easier to write the code in this way so I did not encounter line-wrapping. I normally use vim for editing.

https://trac.wildfiregames.com/wiki/Coding_Conventions says 80 120 or 132 are okay, so I guess follow whatever is done that file :)

"Why is there no parameter name ?": I am used to name only those parameters whose values are actually used in the function body, and leave those parameters anonymous which are unused and supplied only to match a given interface. This usually avoids an unused parameter warning as well. Since the ArchiveEntryCallback function needs the first parameter (path) only but has to provide all four parameters to comply with the callback interface, I left the other three parameters anonymous. Is this a nuisance?

We use a macro called UNUSED for this Example

void* debug_GetCaller(void* UNUSED(context), const wchar_t* UNUSED(lastFuncToSkip))
{
    return NULL;
}

Thanks for keeping working on this. Hope you'll get better.

Teiresias updated this revision to Diff 10165.Oct 16 2019, 10:10 PM

Updated patch according to Stans requests

  1. static cast inserted as per suggestion
  2. for loop content has been condensed into one LOC (but prototype in test header still spanned over multiple lines as it would grow to approx. 150 chars as a one-liner)
  3. UNUSED()-macro applied to callback parameters in test

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/462/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/977/display/redirect

Teiresias updated this revision to Diff 10166.Oct 16 2019, 10:13 PM

Minor second update to get parameter references to match CCs

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/463/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/978/display/redirect

Imarok added a subscriber: Imarok.Oct 17 2019, 9:05 AM
In D1511#99249, @Vulcan wrote:

Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/978/display/redirect

Have you seen that our CI-Bot failed with the patch?
That is in most cases a sign that there is an error in your patch. :)

Stan added a comment.Oct 17 2019, 9:33 AM

Segfault in tests for the record

Teiresias added a comment.EditedDec 27 2019, 10:20 PM

I cannot comment on the segfault, as I do not have a stacktrace available. Here is what I can say by now:

  1. I recently applied the patch to an svn workspace rev. 23254, rebuilt and the test passed (release built on OpenSuse 15/Leap) - with the zip archive mentioned manually copied into place.
  2. Since I do not have arcanist available, I created a diff for the code patches and uploaded. However, this patch also requires a binary file (.zip) to be placed below binaries/data/... . The binary cannot be included in the svn patch (limitation of svn and unlikely to ever get removed, see discussion at http://subversion.1072662.n5.nabble.com/Create-Apply-Patch-UTF-16-and-binary-support-td181079.html), so I presume CI test run will always fail, unless the zip somehow gets inserted into the svn trunk.

I admit I am at a loss on how to proceed here.

source/lib/file/archive/tests/test_archive_zip.h
50

Fixed in diff update. Normally, I use parameter names only when a parameter is actually used in a function body and keep pro-forma params anonymous.

Stan added a comment.EditedDec 27 2019, 10:21 PM

You can upload the zip file here, if you haven't already. Maybe it could be generated by the test? We have some python scripts that generate bogus dae files.

Teiresias updated this revision to Diff 10836.Jan 1 2020, 8:22 PM

Unit test case now writes the binary sample test.zip to the _test.lib directory tree on its own.
This way CI can pass although SVN patches cannot include binary files (suggested by Stan).

Vulcan added a comment.Jan 1 2020, 8:25 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/920/display/redirect

Vulcan added a comment.Jan 1 2020, 8:25 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/15/display/redirect

Vulcan added a comment.Jan 1 2020, 8:28 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1438/display/redirect

Stan added a comment.Jan 1 2020, 9:00 PM

Strange that it failed on macos... Any reason while the file would not have been created?

No, but I can give it a try on FreeBSD (closely related to MacOS). Further, I have trouble decoding the error:

+ binaries/system/test_dbg
Debugger launch failed: No such file or directory
/Users/wfg/Jenkins/workspace/macos-differential@tmp/durable-6d9cd9e9/script.sh: line 1: 27080 Trace/BPT trap: 5 binaries/system/test_dbg > cxxtest-debug.xml

I fail to see which file it complains about. When I read "Debugger launch failed" my first thought is that test_dbg would not be run at all. OTOH, cxxtest-debug.xml seems to have been created according to step five ("Archive JUnit-formatted test results"). Can I obtain a copy of that file? "Artefacts" provides a download link for pipeline.log only.

Stan added a comment.Jan 2 2020, 11:05 AM

My bad it fails because of the following reason.

unique_range.cpp(72): Assertion failed: "idxDeleter < numDeleters"
Assertion failed: "idxDeleter < numDeleters"
Location: unique_range.cpp:72 (CallUniqueRangeDeleter)

You can see it in the section just below the cxxtest-debug.xml session.
https://jenkins.wildfiregames.com/blue/organizations/jenkins/macos-differential/detail/macos-differential/15/pipeline

Stan added inline comments.Jan 2 2020, 11:28 AM
source/lib/file/archive/archive_zip.cpp
538

static_cast

543

static_cast or ecdr_magic?

source/lib/file/archive/tests/test_archive_zip.h
74

Extra ' '

Teiresias added inline comments.Jan 2 2020, 7:38 PM
source/lib/file/archive/archive_zip.cpp
538

static_cast<const u32*>(pECDRTest) gives a compiler error "invalid cast from type const u8* (aka unsigned char) to const u32*...". reinterpret_cast or classic C-style-cast, as currently used, will pass. Please state your preference.

I checked the coding conventions for that and all I found about casting is "Don't use RTTI (dynamic_cast etc). " Is there another CC doc?

543

static_cast leads to a compiler error "invalid cast from const u8* to const ECDF*".
reinterpret_cast or classic C-style casts work.

As for the "or ecdr_magic" alternative, I'm not sure. Do you mean something like "ecdr = &ecdr_magic;"? That does not fit, since ecdr has to grab the position of the whole zip structure within the file content, not only the magic-id. Lateron we do ecdr->Decompose(...) on that.

source/lib/file/archive/tests/test_archive_zip.h
74

Will remove the extra space

Stan added inline comments.Jan 2 2020, 7:52 PM
source/lib/file/archive/archive_zip.cpp
538

We prefer explicit C++ casts, static_cast where possible, reinterpret_cast when necessary, dynamic_cast where there is no choice.

So in this case I'd go with reinterpret_cast if that works :)

543

Same here for the cast.

Oh, I did not notice it was a pointer :/ Just wanted to avoid the cast. Maybe there could be a temporary variable storing the reinterpreted value.