575746 - Use strchr() instead of strstr() in NSS code

Status: RESOLVED FIXED

(NSS :: Tools, defect, P4)

Description

[Steve Snyder]

Attachment: Replace 2 calls to strstr() with equivalent strchr() calls

A trivial patch to provide a trivial performance improvement: strstr() needs to validate 2 pointers where strchr() only needs to validate one.

Patch diff'd against NSS v3.12.6.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Elio, thanks for your review.  Now, please commit the patches you reviewed.

Comment 2

[Elio Maldonado]

Only the first part, the one for ... /nss/cmd/signtool/javascript.c, is actually needed. The current version of ./mozilla/security/nss/lib/jar/jarver.c has no calls to PORT_Strstr at all. The cvs log doesn't help me understand the things as it shows the last changelog dated 2010/04/25 which is earlier than the 2010-06-29 date in the diff Steve Snyder made for the submitted patch. I will commit the first part but I wonder how that second part came about.

Comment 3

[Steve Snyder]

Both Firefox versions 3.6.7 and 4.0b1 contain that reference to PORT_Strstr() that I changed.  (In each case from the official source tarball.)

Maybe this is a Firefox-specific modification?

Ah,  here we go: http://mxr.mozilla.org/mozilla1.9.2/source/security/nss/lib/jar/jarver.c#1130

Comment 4

[Elio Maldonado]

Ah, that explains it. Your patch was done with a diff between an original directory and the modified one. I bet had you created the patch via
csv diff -Nup .../mozilla/security/nss > mychanges.path
it would have provided needed information about brances and revisions.

I'll check in the first half in the trunk. Once Firefox synchs. up with our upcoming 3.12.6_RTM (or may Beta's ir RC's) it will pick up your fixes.
Thank you Steve for the clarification and the patch.

Comment 5

[Elio Maldonado]

Checking in javascript.c;
/cvsroot/mozilla/security/nss/cmd/signtool/javascript.c,v  <--  javascript.c
new revision: 1.8; previous revision: 1.7

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Elio, 
You should not give r+ to patches that do not apply cleanly to the NSS source
code on the cvs trunk, or in the NSS 3_13 branch on CVS.  Lots of people 
submit patches against NSS source code in downstream repositories.  You 
should not r+ such review requests, IMO.

Comment 7

[Steve Snyder]

(In reply to comment #6)
> Elio, 
> You should not give r+ to patches that do not apply cleanly to the NSS source
> code on the cvs trunk, or in the NSS 3_13 branch on CVS.  Lots of people 
> submit patches against NSS source code in downstream repositories.  You 
> should not r+ such review requests, IMO.

In the interest of correctness, would you like me to submit a revised patch?

Comment 8

[Elio Maldonado]

(In reply to comment #6) Yes, I that was wrong on my part.  I shown have known better that code inspection even of simple and clear patches isn't enough. I should have verified the patch applies correctly throughout, compiles and even run the suite myself before approving it.

Comment 9

[Elio Maldonado]

(In reply to comment #7)
> In the interest of correctness, would you like me to submit a revised patch?

In the interest of correctness yes. The changes I made are here
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/security/nss/cmd/signtool&command=DIFF_FRAMESET&file=javascript.c&rev1=1.7&rev2=1.8&root=/cvsroot

Comment 10

[Steve Snyder]

Attachment: Revised to change only 1 instance of strstr()

Per connect #9.

Comment 11

[Elio Maldonado]

(In reply to comment #10) Matches changes committed. Thanks.

Comment 12

[Steve Snyder]

That should have been:

"Per comment #9"

Three incorrect letters out of six.  That's not so bad, is it?  Sigh.