Bug 976572

Created attachment 763596 [details]
Patch 1 of 5

Created attachment 763597 [details]
Patch 2 of 5

Created attachment 763598 [details]
Patch 3 of 5

Created attachment 763599 [details]
Patch 4 of 5

Comment on attachment 763596 [details]
Patch 1 of 5

r+ rrelyea

Comment on attachment 763597 [details]
Patch 2 of 5

r+ rrelyea

Comment on attachment 763598 [details]
Patch 3 of 5

r+ good, matches gcm.nits in RHEL-5

Comment on attachment 763599 [details]
Patch 4 of 5

r+ rrelyea

good, matches freebl-gcm.patch in RHEL-5.

yes, these are needed --- for FIPS if for no other reason: dev_ack+

Thank you for the prompt review. 

Unfortunately, further testing makes me think there is still "something wrotten in Denmark."

Just did a scratch using the source rpm downloaded from the last brew build - https://brewweb.devel.redhat.com/buildinfo?buildID=273865
wget http://download.devel.redhat.com/brewroot/packages/nss-softokn/3.14.3/4.el6/src/nss-softokn-3.14.3-4.el6.src.rpm

The scratch build failed! 
https://brewweb.devel.redhat.com/taskinfo?taskID=5932642
This is an x86_64 one but a previously one I did failed s390x,  both with a core and in the same test.

cipher.sh: #15: AES GCM (Failed in/out offset pairs:  [0:0][0:1][0:2][0:3][0:4][0:5] ----- skipped .... [7:7]) - Core file is detected - FAILED

By the way, a scratch build with those four patches disabled had no problems,
https://brewweb.devel.redhat.com/taskinfo?taskID=5932673

There is something wrong in one of them.

There is nothing wrong with any of them one ones views them one by one. The problem may be when they are applied one after the other. This is a different conext that what we had upstream in 3.15 whre this patches are being picked up. I consolidated them into one patch and the final patched code is actually different. Then I did a scratch build
https://brewweb.devel.redhat.com/taskinfo?taskID=5932776
It succeeded!

Tried scratch builds again, no changes, and it failed twice.

Exactly the same failure but this time with a RHEL-5.10 scratch build.

A bit of good news for a change. 

Since we never have any failures in fedora that have me clue that the backported patches need to be more faithfull to the original ones on rhel-3.5. So I did another experiment. Created an additional patch that makes the blapitest.c use PLArenaPool instead of PRArenaPool and I am pleased to inform that I am getting good builds mutiple times. 

It's doing the job so far but it may not be the final solution yet.

I found the following comments in nss/util/secitem.h.

/*
** This is a legacy function containing bugs. It doesn't update item->len,
** and it has other issues as described in bug 298649 and bug 298938.
** However, the function is  kept unchanged for consumers that might depend 
** on the broken behaviour. New code should call SECITEM_ReallocItemV2.
**
** Reallocate the data for the specified "item".  If "arena" is not NULL,
** then reallocate from there, otherwise reallocate from the heap.
** In the case where oldlen is 0, the data is allocated (not reallocated).
** In any case, "item" is expected to be a valid SECItem pointer;
** SECFailure is returned if it is not.  If the allocation succeeds,
** SECSuccess is returned.
*/
extern SECStatus SECITEM_ReallocItem( /* deprecated function */
				     PLArenaPool *arena, SECItem *item,
				     unsigned int oldlen, unsigned int newlen);

/*
** Reallocate the data for the specified "item".  If "arena" is not NULL,
** then reallocate from there, otherwise reallocate from the heap.
** If item->data is NULL, the data is allocated (not reallocated).
** In any case, "item" is expected to be a valid SECItem pointer;
** SECFailure is returned if it is not, and the item will remain unchanged.
** If the allocation succeeds, the item is updated and SECSuccess is returned.
 */
extern SECStatus SECITEM_ReallocItemV2(PLArenaPool *arena, SECItem *item,
				       unsigned int newlen);

