87965 - Can't expand chrome/*.jar files on ARM

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect, P3)

Description

[Koike Kazuhiko]

If you build Mozilla for ARM and run it on ARM, nsZipArchive::BuildFileList()
fails on expanding chrome/*.jar files.

Size of ZipEnd in zipstruct.h becomes 24. But it should be 22.

Comment 1

[cls]

This is not a build issue but I can't seem to find a libjar bugzilla component. :-/

What version of zip do you have installed on the system you used to compile mozilla?

Comment 2

[Koike Kazuhiko]

Version of zip is 2.3.

Comment 3

[cls]

Here's a silly question:  How do you know that the size of ZipEnd is being
treated as 24 instead of 22 ?

Comment 4

[Koike Kazuhiko]

You can insert printf() in nsZipArchive::BuildFileList() and see sizeof(ZipEnd).

Comment 5

[Koike Kazuhiko]

On x86, sizeof(ZipEnd) is 22.

Comment 6

[cls]

Just on a hunch, can you do the following:

rm -rf dist/bin/chrome

edit config/autoconf.mk and add -k to the end of the ZIP_PROG variable
rebuild and see if you still have the error

Comment 7

[Koike Kazuhiko]

The original reporter(in Bugzilla-jp, bug_id = 1091) tested the steps
Christopher Seawood suggested on 29 Jun. Nothing new happened.

Comment 8

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 9

[Jeroen Dobbelaere]

The reason for this behavior of gcc has to do with the ARM SDK :(

From gcc/gcc/config/arm/arm.h :

/* Setting STRUCTURE_SIZE_BOUNDARY to 32 produces more efficient code, but the
   value set in previous versions of this toolchain was 8, which produces more
   compact structures.  The command line option -mstructure_size_boundary=<n>
   can be used to change this value.  For compatability with the ARM SDK
   however the value should be left at 32.  ARM SDT Reference Manual (ARM DUI
   0020D) page 2-20 says "Structures are aligned on word boundaries".  */
#define STRUCTURE_SIZE_BOUNDARY arm_structure_size_boundary
extern int arm_structure_size_boundary;

/* This is the value used to initialise arm_structure_size_boundary.  If a
   particular arm target wants to change the default value it should change
   the definition of this macro, not STRUCTRUE_SIZE_BOUNDARY.  See netbsd.h
   for an example of this.  */
#ifndef DEFAULT_STRUCTURE_SIZE_BOUNDARY
#define DEFAULT_STRUCTURE_SIZE_BOUNDARY 32
#endif



