Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIXED in Git 679c45f28] Unrar.dll - technical proposal for correct extraction and find for load algoritm used after Git-2ce8617ba #1116

Closed
VictorVG opened this issue Dec 31, 2018 · 14 comments

Comments

@VictorVG
Copy link

Users make reasonable comments about the loss of portability of the program after Git 2ce8617 - the unrar.dll library is always unpacked in the %LOCALAPPDATA%\SumatraPDF\Extracted<randomdiir> directory even if it is in the same directory as the SumatraPDF executable.

To eliminate their comments, I propose to adjust the algorithm for searching and loading this DL like:

Proc DLLTools(); 
 Dcl CDir Char
  Dcl DllPatch Char
  CDir = GetEnv(%CurrentDir%);
      Begin; 
          IfFileExists(CDir\unrar.dll) Then Load(CDir\unrar.dll) Else
              Begin
                   If (GetFileAttrib(CDir) == "Read Only") or (GetDirLockStatus(CDir) == "Locked")) Then 
                      DllPatch = GetEnv(%TEMP%)..L'\SumatraPDF\Extracted Else DllPatch = CDir;
                   End;
                     Put(unrar.dll,DllPatch);
                     Load(unrar.dll,DllPatch)
               End;
            End;
        End; 
      Return;      
   End;

P.S.

And taking this opportunity, let me congratulate you on the upcoming New Year and wish you happiness, health and more good and interesting people on the path of life.

@kjk
Copy link
Member

kjk commented Dec 31, 2018

Thanks for the good wishes and happy New Year to you as well.

Yes, I'll improve this to use existing dll first.

@VictorVG
Copy link
Author

Thank you And once again you a Happy New Year!

@VictorVG
Copy link
Author

VictorVG commented Feb 26, 2019

I rewrites src\AppTools.cpp DLL extract algorithm and added try check for Read Only folders:

Rewrites src\AppTools.cpp DLL extract algorithm

/* Begin startup variable block */

Dcl ExePath Char
Dcl DllPath Char

/* Get random part for DLL patch */

   Suf = Subst(1,32,Random());

/* Get 'SumatraPDF.exe' patch use WinAPI */

   ExePath = GetExePath('SumatraPDF.exe');

/* End startup variable block */

/* Begin function block */

/* Check startup patch */

/* Check "%Program Files%" or "%Program Files(x86)%" */
  Function CheckPF()
     Return = (Subst(4,13,ExePath) == "Program Files");
  End;

/* End function block */

