1308103 - Replace PL_strcmp/PL_strncmp with strcmp/strncmp

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P5)

Description

[Chris Peterson [:cpeterson]]

We should replace NSPR functions with standard C functions when they available on all platforms.

1. Replace PL_strcmp() and PL_strncmp() calls with strcmp() and strncmp(), respectively:

http://searchfox.org/mozilla-central/search?q=%5CbPL_strn%3Fcmp%5Cb&regexp=true&path=.h%24

http://searchfox.org/mozilla-central/search?q=%5CbPL_strn%3Fcmp%5Cb&regexp=true&path=.cpp%24

*** CAUTION: PL_strcmp() and PL_strncmp() will handle a null pointer without crashing, but strcmp() and strncmp() will crash! Any calls to PL_strcmp() and PR_strncmp() that are changed should be audited to see if they might need to handle a null pointer.

2. Add #include <string.h> if it's needed for strcmp() or strncmp() function declarations.

3. Remove #include "plstr.h" if it's no longer needed for other NSPR string function declarations.

Comment 1

[Chris Peterson [:cpeterson]]

Waldo recommends using a safer Gecko string class or function instead of just swapping PL_strcmp() for strcmp().

Comment 2

[Chris Peterson [:cpeterson]]

We can also replace calls to nsCRT::strcmp with the standard C library function strcmp:

https://searchfox.org/mozilla-central/search?case=true&q=nsCRT%3A%3Astrcmp

Comment 3

[Takanori MATSUURA]

(In reply to Chris Peterson [:cpeterson] from comment #2)

We can also replace calls to nsCRT::strcmp with the standard C library function strcmp:

Is this bug 798840?

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Takanori MATSUURA from comment #3)

(In reply to Chris Peterson [:cpeterson] from comment #2)

We can also replace calls to nsCRT::strcmp with the standard C library function strcmp:

Is this bug 798840?

Looks like it. I didn't know about that bug. Thanks for sharing it!

Comment 5

[Nicklas Boman [:smurfd]]

Attachment: Bug 1308103 - Replace PL_strcmp/PL_strncmp with strcmp/strncmp in netwerk/ r=#necko-reviewers

Comment 6

[Nicklas Boman [:smurfd]]

Attachment: Bug 1308103 - Replace PL_strcmp/PL_strncmp with strcmp/strncmp in dom/events/KeyEventHandler.cpp r=#geckoview-reviewers

Comment 7

[Nicklas Boman [:smurfd]]

Attachment: Bug 1308103 - Replace PL_strcmp/PL_strncmp with strcmp/strncmp in xpcom/ and parser/htmlparser/ r=#xpcom-reviewers

Comment 8

[Nicklas Boman [:smurfd]]

Attachment: Bug 1308103 - Replace PL_strcmp/PL_strncmp with strcmp/strncmp in extensions/pref/autoconfig r=#necko-reviewers

Comment 9

[Nicklas Boman [:smurfd]]

Please let me know if some of the patches got the wrong r= groups... then ill fix.

Also please have a special look in the netwerks/ so that no parameter can have a nullptr value, i have tried to keep an eye on that but some places are abit tricky.

Comment 10

[Mike Kaply [:mkaply]]

Waldo recommends using a safer Gecko string class or function instead of just swapping PL_strcmp() for strcmp().

So are we not doing this?

Comment 11

[Nicklas Boman [:smurfd]]

(In reply to Mike Kaply [:mkaply] from comment #10)

Waldo recommends using a safer Gecko string class or function instead of just swapping PL_strcmp() for strcmp().

So are we not doing this?

As far as i know such "safer Gecko string" functions does not exist. And these days standard strcmp() exists on all platforms, and strcmp() is used in alot of places already throughout the firefox codebase.
@cpeterson please chime in if you have something to add :)

Comment 12

[Andrew McCreight [:mccr8]]

Maybe you could do something like put the two operands into nsDependentCSubstrings and then use the nsString interface to compare them? I don't know if that would be better or not.

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Nicklas Boman [:smurfd] from comment #11)

As far as i know such "safer Gecko string" functions does not exist. And these days standard strcmp() exists on all platforms, and strcmp() is used in alot of places already throughout the firefox codebase.
@cpeterson please chime in if you have something to add :)

In the five years since Waldo suggested using "safer Gecko string" functions, these "Replace PL_str*()" bugs have morphed into "remove Gecko's dependency on NSPR string functions when comparable standard C functions exist".

For string comparisons when we just have char* pointers, I don't think constructing and comparing nsDependentCSubstrings is worth the trouble. strcmp() is admittedly less safe than PL_strcmp() because strcmp() will crash on NULL pointers, but I don't think that's a big risk because Nicklas is verifying the callers handle the NULL pointer case.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ff2b091aed4
Replace PL_strcmp/PL_strncmp with strcmp/strncmp in netwerk/ r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/491be401a9f5
Replace PL_strcmp/PL_strncmp with strcmp/strncmp in dom/events/KeyEventHandler.cpp r=masayuki
https://hg.mozilla.org/integration/autoland/rev/53d8c7b3bb78
Replace PL_strcmp/PL_strncmp with strcmp/strncmp in xpcom/ and parser/htmlparser/ r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/774b99dc859c
Replace PL_strcmp/PL_strncmp with strcmp/strncmp in extensions/pref/autoconfig r=mkaply

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6ff2b091aed4
https://hg.mozilla.org/mozilla-central/rev/491be401a9f5
https://hg.mozilla.org/mozilla-central/rev/53d8c7b3bb78
https://hg.mozilla.org/mozilla-central/rev/774b99dc859c