YACReaderLibraryServer - Problems with patching p7zip

[xthursdayx]

As I mentioned in my comment on issue https://github.com/YACReader/yacreader/issues/259 I maintain a dockerized version of YACReaderLibraryServer. My previous version was based on josetesan's Debian-based docker image, however, I ended up starting over from scratch in order to create a Docker image with a bit more flexibility in terms of directory and file permissions and one that doesn't run YACReaderLibraryServer as root. My new image is based on Ubuntu Bionic and uses S6 overlay to manage permissions and start YACReaderLibraryServer. I also chose to compile YACReaderLibraryServer with p7zip as the decompression backend to avoid the 7z errors I was seeing with my previous image, which used the default unarr backend.

So far my new Docker image seems to be working well, however when building the image I see the following patch error:

patching file libp7zip/CPP/myWindows/StdAfx.h
patching file libp7zip/CPP/7zip/Archive/7z/7zIn.cpp
patching file libp7zip/CPP/7zip/Compress/ShrinkDecoder.cpp
Hunk #1 FAILED at 121.
1 out of 1 hunk FAILED
-- saving rejects to file libp7zip/CPP/7zip/Compress/ShrinkDecoder.cpp.rej

I checked libp7zip.patch and saw that the hunk that is failing is CVE-2017-17969:

CVE-2017-17969
--- libp7zip/CPP/7zip/Compress/ShrinkDecoder.cpp
+++ libp7zip/CPP/7zip/Compress/ShrinkDecoder.cpp
@@ -121,8 +121,13 @@
     {
       _stack[i++] = _suffixes[cur];
       cur = _parents[cur];
+      if (cur >= kNumItems || i >= kNumItems)
+        break;
     }
-
+
+    if (cur >= kNumItems || i >= kNumItems)
+      break;
+
     _stack[i++] = (Byte)cur;
     lastChar2 = (Byte)cur;

Based on issue https://github.com/YACReader/yacreader/issues/246 and PR https://github.com/YACReader/yacreader/pull/249 I see that you all don't tend to use this configuration (p7zip on Linux) and consider it "pretty much deprecated", but I was wondering if you might know what is causing this patch to fail?

Thanks for any help or ideas!

[selmf]

The first thing you should verify is if it actually works when you use the official p7zip code instead of a third party github mirror. I think this page has the correct one: p7zip

The 7z errors you see in the unarr builds are actually harmless. They only mean your version was built without 7z support. You could also get rid of them by using a git version of unarr and enabling 7z support.

[luisangelsm]

@xthursdayx having 7z support is desirable, there is a good amount of people with 7zip comics out there, probably because YACReader supported 7z files since the beginning as a side effect of using 7zip as the decompression library.