/* Begin SumatraPDF.exe DLL init block */

   If ((CheckPF() or IsAttrib(ExePath,ReadOnly)) Then
      Begin:
        If IsAmd64('SumatraPDF.exe') Then
          Put('unrar64.dll',GetEnv('APPDATA').."\Local\SumatraPDF\extracted");
         Else
          Put('unrar.dll',GetEnv('APPDATA').."\Local\SumatraPDF\extracted");
        End;
          DllPath = GetEnv('APPDATA').."\Local\SumatraPDF\extracted";
      End;
     Else
      Begin;
       If not (IsExist('unrar.dll',ExePath) or IsExist('unrar64.dll',ExePath)) Then
        If IsAmd64('SumatraPDF.exe') Then Put('unrar64.dll',ExePath) Else Put('unrar.dll',ExePath) End;
        DllPath = ExePath;
       End;
      End;
    End;

/* Load needed DLL */

    If IsAmd64 Then load('unrar64.dll',DllPath) Else load('unrar.dll',DllPath) End;

/* End SumatraPDF.exe DLL init block */

Reasone for editing is minimized write's on Flash/SSD disks

@VictorVG
Copy link
Author

VictorVG commented Mar 1, 2019

Test files for check open RAR 5.0 format - test-rar29.cbr - used RAR 2.9 format is supported the lib unaar and test-rar50.cbr used RAR 5.0 format (header signature 0x52 0x61 0x72 0x21 0x1a 0х00 0x00) not supported existing version lib unaar.

test-rar29.cbr.zip
test-rar50.cbr.zip

@VictorVG
Copy link
Author

VictorVG commented Apr 20, 2019

I read Rar unpack code in to rar/rar rar.c (212):

    if (memcmp(signature, "Rar!\x1A\x07\x00", sizeof(signature)) != 0) {
        if (memcmp(signature, "Rar!\x1A\x07\x01", sizeof(signature)) == 0)
            warn("RAR 5 format isn't supported");

and in the yard the 2019th year :)

ps. - fix typo

@VictorVG
Copy link
Author

I added wish up to unarr project for add support RAR5 format Issues #3 ...

@SumatraPeter
Copy link

While the 32 and 64-bit DLLs are extracted to different randomdirs so there's no overlap, the main issues are loss of both self-containment and stealth portability.

Loss of self-containment is obvious since the portable versions instead of extracting the UnRAR DLLs to the EXE's folder are extracting to %LocalAppData% by default. %LocalAppData% should IMO be used only as a fallback in case the portable EXE's folder is not writeable for some reason.

Loss of stealth portability is due to the fact that closing neither the 32 or 64-bit portable version seems to wipe its associated randomdir (although the 64-bit version wipes the 32-bit one's randomdir and vice versa).

So not only are the current 'portable' versions not truly self-contained, they're not even cleaning up after themselves. 😞

@VictorVG
Copy link
Author

Fixed in Git 679c45f

@VictorVG VictorVG changed the title Unrar.dll - technical proposal for correct extraction and find for load algoritm used after Git-2ce8617ba [FIXED in Git 679c45f28] Unrar.dll - technical proposal for correct extraction and find for load algoritm used after Git-2ce8617ba Nov 30, 2019
@SumatraPeter
Copy link

@kjk: So if I understand the changes correctly, the latest builds are no longer extracting any UnRAR DLLs at all and are relying on incorporating the UnRAR source itself into Sumatra? And if so, does this mean UnArr is no longer being used?

While this seems fine and dandy, a matter of concern when it comes to relying on external sources is that if Sumatra experiences another prolonged development hiatus then in effect the UnRAR code inside (with bugs if any) will be 'frozen' too, despite the fact that newer UnRAR DLLs will continue to be released. External DLLs do have the advantage of allowing the user to simply substitute them with the latest API-compatible release...

@kjk
Copy link
Member

kjk commented Nov 30, 2019

Yes to all that.

However, in practice:

  • UnRAR code doesn't have bugs
  • almost no-one would actually bother to replace UnRAR.dll used by Sumatra. I doubt many people even knew it was an option

@VictorVG
Copy link
Author

VictorVG commented Dec 1, 2019

The format of the RAR container changes quite rarely (it changed in RAR 1.3, RAR 1.4, RAR 1.5, RAR 2.9 and RAR 5.0,) and the changes made to the UnRAR code are mostly service-based, which we see on WhatsNew.txt in ftp://Anonymous:[email protected]:@ftp.rarlabs.com:21/rar/UnRARDLL.exe, and the unrarsrc*.tar.gz sources are which can be download from ftp://Anonymous:[email protected]:@ftp.rarlabs.com:21/rar/unrarsrc-.tar.gz (current version 5.8.4 is the third digit is the current beta number), and the library itself has not changed much since 2013 (source UnRARDLL.exe/WhatsNew.txt, extract):

  • February 2, 2019

New CmtBufW field of RAROpenArchiveDataEx structure allows to read the archive comment in Unicode.

  • 10 January 2019

New OpFlags field and ROADOF_KEEPBROKEN flag in RAROpenArchiveDataEx structure to specify if extracted files with invalid checksum shall be preserved.
Reserved [28] in the same structure is changed to Reserved [27].

  • 3 October 2018

UnpVer field of RARHeaderData and RARHeaderDataEx structures erroneously returned 200 instead of 50 for RAR 5.0 archives. Fixed now.

  • 17 September 2016

New MtimeLow, MtimeHigh, CtimeLow, CtimeHigh, AtimeLow, AtimeHigh fields of RARHeaderDataEx structure provide information about high precision times of archived files.

RAR_DLL_VERSION is incremented to 8.

  • July 6, 2015

New RedirType, RedirName, RedirNameSize, DirTarget fields of RARHeaderDataEx structure provide information about file system redirection headers.

Please be sure to fill either the entire RARHeaderDataEx structure or at least its Reserved field with zeroes before passing to RARReadHeaderEx.

RAR_DLL_VERSION is incremented to 7.

  • November 5, 2013

OpenResult field of RAROpenArchiveData and RAROpenArchiveDataEx structures can return the new ERAR_BAD_PASSWORD error code.
This code can be returned only for archives with encrypted headers in RAR 5.0 format.

so I don’t think that we can encounter a hidden error in the code of the RAR5 unpacker, and if Yevgeny Roshal changes the format of the container, he will immediately lay out his new description and the source code of the new UnRAR and we can take this into account.

@SumatraPeter
Copy link

I really wouldn't go so far as to declare with absolute certainty that the UnRAR code has no bugs or vulnerabilities whatsoever, but anyway, as long as any significant changes are incorporated into Sumatra on a timely basis I guess it's all good. :) Certainly one less thing for your average user who knows nothing of DLLs to contend with.

@VictorVG
Copy link
Author

VictorVG commented Dec 1, 2019

Eugene is often the subject of WinRAR on forun.ru-board.com and I know how carefully he checks any suspicion of errors - they have practically no chance to go unnoticed, including the people who hunt them. And even an error occurs, until the next update of the unrar sources in the DLL / EXE is saved, and if it is easier to fix it ourselves and transfer the patch to Eugene, for which he will only be grateful. At least during the period of our acquaintance with him, I had an opinion about him as a very friendly, decent, and attentive person.

@SumatraPeter
Copy link

SumatraPeter commented Dec 1, 2019

@VictorVG: Eugene is definitely polite and a conscientious programmer. However no disrespect to him, but the very nature of the beast is such that any complex piece of software is bound to have bugs and vulnerabilities, and even open source (which WinRAR except for the UnRAR part isn't) cannot guarantee perfection. In short, let's not pretend that his programs have never suffered from any bugs at all (even long-term ones), and if you need proof you can check out various exploit/vulnerability DBs including 1, 2, 3 and 4 (not to mention those on the dark web where exploits are regularly bought and sold).

Anyway, as I've said already above, we have nothing to worry about as long as Sumatra is under active maintenance and development and incorporates updated (bug-fixed) third-party components on a timely basis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants