487007 - Make lib/jar conform to NSS coding style, eliminate Win16 hacks, and fix gross bugs

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

While looking into Julien's bug 485145, I decided to do somthing that should 
have been done long ago, namely, to run nss/lib/jar through a c language 
beautifier.  So I did so.  But my version of "cb" doesn't work very well
so I had to go through afterwords and try to clean up more things by hand.

In the process, I found LOTS of awful bugs, not the least of which is that 
the code uses CERTCertificate handles without any regard to reference counts.
It doesn't call CERT_DestroyCertificate after getting a new reference, and 
it doesn't call CERT_DupCertificate in places where it should.  

So, I tried to clean it up somewhat.  I also eliminated a LOT of Win16 crap.
The result is a rather large patch that I will attach shortly.  

I have also prepared a patch for one of the causes of bug 485145.  
That patch depends on this cleanup patch being applied first.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Monster patch v1

It's a monster, but it's mostly just formatting.

Comment 2

[Julien Pierre]

Nelson,

I wish you had separate patches for the formatting and the bug fixes. I would rather check in the formatting changes first, and then reviewing the code changes separately. It will take longer to review this way. Also, given the problems I saw with my testing last week, in comment https://bugzilla.mozilla.org/show_bug.cgi?id=485145#c16, when I had this patch plus the other one, I think I will need to test this patch on its own first before giving a positive review.

Comment 3

[Julien Pierre]

Attachment: Same patch, but cvs diff -uw , shorter and easier to review

Comment 4

[Julien Pierre]

Comment on attachment 371204 [details] [diff] [review]
Monster patch v1

Most of the patch is good, but there are a couple of issues. I gave my feedback to Nelson over IM.

A very significant portion of this patch is to dead code, unfortunately.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: supplemental patch, v1

Julien, rather than give you another monster patch to review, here is a 
supplemental patch, to be applied on top of (after) the preceding patch.
In addition to this supplemental patch, I would cvs remove jarjart.[ch]

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: supplemental patch, v1 - slightly different format

Bosai's patch viewer tool was clearly confused by that last patch.
It was an RCS diff, as is this patch, but maybe this patch will work better.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: supplemental patch v1, format 3

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 374617 [details] [diff] [review]
supplemental patch v1, format 3

OK, bonsai likes this one. 
Julien, please review this tiny supplemental patch.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Monster patch v2

With any luck, this monster patch will be comparable to the previous monster
patch.  The interdiff between them should patch the "supplemental patch v1, format 3" above, except that this monster diff does NOT contain the diffs to
manifest.mn, because that would break the interdiff.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 374617 [details] [diff] [review]
supplemental patch v1, format 3

Removing review request from this patch. Julien would prefer the interdiff of the two monster patches.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 374626 [details] [diff] [review]
Monster patch v2

This interdiffs well with Monster patch v1.  It contains the changes you requested, plus some additional changes to jarsign.c that should have been in Monster patch 1 but weren't.

Comment 12

[Julien Pierre]

Comment on attachment 374626 [details] [diff] [review]
Monster patch v2

Nelson,

Thanks for making this reviewable with interdiff.

Unfortunately, there are a couple problems with the changes.

a) The new ifdef _X86_ logic in jarfile.c is incorrect.

Firstable, I think it is reversed - you want to use the defines when not on X86.

Second, and more importantly, the _X86_ symbol doesn't really cover the platforms you want. We went over this recently, but only for freebl. We didn't make changes to coreconf, so there is no symbol you can use in lib/jar that means "32-bit X86". The symbol you want from freebl would be NSS_X86 . If you want to use that, then we should probably move those definitions out of freebl and into coreconf.

However, since this is really a cosmetic issue, I would suggest we don't really need the ifdef urgently. 

Just keeping these function names the way they are, with a comment, is probably sufficient. Or another function name  that isn't platform-specific, + comment.

b) thanks for fixing the magic numbers issue. I wasn't aware that we had PR_STATIC_ASSERT. I would have used it in several other places to deal with this if I knew.

However, I'm not confident about one of the other changes you made in jar_listtar :