I am not sure how bad unarr performs with 7z files, but unarr has been used as the default library in Linux because p7zip was abandoned for a while (I know there is a fork now but I don't know its state), the right package versions were missing, etc. Controlling this in a Docker container is much easier, so if you can make it work is good news.

And yeah, the right version to try with is p7zip 16.02 as it was in the original repo.

Also, it would be nice to integrate the dockerized version of the headless server in the CI/CD pipeline so it gets updated on new releases and a dev version can also get updated when new stuff is merged into develop. The project is using Azure Pipelines in case you want to take a look.

[xthursdayx]

Thanks for getting back to me @selmf and @luisangelsm. I got kind of obsessed with working on this for the last day, and what I've found is this:

I've been able to successfully compile YACReaderLibraryServer with p7zip 16.02 from the original Sourceforge location @selmf linked. However, building this image still resulted in the patch error I mentioned above, though the container built from this image seemed to function without any problems.

I also created another branch of my docker repo and was able to successfully build an image that builds YACReaderLibraryServer with unarr installed via git from @selmf 's repo as a shared system library, with zlib1g, libbz2, and liblzma libraries and 7z support enabled.

What I found, though anecdotal, was that YACReaderLibraryServer compiled with p7Zip was significantly faster while scanning to create or update a library versus YACReaderLibraryServer compiled with unarr; taking around 45 minutes to scan my ~6500 comics versus 4-5 hours.

Running the YACReaderLibraryServer create-library and update-library commands found 6499 comics in total, and I experienced no 7z or rar errors, or any other errors (other than the lingering QObject error related to RequestMapper, which has been mentioned elsewhere (either here or on the forums, I can't remember) and seems to always appear in the logs no matter how YACReaderLibraryServer is compiled and installed, though I haven't noticed any negative impact on container performance :

QObject: Cannot create children for a parent that is in a different thread. (Parent is RequestMapper(0x562dfdd21060), parent's thread is QThread(0x562dfdd11ff0), current thread is QThread(0x562dfdd16070)

As I mentioned, complied with unarr (with 7z enabled), YACReaderLibraryServer was significantly slower, and also logged 3 errors:

! rar.c:112: Invalid header checksum @7                                                                                                                                                                       
! zip.c:167: Checksum of extracted data doesn't match
rar.c:214: RAR 5 format isn't supported 

However, that is far fewer than the number of 7z errors I received with the previous Docker image which logged literally hundreds of these 7z errors: _7z.c:200: 7z support requires 7z SDK (define HAVE_7Z). My previous Docker Image built YACReaderLibraryServer with unarr as an embedded library at /compressed_archive/unarr/, and with 7zTypes.h symlinked to Types.h at /compressed_archive/unarr/unarr-master/lzmasdk.

The new Docker image with YACReaderLibraryServer compiled with unarr (from git, with 7z support) actually ended up finding 11 more comics than the p7zip version (6510 in all), despite the three errors.

For now, I have the stable YACReaderLibraryServer Docker image compiled with p7zip pushed to Docker Hub, but I am currently working to ensure the stability of the Docker branch that uses unarr installed via git with 7z support, reducing the image size (it is already half the size of the image with p7zip, and am improving permissions, system supervision (using s6) and user:group/PUID/PGID management.

Anyway, you're welcome to check out my unarr development branch here: https://github.com/xthursdayx/yacreaderlibrary-server-docker/tree/unarr_dev I'm still actively working on it, but it seems to be working well currently. I'm happy to hear any thoughts or suggestions though and would be open to eventually integrating the dockerized version into the CI/CD pipeline, so happy to discuss that more.

[xthursdayx]

One thing that would be helpful to know in order to further streamline my image (though outside the scope of this specific issue), is what packages are necessary dependencies for building standalone YACReaderLibraryServer, and which are needed for actually running and using the server.

If I can remove the packages that are no longer needed at runtime, I'll be able to further reduce the size of the image.

[luisangelsm]

Ah, yes I forgot about rar5, which is also pretty much needed as more and more comics are coming in that format.

About the packages, I think @selmf knows better. It's been a while since the last time I built YACReader for Linux.

[selmf]

@xthursdayx 7z support in unarr currently has two limitations which severely impact performance. The biggest is that it needs to decompress the whole 7z archive or at least a whole chunk of solid data before returning any decompressed data. The other is that only 'standard' 7z archives with default compression algorithms work. This means that it will spend a significantly longer time processing 7z files and also could mean that you can get problems with memory if your machine is limited. These limitations are actually not an unarr problem but come from the fact that 7z support is provided by 7z's lzma-sdk. I would need to rewrite large portions of the 7z code to fix this.

As for your streamlining question, you could start with these:

• Qt5Core
• Qt5Gui
• Qt5Network
• Qt5Sql
• sqlite3
• qt5-image-formats-plugins (runtime only)

One of the drawbacks of Qt's qmake build system, especially on Debian and Ubuntu, is the poor support for checking build dependencies. I implemented a bare minimum check for some requirements but even this won't work on .deb systems as they generally tend to ship qmake without pkg-config support.

[selmf]

One thing I forgot to mention regarding the failing patch: You can only apply a patch once, it will 'fail' afterwards. Which actually is fine.

Having some sane defaults for a Docker image that we could reasonably assume everyone is using would be great. On the other hand, I am also happy that there is an active community developing these images in several flavors, many of them with interesting approaches.

Now the real question here is how to ensure these sane defaults while still having a diverse downstream ...

Btw, for a better idea on what dependencies YACReaderLibraryServer actually needs you can run ldd /usr/bin/YACReaderLibraryServer to see which libraries it links against.

[F-TD5X]

@xthursdayx Hello, I'm the one who did the PR #249. I checked the origin file at 121,8 was

    {
      _stack[i++] = _suffixes[cur];
      cur = _parents[cur];
    }

    _stack[i++] = (Byte)cur;
    lastChar2 = (Byte)cur;

So I think this error may be caused by some wrong newline code. This patch is just to fix an exploit, I think there will be no one to make a POC in some compressed comic files.

[xthursdayx]

I'll go ahead and close this since I think it's resolved for now.

I also wanted to let you know all know that I successfully redesigned my YACReaderLibraryServer docker image. The repo now builds two versions of the server - one with p7zip and one with unarr - that can be selected by docker image tag. The images are also multi-arch and should automatically work on amd64, arm32(v7), and arm64(v8). Each arch tag can also be specifically pulled. Cheers!