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

Can p7zip be included in this project? #1

Open
ghost opened this issue Mar 24, 2020 · 29 comments
Open

Can p7zip be included in this project? #1

ghost opened this issue Mar 24, 2020 · 29 comments

Comments

@ghost
Copy link

ghost commented Mar 24, 2020

It looks like p7zip is no longer actively maintained:
https://sourceforge.net/projects/p7zip/

The last release is 16.02 while 7-Zip is already at 19.00.
There are CVEs but since the code is not updated, package maintainers of Linux distributions have to include the patch themselves.
Someone asked whether p7zip will be updated, but there is no response.

Although 7-Zip is open source, the source is only released when there is a new version, so instead of being able to see the actual commits (and cherry-pick / port the patch to p7zip), we can only see a large diff which affects hundreds of files.

@rzr
Copy link

rzr commented Mar 24, 2020

Thanks for suggestion,

Can you suggest a fork to use a the base ?

https://github.com/search?q=p7zip

@rzr
Copy link

rzr commented Mar 24, 2020

Or maybe it's safer to use this base :
https://salsa.debian.org/debian/p7zip

@ghost
Copy link
Author

ghost commented Mar 25, 2020

I think https://github.com/3rdpartyrepos/p7zip looks good, since it just imports the latest source (16.02) and apply the CVE patches on top of it.

https://salsa.debian.org/debian/p7zip is also good (because you can trust the repo owner/maintainers), but it contains Debian-specific files (e.g. https://salsa.debian.org/debian/p7zip/-/tree/master/debian).

@rzr
Copy link

rzr commented Mar 25, 2020

Well it looks like @pgp know what should be done:

https://github.com/3rdpartyrepos/p7zip/commits?author=pgp

how can @abandonware/maintainers help ?
repo could be forked or transferred here,
if someone has a plan for better maintenance..

Any opinion welcome

Anyway It's good to have this ticket opened here

May I also share this tracker link:
https://bugs.debian.org/cgi-bin/pkgreport.cgi?repeatmerged=no&src=p7zip

ps: @86423355844265459587182778 I am curious about your nickname, can you give me more hints about it :)

@pgp
Copy link

pgp commented Mar 25, 2020

Hi everyone,
if you look at the diffs, those patches consist of few code lines each, so they were trivial to copy from the linux repos. I also wanted to bring the work ahead, by diffing 7-zip 16.02 against the subsequent versions (from 17.00 onwards), and then porting back the relevant differences to p7zip 16.02, but Meld indicates A LOT of modified source code files, so it will take time, and I have not enough free time to take over this at the moment. Maybe in the summer :)

@rzr
Copy link

rzr commented Mar 25, 2020

Would @abandonware/maintainers help for cooperative maintenance here,
see our dutties (draft) :

https://abandonware.github.io/

@ghost
Copy link
Author

ghost commented Mar 25, 2020

@pgp

I attempted to port the patches from 7-Zip to p7zip, but 16.02→16.03 is already a large diff so I can only give up - I am only an enthusiast and do not have enough knowledge to proceed.

Another problem is that p7zip has an alpha-quality (or experimental) GUI.
My two cents: Dropping the GUI may make maintenance easier, since we already have GUI front-ends like File Roller, Ark and PeaZip.
In any case, we probably have to filter out (exclude) GUI-related patches in 7-Zip.

Glad to hear that you are willing to bring the work ahead. Good luck and take care!

@rzr

My nickname is just a randomly generated string :)

@pgp
Copy link

pgp commented Mar 25, 2020

Yes @86423355844265459587182778 , that's exactly what I meant... I'm not really interested in the GUI part, since I use p7zip as dependency for this project of mine, where the GUI code is clearly not even compiled. Moreover, as you said, there are already plenty of good front-ends for handling archives on Unix, so IMHO porting the 7zFM code shouldn't be strictly needed; instead, there are other modifications (like raising the IV size to 128 bits in encrypted 7z-AES archives) that I consider appropriate to be ported in p7zip.

@pgp
Copy link

pgp commented Mar 25, 2020

Almost forgot... @86423355844265459587182778
I've already ported back modifications till version 16.04, but they are not in the main branch of the repo you mentioned, they're in the updatesFrom7Zip branch

@rzr
Copy link

rzr commented Mar 25, 2020

It would be also nice to hear form 7z community which strategy is best? I am also in favor of dropping the GUI.

If there is a need to have a repo here please let me know, I can do upstream import too, just tell me to
I am not sure @pgp is granted to transfer his project here or maybe he prefer to have it forked here...
in that case I would ask some justification on why this code base should be trusted :)

@ghost
Copy link
Author

ghost commented Mar 26, 2020

In addition to hearing from 7z community, I think we can also contact Igor Pavlov on SourceForge.

He said in a post that dates back to 2018:

There was another developer that worked for all p7zip related things. And there are no any news or messages from that developer. That situation with p7zip update stall is not good. But I don't work with linux code myself. Maybe later I'll try to look it.

