Closed Bug 435959 Opened 16 years ago Closed 16 years ago

Firefox 3 RC 2 should take NSS 3.12 RC 4

Categories

(Core :: Security: PSM, defect)

1.9.0 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

(Whiteboard: [RC2+])

Attachments

(1 file)

Bug 433386 is a topcrasher in FF 3 RC 1, when visiting secure sites, a double memory free.

We have a minimal one line patch, with 2 reviews, the patch avoids using already-freed data. The suppressed code path would have updated a cache twice, so I don't see a risk in functionality.

It depends on bug 433594, which has another one-line fix, a trivial check for non-null.

The NSS team has prepared a special 3.12.0 release branch, and is willing to land both patches and produce a new RC 4.

Approval in this bug means: Update the tag in mozilla/client.mk

The two mentioned bugs would be the only changes since the NSS snapshot currently being used by FF 3 RC1.
Flags: blocking1.9?
Regarding testing, I have compiled Firefox and NSS locally, i was able to reproduce the crash, and the patches work for me.
Whiteboard: [RC2?]
Note we are not going to commit these fixes to the NSS 3.12.0 release branch 
until we have a commitment from Mozilla that they will take these fixes.
Well, the clock is ticking.  
I guess mozilla doesn't want these fixes very much.
(In reply to comment #3)
> Well, the clock is ticking.  
> I guess mozilla doesn't want these fixes very much.

It's past midnight for half the 1.9 drivers, and they have been super-busy all day. I'm sure they will get to this tomorrow.
Can you attach the exact diff between those tags?  Has anyone tested builds with the resulting NSS?  It sounds like you aren't going to bother committing it until we approve it, but I don't really like approving code that hasn't been tested in the final config at this stage.

Nelson: the clock had been ticking for all of 8 hours, most of them pretty late where most of the drivers are, and there was nothing other than the "blocking1.9?" flag setting to find it by, which requires an active search -- not even a driver cc'd on the bug, let alone an email indicating that there was a very-last-minute request for a change to our security software.  I guess you don't want "mozilla" to take these fixes very much? :(
(In reply to comment #5)
> Can you attach the exact diff between those tags?

The tag does not yet exist.
The exact diff are attachment 321998 [details] [diff] [review] and attachment 321538 [details] [diff] [review] combined, plus version number increment, nothing else.


> Has anyone tested builds with the resulting NSS?

I've applied and tested locally, that's what I meant in comment 1.

For testing purposes (only) I've also applied attachment 321965 [details] [diff] [review], so I don't need to change system time.

I offer to do testing with both the time-warp-patch applied and not-applied.


> It sounds like you aren't going to bother committing
> it until we approve it, but I don't really like approving code that hasn't been
> tested in the final config at this stage.

Can you please describe what environment you see suitable to match your "final config" expectation?
Is a local build of cvs trunk with those tiny patches applied a good test?


> Nelson: the clock had been ticking for all of 8 hours, most of them pretty late
> where most of the drivers are, and there was nothing other than the
> "blocking1.9?" flag setting to find it by, which requires an active search --
> not even a driver cc'd on the bug, let alone an email indicating that there was
> a very-last-minute request for a change to our security software.

Sorry, that was my fault.
I had expected that setting the flag would be sufficient, in addition I had mentioned the request on #granparadiso.
Kaie, please test ASAP and let us know the results. A local build is a good test, but having a testsuite of NSS use cases which you could run it against and show that other than this fix we're looking at an idempotent change would be better. Don't know if that exists for NSS or not.
As I had said in comment 1 already, I had done testing using a local build, and it worked fine.

During the last 1:30 h I tried to setup mochitests. I succeeded, but after having run mochitests for 30 minutes, I stopped them. All was green up to that point. It appears to me that it would take ages for these tests to complete. And I don't believe that running mochitests is helpful for our scenario.

I really think that our patches are obviously an improvement. I will run some more local tests, that's all I can do.
(In reply to comment #7)
> but having a testsuite of NSS use cases which you could run it against
> and show that other than this fix we're looking at an idempotent change would
> be better. Don't know if that exists for NSS or not.

NSS does indeed have an test suite and it's run as part of NSS own tinderbox.

The proposed patches have been checked in on the trunk of NSS (for 3.12.1) and did not show any regressions or test suite failures.

You are right, I have not explicitly tested the combination of 3.12.0-release-branch and these two mini-patches. I will do so now. Testing will take a couple of minutes.

Please don't let this stop you from approving it.
If the test suite fails, I'll remove the approval and not check in.
So, I've done testing of Firefox current trunk, optimized build.
I made two cycles of my tests, with and without the time-warp patch applied.

I've connected to sites from 6 different CAs.
I used page info / view cert for all of the pages I visited.
No crashes, all worked as expected.

For the pages that rely on recent OCSP for EV, I got the non-green with time-warp patch applied (because as expected OCSP response is too old), and without time-warp patch (trunk timecode) I get the expected EV green.

I'm confident these patches can only do better.
I can not imagine side effects of our null check and the double-free-avoid check.
The NSS test suite completed successfully without failures on the proposed NSS snapshot.
Kaie: thank you so much for your diligence, it really makes the end-game a lot more bearable for people making the last-minute triage calls.

I've marked approval on the two patches in question; please land and advance the NSS tag!
Flags: blocking1.9? → blocking1.9+
Whiteboard: [RC2?] → [RC2+]
Thanks a lot for approving!

Patches and new version number landed, tag created.

cvs rdiff -u -r NSS_3_12_RC3 -r NSS_3_12_RC4 mozilla/security/nss mozilla/security/coreconf mozilla/security/dbm mozilla/dbm

cvs command only shows the expected changes.

mozilla/client.mk changed to pick up the new tag.

Done.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attached patch obvious patchSplinter Review
for the record, this is the obvious patch that "advanced the tag", which I mentioned in comment 0, which shaver approved in comment 12.

It landed exactly the changes that shaver approved in bug 433386 and 433596.
due to the complications of setting up tests, and based on the thorough testing from comment #6, i'm going to mark this bug verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.