sz = PR_ROUNDUP(sz,sizeof tarball);

I think this is only an exact replacement if sz < 512 coming in . If sz is for example 513, the in the existing code it would be incremented to 1024, but in your code it would remain at 513. Since sz is apparently initially read from the tar file, I don't think we can assume that it is always within the range of 0-511.
I think one valid replacement would be :

sz = (sz / sizeof tarball + ((sz % sizeof tarball) ?1:0 ) ) * sizeof tarball;

The rest looks fine.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 374626 [details] [diff] [review]
Monster patch v2

Julien, You had 3 issues with the patch. I want to address them all.

1) You wrote:
> I think it is reversed - you want to use the defines when not on X86.

It is definitely not reversed.

>+#ifdef _X86_
>+#define x86ShortToUint32(ii)   ((const PRUint32)*((const PRUint16 *)(ii)))
>+#define x86LongToUint32(ii)    (*(const PRUint32 *)(ii))
>+#else
>+static PRUint32
>+x86ShortToUint32(const void *ii);
> 
>-static unsigned long xtolong (unsigned char *ll);
>+static PRUint32
>+x86LongToUint32(const void *ll);
>+#endif

Among our existing supported platforms, only on an x86 family CPU can we take 
an unaligned address in memory and use it as an address of a ushort or ulong 
and fetch it without a crash, and have it be a little-endian fetch.  On all 
other platforms, an unaligned address would crash, and on most, it would be a 
big-endian fetch, so the function is necessary to simulate the x86 behavior.

2. I'm aware that _X86_ is 32-bit only.  I chose it on purpose.
The new symbol NSS_X86_OR_X64 is only defined in freebl.  It is beyond the 
scope of this bug to implement it for all of NSS.  Use of the function is
not incorrect on x86_64.  Presently, we use it on all platforms.  The ifdef
is correct, as far as it goes, and can be made to be used further in another
patch.

3. You believe the following substitution is incorrect when sz > 512.

>-    sz += 511;
>-    sz = (sz / 512) * 512;
>+	/* Advance to next file entry */
>+	sz = PR_ROUNDUP(sz,sizeof tarball);

It is correct.  it expands to 

    sz = ((((sz)+((sizeof tarball)-1))/(sizeof tarball))*(sizeof tarball));

Since sizeof tarball is 512, if we substitute 512 for it, we see it is

    sz = ((((sz)+(512-1))/512)*512);

which produces an identical result to the old code.
Please reconsider your review in light of these facts.

Comment 14

[Julien Pierre]

Nelson,

1. You are right, the logic is not reversed.

2. The _X86_ symbol is now defined only a single platform in coreconf, Windows 32 bit, and nowhere else. It is also never used anywhere in NSS, and should never be used in NSS anymore. If you really want to skip the unnecessary conversion function only on Windows 32 bit, but still go through it on other 32 bit x86 platforms, please use
#ifdef WIN32
which will have the same effect as
#ifdef _X86_
but will be much more understandable to readers of the code about what the test really does.

I would prefer that we either have this optimization for all x86 platforms, or that we don't have it at all, but if we want to special-case Windows, then let's do it in an obvious way as above.

May I also suggest you rename 
x86ShortToUint32 and x86LongToUint32 to unalignedShortToUint32 and unalignedLongToUint32 ? I think this would also make the code easier to understand.

3. Sorry, I didn't review the definition of PR_ROUNDUP. You are right.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Julien, 
WIN32 is also defined for WinCE which runs on ARM CPUs which do not have the
memory access properties of X86 family CPUs, so it would be incorrect to use
WIN32 as the ifdef for these macros.  

The functions are not unly for unaligned but also little endian, so if we
wanted to give them a fully descriptive name it would be 
unalignedLittleEndianLongToUint32, which is too long.  
As there is only one CPU family we support with both of those properties, 
it is appropriate to identify these macros that simulate that CPU family's 
behavior with the name of that family.  

The symbol _X86_ is defined by the gcc compilers that come with cygwin and 
MingW.  It is also defined in one of our makefiles so that it is ALSO 
defined for MSVC builds.  