So I guess Igor knows how to contact p7zip's developer. If @pgp (and possibly other developers) are willing to maintain p7zip, we can ask Igor (and/or p7zip's original maintainers) to point to the new repository instead of the old SourceForge project page.

(In my opinion, SourceForge's UI is not really suitable for project maintenance. GitHub, GitLab, BitBucket, etc. are more user-friendly.)

@pgp
Copy link

pgp commented Mar 26, 2020

It would be also nice to hear form 7z community which strategy is best? I am also in favor of dropping the GUI.

If there is a need to have a repo here please let me know, I can do upstream import too, just tell me to
I am not sure @pgp is granted to transfer his project here or maybe he prefer to have it forked here...
in that case I would ask some justification on why this code base should be trusted :)

Well, in the case I start to dedicate myself seriously to the project, I'll continue to perform modifications on the above mentioned repo (3rdpartyrepos/p7zip), or at most move it under my personal repos if it gains visibility; you are welcome to fork it and use it as upstream repo. As for the trustability issue, I already said I have started by the base version 16.02 on sourceforge (you can download it, clone the p7zip 16.02 repo, and perform a diff, and, clearly, I'm not responsible for the auditing of that version itself, I take it as is), and the few commits I've done for adding CVE patches and modifications taken from 7-zip till 16.04, are still easily auditable in my opinion.
Again, the strategy I've followed so far is:

  • diff a 7-zip version against the next one (e.g. the next diff to be performed will be: current--16.04 vs next--17.00);
  • for each modified code file, look at the corresponding one in current--p7zip -if it exists-, and try to propagate the differences;
  • if there are code blocks delimited by #ifdef _WIN32, or code blocks that depend on <windows.h> and windows-only headers, ignore them if possible, otherwise try to adapt them using Posix APIs

And, as for @86423355844265459587182778 questions, I'm obviously not going to put anything on sourceforge, all the future commits will stay here on github, at most they will be mirrored on bitbucket and/or gitlab; and, sure, if I succeed in porting the modifications at least from 16.04 to 17.00, and there will be a consistent user base for testing those modifications, we can safely ask Igor Pavlov to link to the repo from his site.

@pgp
Copy link

pgp commented Mar 27, 2020

Thank you, I'll try to have a look tomorrow and try to build it myself from Linux, OSX and FreeBSD

@ghost
Copy link
Author

ghost commented Mar 28, 2020

Sorry everyone, I forgot to change branch so no wonder why it seems there is no error.

I tried my best to eliminate build errors, but now I am stuck at:

../../../../CPP/Common/StringConvert.cpp: In function ‘AString UnicodeStringToMultiByte(const UString&, UINT)’:
../../../../CPP/Common/StringConvert.cpp:178:27: Error: invalid conversion from ‘wchar_t’ to ‘const wchar_t*’ [-fpermissive]
  178 |       srcString.Insert(i, c);
      |                           ^
      |                           |
      |                           wchar_t
In file included from ../../../myWindows/StdAfx.h:20:
../../../../CPP/Common/MyString.h:668:46: Note:   initializing argument 2 of ‘void UString::Insert(unsigned int, const wchar_t*)’
  668 |   void Insert(unsigned index, const wchar_t *s);
      |                               ~~~~~~~~~~~~~~~^
make[1]: *** [makefile.list:284: StringConvert.o] Error 1

Although the code failed to build, it will still be available here in case it can be of any use.

@pgp
Copy link

pgp commented Mar 29, 2020

Hi, from what I've seen in your attempts, you tried to apply the whole diff from 7-Zip 16.04 -> 7-Zip 17.00 in one round, and then try to refine and adjust build errors...
This can be a total mess since you lose track of modified dependencies. I've opened another branch in my repo, here, and I've started to merge the files with few modifications (following include dependencies if necessary). Latest commit successfully builds on Linux x64. Moreover, you can find attached an excel file with the modified files, sorted by number of modifications. In the next days I'll try to continue merging lighter files, so that the heaviest ones will be left for subsequent analysis later.
Also, I'll setup Cirrus CI in order to have build results on at least Linux, OSX and FreeBSD for every commit.
7zip_1604_to_1700_sorted_modified_files.xlsx

@ghost
Copy link
Author

ghost commented Mar 30, 2020

Can I describe your strategy as the following?

  1. Choose a changed file (let's call it "A") with very little modifications, and apply patch A.
  2. Using CI, look for any build error and see what A depends on.
  3. Suppose we find out that when A is changed, files B and C also need to change. Take note of this (in the spreadsheet) and apply patch B and C.
  4. Repeat the process until there is no build error.

@pgp
Copy link

pgp commented Mar 30, 2020

Yes, exactly. One more trick to speed things up is: given a modified file A in 7-Zip, if there are a lot of modifications, I diff the version before modification (in this case, A from 16.04) with its corresponding file A' from p7zip current version. If the two files are equal, I try to copy directly the new file, otherwise (it's common in p7zip to find additions consisting of several ifdef blocks) I merge back the differences with Meld.

