Bug 244181
| Summary: | unzip: Add passphrase and GLIBC build support | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Mingye Wang <arthur200126> | ||||||||||||||
| Component: | bin | Assignee: | Yoshihiro Takahashi <nyan> | ||||||||||||||
| Status: | Closed FIXED | ||||||||||||||||
| Severity: | Affects Some People | CC: | ak, des, nyan, tom, ypankov | ||||||||||||||
| Priority: | --- | Keywords: | needs-qa, patch | ||||||||||||||
| Version: | CURRENT | Flags: | koobs:
maintainer-feedback?
(des) koobs: maintainer-feedback? (ak) nyan: mfc-stable13+ nyan: mfc-stable12+ koobs: mfc-stable11? |
||||||||||||||
| Hardware: | Any | ||||||||||||||||
| OS: | Any | ||||||||||||||||
| See Also: | https://github.com/libarchive/libarchive/issues/1138 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 211704 [details]
Add passphrase support
Created attachment 211705 [details]
Mention ZIPX in unzip(1)
The 'support glibc' and 'mention zipx' patches looks alright, but I think you forgot to free passphrase_buf in 'add passphrase' patch. Created attachment 211738 [details]
Add passphrase support (v2)
Oops! Here is the patch with the free() added.
Expect a passphrase v3 -- it simply doesn't make sense to free in main() when the only cause of malloc() is planted by unzip(). You might want to open this at https://reviews.freebsd.org so it's easier to comment on the code. Created attachment 211753 [details]
Add passphrase support (v3)
Created attachment 212674 [details]
Add passphrase support
It seems that unzip fails to build with the last passphrase support patch. I fixed it a little, please check it out.
+ memset(passphrase_buf, 0, PPBUFF_SIZE); + free(passphrase_buf); This should probably be using explicit_bzero() to prevent the compiler from optimizing it away. tar(1) has the same issue. The explicit_bzero(3) is not portable. So it can't be used in libarchive and it's also absent in NetBSD, which is (still ?) upstream for our unzip. What about memset_s(), which is in C11? Looks like NetBSD wants explicit_memset(), which also isn't portable. readpassphrase() in the same patch is no more portable than explicit_bzero(), so you should just use explicit_bzero. Yes, memset_s() should work. The libarchive's cpio and bsdtar use portable version from libarchive_fe if necessary (see use.bin/tar/Makefile). I left readpassphrase() because Free/Open/NetBSD have it. Can be easily changed back to lafe_readpassphrase(). *** Bug 253726 has been marked as a duplicate of this bug. *** I found a problem with your patch to add passphrase support. In passphrase_callback(), passphrase_buf is (re)defined as a local variable, so the global passphrase_buf is not changed here. I have fixed it. Please see https://reviews.freebsd.org/D28892 BTW, I except GLIBC part at this time. I think that it should be done by libarchive's repository first. Thanks. That was silly braino. As for GLIBC, as I wrote in 253726, it needs more work and testing. The NetBSD opted for use defines with getpass/getpass_r/readpassphrase but I'm not sure it's the best variant or even if it will work on all/most linux distros. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=a4724ff48108840416c83f10e15d666ac8d78937 commit a4724ff48108840416c83f10e15d666ac8d78937 Author: Yoshihiro Takahashi <nyan@FreeBSD.org> AuthorDate: 2021-09-25 16:32:42 +0000 Commit: Yoshihiro Takahashi <nyan@FreeBSD.org> CommitDate: 2021-09-25 16:32:42 +0000 unzip: sync with NetBSD upstream to add passphrase support - Add support for password protected zip archives. We use memset_s() rather than explicit_bzero() for more portable (See PR). - Use success/failure macro in exit() - Mention ZIPX format in unzip(1) Submitted by: Mingye Wang and Alex Kozlov (ak@) PR: 244181 Reviewed by: mizhka Obtained from: NetBSD MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D28892 usr.bin/unzip/unzip.1 | 10 ++++++-- usr.bin/unzip/unzip.c | 66 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 66 insertions(+), 10 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=cea130c0b8f5a32b0e4a22befb89bad73f8663c2 commit cea130c0b8f5a32b0e4a22befb89bad73f8663c2 Author: Yoshihiro Takahashi <nyan@FreeBSD.org> AuthorDate: 2021-09-25 16:32:42 +0000 Commit: Yoshihiro Takahashi <nyan@FreeBSD.org> CommitDate: 2021-10-02 11:56:19 +0000 unzip: sync with NetBSD upstream to add passphrase support - Add support for password protected zip archives. We use memset_s() rather than explicit_bzero() for more portable (See PR). - Use success/failure macro in exit() - Mention ZIPX format in unzip(1) Submitted by: Mingye Wang and Alex Kozlov (ak@) PR: 244181 Reviewed by: mizhka Obtained from: NetBSD Differential Revision: https://reviews.freebsd.org/D28892 (cherry picked from commit a4724ff48108840416c83f10e15d666ac8d78937) usr.bin/unzip/unzip.1 | 10 ++++++-- usr.bin/unzip/unzip.c | 66 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 66 insertions(+), 10 deletions(-) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=970c3982cd6a05efe9b4666a8a7f98670f18f36e commit 970c3982cd6a05efe9b4666a8a7f98670f18f36e Author: Yoshihiro Takahashi <nyan@FreeBSD.org> AuthorDate: 2021-09-25 16:32:42 +0000 Commit: Yoshihiro Takahashi <nyan@FreeBSD.org> CommitDate: 2021-10-02 11:57:24 +0000 unzip: sync with NetBSD upstream to add passphrase support - Add support for password protected zip archives. We use memset_s() rather than explicit_bzero() for more portable (See PR). - Use success/failure macro in exit() - Mention ZIPX format in unzip(1) Submitted by: Mingye Wang and Alex Kozlov (ak@) PR: 244181 Reviewed by: mizhka Obtained from: NetBSD Differential Revision: https://reviews.freebsd.org/D28892 (cherry picked from commit a4724ff48108840416c83f10e15d666ac8d78937) usr.bin/unzip/unzip.1 | 10 ++++++-- usr.bin/unzip/unzip.c | 66 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 66 insertions(+), 10 deletions(-) Committed. Just for completeness sake, I've re-considered the whole GLIBC matter. GLIBC actually is okay with `optind = 1` resetting, so all we do is shadow optreset away, which @somasis on github did. glibc or not, there is one tiny issue remaining: we can't deal with member filenames starting with a dash. This is easy to fix too: you just add a pass after the second getopt to pick up any remaining arguments. You will need to double-dash after the archive name, which is weird, but it works. There is a tiny complication with glibc, however. Consider `unzip a.zip -- -f.bmp`. On BSD this all makes sense. On glibc, because it rearranges by default, it becomes `unzip -- a.zip -f.bmp`, with nopts putting you on "a.zip". And then "-f.bmp" gets fed to getopts again without the protecting "--". So what you do is disable the rearrangement via POSIXLY_CORRECT. It's very possible that I've still not thought this through. Anyways, the diff is at https://github.com/AdrianVovk/libarchive/pull/1. It's written on top of an attempt at putting BSD unzip into the libarchive code base. |
Created attachment 211703 [details] Support glibc builds I would like to add the attached series of patches to unzip: 1. glibc support, since some people on libarchive's GitHub page is showing interest in this replacement util. 2. passsphrase support, both in the unsafe command-line entry form (-P) and in the safe getpassphrase(3) form. 3. minor edit to the manpage, just to remind people that we can decompress fancy ZIPX archives if libarchive can. I thought about adding the filename iconv stuff from info-zip's unzip beta 6.10b, but doing stuff like iconv_open is a bit too much for me now. It *is* a nice feature though, at least according to the Linux users who came up with unzip-iconv in the first place. I might just add it sometime. The patch is based against FreeBSD GitHub src tree. The last commit in usr.bin/unzip should be 7551d83.