wtc's comment "New code should call SECITEM_ReallocItemV2." is telling.
This is new code after all. To use it I would need to patch nss-util, a spearte package. That would need a separate bug that would make a blocker a blocker to this one. I seem to be getting closer to a solution but there is a bit more to do analysis-wise.

(In reply to Elio Maldonado Batiz from comment #14)
> Exactly the same failure but this time with a RHEL-5.10 scratch build.

After applying that extra patch, which I haven't showed show you yet, the RHEL-5.10 scratch builds are now working.

Created attachment 763882 [details]
Patch 5 of 5 - switch to using PLArenaPool on blapitest.c

That extra patch that fixes those intermittent core dumps on cipher (blapi) tests.

Comment on attachment 763882 [details]
Patch 5 of 5 - switch to using PLArenaPool on blapitest.c

Are this patches from upstream?

bob

(In reply to Bob Relyea from comment #18)
> Comment on attachment 763882 [details]
> Patch 1 of 5 - switch to using PLArenaPool on blapitest.c
> 
> Are this patches from upstream?
> 
> bob

Yes, Patch 5 is a subset of the last patch from this upstream bug https://bugzilla.mozilla.org/show_bug.cgi?id=802430
See https://bug802430.bugzilla.mozilla.org/attachment.cgi?id=744975

With meld, a convenient way to compare is with 
meld NO_NSPR_10_SUPPORT.patch usePL.patch. 
Click on the "keep Highlighting button" on the top right to make it easier to view.

Today I had again a build failure on a regular brew build of nss for RHEL-5.10. The cause was the same sporadic core dump I have reported earlier, previously seen on scratch builds, and for which I have the fifth patch under consideration. It happens on 64-bit platforms.

From http://download.devel.redhat.com/brewroot/work/tasks/2670/6062670/build.log
...
bltest -T -m aes_gcm -d /builddir/build/BUILD/nss-3.14.3/mozilla/security/nss/tests/../cmd/bltest -1 7 -2 6
./cipher.sh: line 57: 10272 Illegal instruction     (core dumped) ${PROFTOOL} ${BINDIR}/bltest -T -m $PARAM -d $CIPHERTESTDIR -1 $inOff -2 $outOff
bltest -T -m aes_gcm -d /builddir/build/BUILD/nss-3.14.3/mozilla/security/nss/tests/../cmd/bltest -1 7 -2 7
./cipher.sh: line 57: 10275 Illegal instruction     (core dumped) ${PROFTOOL} ${BINDIR}/bltest -T -m $PARAM -d $CIPHERTESTDIR -1 $inOff -2 $outOff
cipher.sh: #15: AES GCM (Failed in/out offset pairs:  [0:0][0:1][0:2][0:3][0:4][0:5][0:6][0:7][1:0][1:1][1:2][1:3][1:4][1:5][1:6][1:7][2:0][2:1][2:2][2:3][2:4][2:5][2:6][2:7][3:0][3:1][3:2][3:3][3:4][3:5][3:6][3:7][4:0][4:1][4:2][4:3][4:4][4:5][4:6][4:7][5:0][5:1][5:2][5:3][5:4][5:5][5:6][5:7][6:0][6:1][6:2][6:3][6:4][6:5][6:6][6:7][7:0][7:1][7:2][7:3][7:4][7:5][7:6][7:7]) - Core file is detected - FAILED
....

Comment on attachment 763882 [details]
Patch 5 of 5 - switch to using PLArenaPool on blapitest.c

r+ probably worth pushing upstream as well.

bob

Created attachment 777876 [details]
gcm fixes - freebl part

Created attachment 777886 [details]
gcm fixes - softoken part

Created attachment 778906 [details]
Print CPU info just before the tests start

Same thing we are doing on rhel-5.10. Good pratcice in case we run run into platform-specific problems.

Comment on attachment 777876 [details]
gcm fixes - freebl part

r+ rrelyea

Comment on attachment 777886 [details]
gcm fixes - softoken part

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-2013-1558.html