And as far as I know (I did a rough check of the c++ draft), this
is allowed by c++, which means that the code depending on 
'sizeof(struct XXX) == an expected value' should be adapted :(

Comment 10

[Daniel Veditz [:dveditz]]

That also means we can't just read stuff off disk into a structure, at least not
stuff that we didn't write out ourselves like a zip archive.

Taking...  What's ARM?

Comment 11

[Jeroen Dobbelaere]

Correct.

This also means that when you've received some data in bytes (possible from an
other architecture) (through file or http or ...),
that you cannot just use a struct of structs and expect the layout to match :(

example :

struct f1 {
  char a0;
  char b;
  char c;
};

struct f2 {
  char a1;
  char b;
  char d;
};

union u1 {
  struct f1 f1;
  struct f2 f2;
};

struct format {
  char a;
  char b;
  union u1 u;
  char end;
};

On ARM, the union u will start at a 4 byte boundary, because
of the alignment restrictions for struct.
Also, the size of the union is 4 bytes (not 3). Because of this,
'end' will also start on a 4 byte boundary.
The total size of 'format' is 12 bytes.
On an i386 system, the total size would be 6 bytes (!)


BTW. ARM is a company that has created a cpu risc architecture (known as ARM)
(See <http://www.arm.com/>) which is used in a lot of embedded systems.

Comment 12

[Jeroen Dobbelaere]

Attachment: proposed patch : decode zipfile independent of sizeof(ZipEnd) etc.

The patch defines sizes of the zip-headers in the .h file, and uses those
to traverse the buffers, in stead of depending on 'sizeof(ZipEnd)' (etc...)
being a constant on all platforms.

Another solution on an ARM platform with gcc, would be to use the
'-mstructure-size-boundary=8' option. (for a better description, see bug 9519)
Patch is against mozilla-0.9.8

Comment 13

[Bradley Baetz (:bbaetz)]

If the problem is that you're assuming that there is no padding, can't you just
use __attribute__((packed)) on the structure, for all platforms? Or does that
not handle trailing padding? (Well, use that on gcc, and the equivalent for
msvc/mac/etc) 'info gcc' has the docs on that attribute.

Any platform is allowed to use padding (6.7.2.1/15 in the C99 spec), so this is
technically a bug on all platforms, and if the structure is packed already, then
that attribtue shouldn't make a difference.

The patch here assumes that there is no padding between each unsigned char
element, and while thats probably a reasonable assumption (since sizeof(unsigned
char)==1, padding is sort of pointless), an implementation can add padding
anywhere it wants after any structure member.

Comment 14

[Jeroen Dobbelaere]

In this case, the problem is not the padding between members. This padding is
strictly specified in C. The problem is the trailing padding, making the
size (and alignment) of structs bigger.
'packed' would also help, but only for gcc. The proposed solution should
work with all compilers.
 

Comment 15

[Bradley Baetz (:bbaetz)]

Sure, but your solution fails if the compiler puts padding between elements.

I'm resonably certain that msvc has a packed option.

My point is that this code is non-portable anyway. If you add an assertion that
sizeof(foo)==whatever somewhere, then at least this can be easily tracked, and
the slower member by member copy used for those cases, if they ever occur.

Will packed cause a SIGBUS for unaligned access on some platforms? With unsigned
char, I suspect not, and any platform which did require padding would have
failed for the same reason ARM is failing, right?

Comment 16

[Jeroen Dobbelaere]

I don't say other compilers don't have a 'packed' option, but it probably
has to be specified in a different way.

The structures in zipstruct.h all contain 'unsigned characters'.
As they are POD (plain old data), this means that each member
is guaranteed to be aligned to the smallest alignment (= alignment of 'char').

The only thing ARM adds, is the restriction that a struct has a size of n words,
and is word-aligned. This should/could only give problems (in case of unaligned
structs) when the struct as a whole is accessed (ex. duplication).
Access to a character member should not insert a SIGBUS.

The best (most portable) solution should be to never take
a bytestream and place a struct above it !

Comment 17

[Bradley Baetz (:bbaetz)]

Right, thats the most portable, but its slower, so unless someone actually needs
it....

why not just use packed on ARM, and add an assertion so that others can find
this problem if they run into it?

In any event, please add a comment.

Comment 18

[Jeroen Dobbelaere]

Ok. I'll prepare a patch which will use the 'packed' attribute when
compiling with gcc, and an extra check to ensure that the structs have the
correct size.
 

Comment 19

[Bradley Baetz (:bbaetz)]

Note that this is just my opinion; see what others think.

Comment 20

[Jeroen Dobbelaere]

Attachment: alternative patch using __attribute__ ((packed))

This is an alternative patch using the __attribute__ ((packed)) alternative.

pro : - only one file to patch
con : - __attribute__ ((packed)) only works with gcc
      - size check can break builds with other compilers

IMHO, attachment 71657 [details] [diff] [review] is for this case the better solution to provide
a portable always working libjar.
(based on content of the structs and how they are used in the code).

Comment 21

[Roland Mainz]

I vote for the portable solution - the other one is just a hack which won't work
for non-gcc compilers...

Comment 22

[Jeroen Dobbelaere]

Attachment: Updated proposed patch : decode zipfile independent of sizeof(ZipEnd) etc.

new version of attachment 71657 [details] [diff] [review] : update to current cvs

Comment 23

[Jeroen Dobbelaere]

Would it be possible to get this reviewed and fixed for 1.0 ?
This is the only remaining problem that needs to be
fixed in order to get a working build for arm-linux.

(Other problems were :
 bug 9519 and 
 bug 106864 (patch is applied but bug is not marked yet as resolved)
)

Comment 24

[Daniel Veditz [:dveditz]]

I'll add it to my review queue, should be 24hrs or so.

Comment 25

[Daniel Veditz [:dveditz]]

Sorry for the delay, it's been a crazy week.

I'd like comments in the .h file where you define the struct sizes explaining
why you're doing what looks like an ugly, crazy thing instead of a simple
sizeof, so some helpful soul doesn't "fix" it in the future.

Something along the lines...

// sizeof <struct> includes padding on Linux ARM and others; see bug 87965

r=dveditz

Comment 26

[Jeroen Dobbelaere]

Attachment: Final proposal - replace sizeof(struct) with #define

This is an update of attachment 73687 [details] [diff] [review], with some more comments explaining
the need of a #define in stead of a sizeof(...).

mozilla-seamonkey (cvs version) has been compiled with it (for arm) and
poses no problems.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 76550 [details] [diff] [review]
Final proposal - replace sizeof(struct) with #define

r=dveditz

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 76550 [details] [diff] [review]
Final proposal - replace sizeof(struct) with #define

sr=[e-mail] -- file format defines should not depend on struct
sizeof (period, bbaetz! :-).

Let's get this in for 1.0.

/be

Comment 29

[Jeroen Dobbelaere]

Mozilla-1.0rc1 + attachment 76550 [details] [diff] [review] works for me on our xscale platform.
(using gcc-3.0.4+float reload patch)

Comment 30

[timeless]

we should get branch approval for this

Comment 31

[timeless]

fixed on trunk
i'll commit the same thing to branch if/when given an a=

Comment 32

[Aleksey Nogin]

According to brad TBox (see
http://tinderbox.mozilla.org/SeaMonkey/warn1020448680.25229.html) this commit
added two warnings:

3-4. modules/libjar/nsZipArchive.cpp:1094 (See 1st of 2 warnings in build log)
Comparison between signed and unsigned

1092
1093     //-- make sure we've read enough
1094     if ( (PRUint32)bufsize < pos + ZIPCENTRAL_SIZE )
1095     {
1096       status = ZIP_ERR_CORRUPT;

Comment 33

[timeless]

this is caused by:
 878 dveditz  1.1    PRInt32  pos = 0L;
we can't simply make it unsigned because of:
 882                 pos = PR_Seek(mFd, 0, PR_SEEK_END);
 883               #ifndef STANDALONE
 884                 if (pos <= 0)
 885               #else
 886                 if (pos || ((pos = ftell(mFd)) <= 0))

Jeroen Dobbelaere: do you want to remove the cast to unsigned?
1094     if ( (PRUint32)bufsize < pos + ZIPCENTRAL_SIZE )

Aleksey Nogin: I can't figure out what the second warning is. My guess would be 
that pos and ZIPCENTRAL_SIZE aren't of the same type, but ...

Comment 34

[Jeroen Dobbelaere]

Attachment: Remove two warnings, introduced by attachment 76550

This patch removes the two warnings which were introduced by attachment 76550 [details] [diff] [review]
(sorry for that :( )
Note : attachment 76550 [details] [diff] [review] must be applied first.

More specifically :
This patch removes the cast to '(PRUint32)' on line 1094
And it adds a cast to '(int)' of variable status in line 1104 :

1101 #if defined(DEBUG)
1102   if (status != ZIP_OK) {
1103	 const char* msgs[] = { "ZIP_OK", "ZIP_ERR_GENERAL", "ZIP_ERR_MEMORY",
"ZIP_ERR_DISK", "ZIP_ERR_CORRUPT", "ZIP_ERR_PARAM", "ZIP_ERR_FNF",
"ZIP_ERR_UNSUPPORTED", "ZIP_ERR_SMALLBUF", "UNKNOWN" };
1104	 printf("nsZipArchive::BuildFileList  status = %d '%s'\n", (int)status,
msgs[(status <= 0 && status >= -8) ? -status : 9]);
1105   }
1106 #endif

Comment 35

[timeless]

i'll use PRInt32 instead of int, but sure that looks fine w/ me.

i wonder if brendan has a policy on trivial warning fixes.

Comment 36

[Jeroen Dobbelaere]

Using PRInt32 probably won't work : the second warning is there 
because the string format '%d' is matched against the parameter status
(of type PRInt32). Depending on the header files and the target architecture,
this can be converted into 'long', 'int' or even 'short' (on 64bit architectures)
To be sure that gcc doesn't give a warning, it should be converted explicitely
to 'int' in order to match %d), or to long (together with changing %d into %ld).

Comment 37

[timeless]

Comment on attachment 82378 [details] [diff] [review]
Remove two warnings, introduced by attachment 76550 [details] [diff] [review]

ok

Comment 38

[Asa Dotzler [:asa]]

Comment on attachment 82378 [details] [diff] [review]
Remove two warnings, introduced by attachment 76550 [details] [diff] [review]

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 82378 [details] [diff] [review]
Remove two warnings, introduced by attachment 76550 [details] [diff] [review]

sr=jst

Comment 40

[Daniel Veditz [:dveditz]]

*** Bug 152953 has been marked as a duplicate of this bug. ***