I agree that it will be good to expand the scope of NSS_X86_OR_X64 to be 
present throughout NSS and not only in freebl, and when we do that, we
can change this ifdef to use it.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Here's an interesting comment I found on MSDN (of all places!)
> For gcc, there's a flag you can use: gcc -dM -E - </dev/null will list 
> all of gcc's builtins, regardless of the gcc version/platform/etc.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

After consulting http://predef.sourceforge.net/prearch.html#sec6 and
http://code.google.com/p/nvidia-texture-tools/source/browse/trunk/extern/poshlib/posh.h?spec=svn841&r=841#72
I propose to use this:

#if defined __X86__ || defined i386 || defined __i386 || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__ || defined _M_IX86 || defined __386__ || defined __x86_64__ || defined __x86_64 || defined __amd64__ || defined __amd64 || defined _M_X64

and 

#if !(defined __X86__ || defined i386 || defined __i386 || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__ || defined _M_IX86 || defined __386__ || defined __x86_64__ || defined __x86_64 || defined __amd64__ || defined __amd64 || defined _M_X64)

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Monster patch v3 - different ifdef for X86 family (checked in)

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 374710 [details] [diff] [review]
Monster patch v3 - different ifdef for X86 family (checked in)

This interdiffs nicely with the two preceding monster patches.

Comment 20

[Julien Pierre]

Comment on attachment 374710 [details] [diff] [review]
Monster patch v3 - different ifdef for X86 family (checked in)

r+, with the condition that you copy the section starting at 
http://mxr.mozilla.org/security/source/security/nss/lib/freebl/Makefile#80
through line 90 to the libjar Makefile, and test one of these macros instead .

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

I'm committing this in steps.  I will start by removing the dead code.

Checking in manifest.mn; new revision: 1.7; previous revision: 1.6
Removing jarjart.c;      new revision: delete; previous revision: 1.7
Removing jarjart.h;      new revision: delete; previous revision: 1.2

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Step 2: the rest of the .c and .h changes.  

Checking in jar-ds.c;  new revision: 1.3;  previous revision: 1.2
Checking in jar-ds.h;  new revision: 1.3;  previous revision: 1.2
Checking in jar.c;     new revision: 1.5;  previous revision: 1.4
Checking in jar.h;     new revision: 1.6;  previous revision: 1.5
Checking in jarfile.c; new revision: 1.11; previous revision: 1.10
Checking in jarfile.h; new revision: 1.3;  previous revision: 1.2
Checking in jarint.c;  new revision: 1.3;  previous revision: 1.2
Checking in jarint.h;  new revision: 1.6;  previous revision: 1.5
Checking in jarnav.c;  new revision: 1.4;  previous revision: 1.3
Checking in jarsign.c; new revision: 1.7;  previous revision: 1.6
Checking in jarver.c;  new revision: 1.16; previous revision: 1.15
Checking in jzconf.h;  new revision: 1.2;  previous revision: 1.1

Step 3 will be the Makefile change Julien requested.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Attachment: supplemental patch 2, v1

This patch adds the requested makefile change, and also adds one more 
PR_STATIC_ASSERT, as we discussed.

Comment 24

[Julien Pierre]

Comment on attachment 374860 [details] [diff] [review]
supplemental patch 2, v1 

This patch looks fine, but please also replace the check for compiler macros in jarfile.c with a check for one of these NSS_ macros.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Checking in config.mk; new revision: 1.4;  previous revision: 1.3
Checking in jarfile.c; new revision: 1.12; previous revision: 1.11

Julien, the other changes you wanted have already been previously committed.
See 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/jar/jarfile.c&rev=1.11&mark=107#106

Comment 26

[Serge Gautherie (:sgautherie)]

Fwiw, there are (only) 3 remnants:
{
/security/nss/lib/jar/jarfile.c
    * line 268 -- * in one bulk calloc. (Necessary under Win16 platform.)
/security/nss/lib/jar/jarsign.c
    * line 67 -- * This version supports huge pointers for WIN16.
/security/nss/lib/jar/jarver.c
    * line 224 -- /* don't expense unneeded calloc overhead on non-win16 */
}