@ghost
Copy link
Author

ghost commented Mar 30, 2020

Thank you for the tip.

After a few trials, I discovered that, if a patch changes the variable type (type conversion?), then it should be set aside first (even if it is a small patch).

For example, the patch for CPP/7zip/Archive/MslzHandler.cpp:

-    _name += (wchar_t)replaceByte;
+    _name += (char)replaceByte;

If it is applied, then MyString.{h,cpp} need to be patched, and then many other files which uses AddAscii are affected as well.

@pgp
Copy link

pgp commented Mar 30, 2020

That's one of the main differences of p7zip w.r.t 7-Zip. At the API and filesystem access level, Unix uses variable-length UTF-8 strings by default, Windows generally uses fixed-width wide strings (2 bytes per char on windows) - this reflects in 7-Zip using wide strings and p7zip using normal ones (with variable length). So additional care has to be taken in involved code blocks, and you cannot know it a priori, since, as you said, the difference in the string type used may involve an included file. The main hint to detect this is checking in the 7-Zip variant of the file you are attempting to modify, occurrences of:

  • std::wstring
  • wchar_t
  • LPCWSTR (win32 api type alias for const wchar_t*)
    If that's the case, with high probability the original file in p7zip WILL differ from the original file in 7-Zip, and you will have to keep this into account when porting modifications.

@rzr
Copy link

rzr commented Mar 30, 2020

My side I can import debian or/and arch base (apply their patches) may and then you can try submit patches over it and we try to review them ? would than help ?

@rzr rzr transferred this issue from abandonware/abandonware.github.io Mar 30, 2020
@pgp
Copy link

pgp commented Mar 30, 2020

Debian/arch patches (from what I've understood, arch uses the same patchset of debian) have been already merged in the branch in which I'm currently working on (actually, they were the first modification I merged, some months ago).
At this stage, if you haven't the necessary insight into both projects (7-zip and p7zip) you could do the following auditing operations in order to ensure I'm not placing anything "irregular" in my commits since p7zip 16.02.

  • Start with downloading 7zip versions from 16.02 onwards (till 16.04 or 17.00 for now) from osdn.net, which is the official channel for old releases -- download 7zip****-src.7z files
  • Compare with this repo's commits and ensure there are no differences in the history w.r.t. the src snapshots from osdn (apart from an added README file)
  • Use a GUI diff tool like Meld, launch two dir diffs, one between adjacent commits in the 7-zip history from the repo at the previous step, and one with dir diffs between significative commits in this branch's history. This command is used for performing the diff between commits under Linux:
    git difftool -t meld --dir-diff commithash1 commithash2

@rzr
Copy link

rzr commented Mar 30, 2020

I have created repo from debian base and applied some of their trusty patches :
others should be ported:
https://salsa.debian.org/debian/p7zip/-/tree/master/debian%2Fpatches

I also applied one patch from arch:

https://git.archlinux.org/svntogit/packages.git/tree/trunk?h=packages/p7zip

others are touching rar files that debian removed

Do we want this rar part ?

IMHO sliming down p7zip is a good strategy

ps: I am trying to promote this https://twitter.com/RzrFreeFr/status/1243569839886696451 if you can help that would be appreciated

@pgp
Copy link

pgp commented Mar 30, 2020

In the project where I use p7zip (actually, that p7zip version contains some slight modifications w.r.t. to the official one) I enabled rar by default (if I remember correctly, due to licensing issue, rar codec is available only in 7z.so, and not in the fat binary 7za), so I think it should be kept. And again, yes, if you look at the beginning of the commit history I mentioned previously, all those patches hsould have been already applied.
ps: I'll be glad to spread the word about your project, anyway

@ghost
Copy link
Author

ghost commented Mar 30, 2020

I don't have any SNS (Twitter, Facebook, etc.) accounts :(

@pgp
Just found out you use make in CI - so I see why e9c8b10 fails if I use make all3 for testing.

@pgp
Copy link

pgp commented Mar 30, 2020

yes, currently I'm building only the 7za target...

@pgp
Copy link

pgp commented Mar 30, 2020

anyway, I think it's a good idea to continue merging files with few, trivial modifications, like the one in PR you just posted (just do it in the proper branch, please)..

@rzr
Copy link

rzr commented Apr 5, 2020

Please try to rebase your patches on @abandonware fork imported from debian

@ghost
Copy link

ghost commented May 20, 2020

Update:

The 7-Zip homepage now lists a new fork of p7zip, szcnick/p7zip.

According to the discussion thread, although the new fork has released a "17.01" version, it is not really based on 7-Zip's 17.01. Instead, it is more like p7zip 16.02 + CVE fixes + Additional codecs in 7z archive.

@rzr
Copy link

rzr commented May 20, 2020

OK
May I suggest @szcnick to have be rebased on @abandonware/maintainers one because the base is loosing some attributions:

p7zip-project/p7zip@96c6249

Relate-to: https://purl.org/rzr/flows

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

2 participants