Bug 1099619

Created attachment 898508 [details]
changes to rebase nss-util to nss-3.16.1

Created attachment 898509 [details]
all changes to rebase nss to nss-3.16.1

Easy to apply but it's bit hard on the eyes. I can split off the nss.spec file and and other changes out for ease of review.

Temporarily working on a private shared branch. If interested, you can get the nss-util and nss sources with:
git clone --branch private-emaldona-bz1099619 nss-util
git clone --branch private-emaldona-bz1099619 nss

Created attachment 898716 [details]
spec file changes for rebase to 3.16.1

Comment on attachment 898509 [details]
all changes to rebase nss to nss-3.16.1

or completeness sake all changes, including removal of patches and adjustements to patches which are hard to inspect. I split off the spec file portion in the other attachment.

Comment on attachment 898508 [details]
changes to rebase nss-util to nss-3.16.1

r+ NOTE: I did not personally check that the rebase contained the patches you removed.

Comment on attachment 898509 [details]
all changes to rebase nss to nss-3.16.1

r+ rrelyea

(In reply to Bob Relyea from comment #6)
> Comment on attachment 898508 [details]
> changes to rebase nss-util to nss-3.16.1
> 
> r+ NOTE: I did not personally check that the rebase contained the patches
> you removed.

Thank yuo Bob for the prompt review. The rebase does indeed renders those patches obsolete. Here is the rundown.

Deleted patches:
#	deleted:    add-missing-option-descriptions.patch
Upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=932001 - RESOLVED FIXED
  
#	deleted:    disable-ocsp-stapling-tests.patch
Because (Remove OCSP stapling tests that rely on external servers)
 https://bugzilla.mozilla.org/show_bug.cgi?id=936778 - RESOLVES FIXED (3.15.4) 

#	deleted:    dont-disable-internal-module.patch
 https://bugzilla.mozilla.org/show_bug.cgi?id=977673 

#	deleted:    nss-ecc-list-3.15.3.patch
#	deleted:    nss-util-ecc-list-3.15.3.patch
Upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=977673 - RESOLVED FIXED

Let me highlight something which is very easy to miss in a review.
# Disable hw gcm on RHEL5-based build environments where older OS lacks support
Patch63: disable_hw_gcm.patch
...
#%patch63 -p0 -b .hw_comp

I temporarily disabled as it no loger applies and needs some thought as I had a discussion with Wan-Teh upstream were he had different ideas. I can't find the bug now which was probably resolved as duplicate of another one. There were some changes coming from Julien Pierre on https://bugzilla.mozilla.org/show_bug.cgi?id=979132 as he ran into similar problems as we did. I need to discuss that a bit with you once I gather all the needed info. Stay tuned.

(In reply to Elio Maldonado Batiz from comment #11)

> Let me highlight something which is very easy to miss in a review.
> # Disable hw gcm on RHEL5-based build environments where older OS lacks
> support
> Patch63: disable_hw_gcm.patch
> ...
> #%patch63 -p0 -b .hw_comp
> 
> I temporarily disabled as it no loger applies and needs some thought as I
> had a discussion with Wan-Teh upstream were he had different ideas. I can't
> find the bug now which was probably resolved as duplicate of another one.
> There were some changes coming from Julien Pierre on
> https://bugzilla.mozilla.org/show_bug.cgi?id=979132 as he ran into similar
> problems as we did. I need to discuss that a bit with you once I gather all
> the needed info. Stay tuned.

Bob, I finally found the information I was looking for. Wan-Teh's comments are in https://bugzilla.mozilla.org/show_bug.cgi?id=941690#c1 where he states that the patch, which as I stated above cannot be applied after the rebase to 3.16.1, is not needed and using NSS_DISABLE_HW_AES=1 should be sufficient.

Created attachment 903189 [details]
Additional changes needed due the rebase

Remove disable_hw_gcm.patch and use NSS_DISABLE_HW_AES=1 per upstream recommendation by wtc. See Comment 18.

Additional informatiom, The NSS_DISABLE_HW_AES=1 part is not needed. The brew builds work fine without it. I confirmed with release engineering that they are still using RHEL-5 based hosts for the builders.

Comment on attachment 903189 [details]
Additional changes needed due the rebase

HW_AES and HW_GCM are different issues. HW_AES works fine on RHEL 5.

IIRC, the hw_gcm patch was applied upstream already. In the meantime someone updated upstream so that it didn't need it.

These are all irrelevant because the code in question is in softoken, which should not be rebased in this bug (softoken needs to be 3.14.x for FIPS reasons, we are not validating 3.16.x).

bob

OK, this is RHEL-6 not RHEL-5. The patch appears to have been added simply to deal with running local tests inside nss and not in any code that actually ships.
Upstream has updated softoken so it's not necessary to explicitly turn it off (though we should get back to wtc and suggest that we still want to be able to do it). 

Turning off AES_HW is fine because it's just the NSS tests, not the softoken tests (which run separately).

The confusion seems to come because we still have a full softoken in the tree even though we don't ship with it.

bob

Created attachment 913569 [details]
backport upstream fix applied for 3.16.2

upstream bug is https://bugzilla.mozilla.org/show_bug.cgi?id=963150 and its fix is required by Firefox 31.

Created attachment 928882 [details]
fix regresssion caused by the previous fix

This supplementary patch by Bob Relyea fixes the regression introduced by the fix for the race condition. I will attach next a revised patch that merges the two into one and is more suitable for submission upstream.

Created attachment 928886 [details]
Revised race condition patch that doesn't cause libpem deallock

Created attachment 928887 [details]
Changes to the spec file in patch format

Comment on attachment 928886 [details]
Revised race condition patch that doesn't cause libpem deallock

r+ rrelyea

Comment on attachment 928887 [details]
Changes to the spec file in patch format

r+ rrelyea

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2014-1378.html