Bug 1045 - unzip unix attributes
Summary: unzip unix attributes
Status: RESOLVED FIXED
Alias: None
Product: Busybox
Classification: Unclassified
Component: Other (show other bugs)
Version: 1.15.x
Hardware: PC Linux
: P5 enhancement
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-05 12:17 UTC by Ilja V. Ryndin
Modified: 2010-05-24 02:34 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:


Attachments
set the unix file attributes for zipped files (2.22 KB, patch)
2010-02-05 12:17 UTC, Ilja V. Ryndin
Details
fixed (1.91 KB, patch)
2010-02-08 12:44 UTC, Ilja V. Ryndin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ilja V. Ryndin 2010-02-05 12:17:34 UTC
Created attachment 1057 [details]
set the unix file attributes for zipped files

The default unzip utility doesn't set unix attributes of unpacking dirs and files. I tried to solve this problem in the following patch.
Comment 1 Denys Vlasenko 2010-02-05 13:11:54 UTC
Three things need fixing:

Trying to apply to pristine 1.15.2:
# patch -p1 </tmp/busybox-1.15.2-unzip-unix-attr.patch 
patching file archival/unzip.c
Hunk #1 FAILED at 252.
Hunk #2 succeeded at 386 (offset -1 lines).
Hunk #3 FAILED at 433.
Hunk #4 succeeded at 478 (offset -2 lines).
Hunk #5 FAILED at 535.
3 out of 5 hunks FAILED -- saving rejects to file archival/unzip.c.rej

You only ever use unix_file_attrs if ENABLE_DESKTOP, but you figure its value even if !ENABLE_DESKTOP.

Please follow the code style.
Comment 2 Ilja V. Ryndin 2010-02-08 12:44:13 UTC
Created attachment 1075 [details]
fixed

Hello.

Sorry for my prev patch. It was really broken.
Please, try to apply the new one.

:)

I use ENABLE_DESKTOP define because the default code design demands it. I use the functions that have been defined in corresponding sections. Could you explain briefy why the functionality depends on this define?
Comment 3 Denys Vlasenko 2010-02-22 10:21:46 UTC
Indentation is still broken.

Style: fstat( dst_fn, &st ) -- do you see that the rest of the file does not have spaces inside ()s?

Finally, there is a bug:

archival/unzip.c: In function ‘unzip_main’:
archival/unzip.c:494: warning: ISO C90 forbids mixed declarations and code
archival/unzip.c:621: warning: ISO C90 forbids mixed declarations and code
archival/unzip.c:622: warning: passing argument 1 of ‘fstat’ makes integer from pointer without a cast
/usr/include/sys/stat.h:220: note: expected ‘int’ but argument is of type ‘char *’
Comment 4 Denys Vlasenko 2010-05-24 01:45:25 UTC
Looked at the code closer.

It's BUGGY!

First, you did not bother unifying cds reading done for streaming archives, which is just above your addition. This makes us read cds header twice.

Then:

cds_header_t cds_attr_header;
cds_attr_offset = read_next_cds(0, cds_attr_offset, &cds_attr_header);
if (cds_attr_header.formatted.version_made_by & 0x0003) { // for unix archives

"& 3" test is wring : it tests wrong byte, it tests "unix" wrongly - should check "= 3", it doesn't bother to swap bytes on big endian machine. *3 BUGS* in one line!

        unix_file_attrs = (cds_attr_header.formatted.external_file_attributes >> 16);

no byte swap here too.

} else {
        unix_file_attrs = 0777;
}

Very bad...
Comment 5 Denys Vlasenko 2010-05-24 02:34:04 UTC
Fixed in git.