1308104 - Replace PL_strchr with a safer Gecko string class or function

Status: RESOLVED FIXED

(Core :: XPCOM, task, P5)

Description

[Chris Peterson [:cpeterson]]

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

1. Replace PL_strchr() and PL_strrchr() calls with strchr() and strrchr(), respectively:

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

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

*** CAUTION: PL_strchr() and PL_strrchr() will return null without without crashing when passed a null pointer, but strchr() and strrchr() will crash! Any calls to PL_strchr() and PR_strrchr() 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 strchr() or strrchr() 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_strstr() for strstr().

Comment 2

[Nicklas Boman [:smurfd]]

Hey
I have looked at this abit. Have some followup questions.

This "safer Gecko string class" is that something that exists today, its a 4 year old bug so maby it exists :) ?

And im guessing, otherwise, to be sure, its better to add a nullptr check for the argument of strchr()?

Comment 3

[Chris Peterson [:cpeterson]]

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

This "safer Gecko string class" is that something that exists today, its a 4 year old bug so maby it exists :) ?

And im guessing, otherwise, to be sure, its better to add a nullptr check for the argument of strchr()?

Unfortunately, I don't think such a "safer Gecko string class" exists today. :)

Looks like there is only one call to PL_strrchr (outside of nsprpub/ and security/nss/) and it's in NPAPI plugin code (dom/plugins/base/nsPluginTags.cpp) that will go away when Firefox drops support for Flash later this year:

https://searchfox.org/mozilla-central/search?case=true&q=PL_strrchr

So we should not bother removing that PL_strrchr. I renamed this bug to just be about replacing PL_strchr:

https://searchfox.org/mozilla-central/search?case=true&q=PL_strchr

These PL_strchr calls can be replaced with strstr if the surrounding code is audited to make sure a NULL pointer can't be passed to the function. And if it can, then a NULL check should be added. I spot checked a couple PL_strchr calls and they had NULL checks, so replacing PL_strchr should be pretty straightforward. I don't see a reason to wait for that mythical "safer Gecko string class".

Looks like the PL_strchr calls are in four modules and the replacements should be split into separate patches (and probably separate bugs in the appropriate components blocking this meta bug). Appropriate code reviewer for the modules:

• dom/plugins/ code will go away soon, so don't bother changing these calls. :)
• layout/ code reviewer = r?#layout-reviewers
• netwerk/ code reviewer = r?#necko-reviewers
• toolkit/components/commandlines/ code reviewer = I don't know but maybe try r?gijs because he is a module owner for the Toolkit module.

Comment 4

[Nicklas Boman [:smurfd]]

Attachment: Bug 1308104 - Replace PL_strchr with a safer Gecko string class or function commandlines/ r?froydnj

Comment 5

[Nicklas Boman [:smurfd]]

Attachment: Bug 1308104 - Replace PL_strchr with a safer Gecko string class or function netwerk/ r?#necko-reviewers

Comment 6

[Nicklas Boman [:smurfd]]

Attachment: Bug 1308104 - Replace PL_strchr with a safer Gecko string class or function layout/ r?#layout-reviewers

Comment 7

[Nicklas Boman [:smurfd]]

Just want to double check, i wont be able to remove the definition and anything from the .h files regarding PL_strchr() since there still is calls from :
nsprpub/ and security/nss
So the above patches should be good, right?

Comment 8

[Chris Peterson [:cpeterson]]

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

Just want to double check, i wont be able to remove the definition and anything from the .h files regarding PL_strchr() since there still is calls from :
nsprpub/ and security/nss
So the above patches should be good, right?

Correct. We don't want to change any files in the nsprpub or security/nss directories in mozilla-central. NSPR and NSS are libraries used by non-Mozilla projects and are officially maintained in a CVS source repo outside of mozilla-central.

You can submit patches for NSPR and NSS code through Bugzilla, but the patch and review process takes extra work. I wouldn't bother for this bug. I see that NSS code uses a lot of other PL_str* functions, so there is little value in removing just these few PL_strchar calls.

https://searchfox.org/mozilla-central/search?q=PL_str&path=nss&case=true

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c8433be706e
Replace PL_strchr with a safer Gecko string class or function layout/ r=emilio

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5c1875c11c7
Replace PL_strchr with a safer Gecko string class or function netwerk/ r=necko-reviewers,dragana

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e78791b93526
Replace PL_strchr with a safer Gecko string class or function commandlines/ r=froydnj

Comment 12

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on nsHttp.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/f3a40f0de7b98ed50d5c30050bf206fc5c72b136
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315350731&repo=autoland&lineNumber=24998

Comment 13

[Chris Peterson [:cpeterson]]

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315350731&repo=autoland&lineNumber=24998

[task 2020-09-10T18:58:46.362Z] 18:58:46    ERROR -  /builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttp.cpp:855:13: error: cannot initialize a variable of type 'char *' with an rvalue of type 'const char *'
[task 2020-09-10T18:58:46.362Z] 18:58:46     INFO -        char* p = strchr(buf.get(), ' ');
[task 2020-09-10T18:58:46.362Z] 18:58:46     INFO -              ^   ~~~~~~~~~~~~~~~~~~~~~~
[task 2020-09-10T18:58:46.362Z] 18:58:46     INFO -  1 error generated.

PL_strchar and the standard C library's strchr return non-const char* (pointing into the const char* parameter buffer, thus breaking const-correctness). Looks like the standard C++ library has both const and non-const overrides of strchr:

https://en.cppreference.com/w/cpp/string/byte/strchr

To work around this compilation error, you can probably cast the buf.get() argument to a non-const pointer like char* p = strchr(const_cast<char*>(buf.get(), ' ');, unless there is some cleaner way to get a non-const pointer to nsAutoCString buf's internal char buffer.

Comment 14

[Nicklas Boman [:smurfd]]

Thanks, should have written here yesterday that i had seen the error and that the changes got backed out.
What im kindof fuzzy about is why this was not caught during my local builds...
But having slept on it, im guessing it has todo with buf.get() beeing a nsAutoCString, the two overloaded functions of strchr() and that in automation you are able to force that error.

const char* aa = "X";
char* cc = strchr(aa, 'X');

Would cause an error: cannot initialize a variable of type 'char *' with an rvalue of type 'const char *' when using gcc to manually compile...
Will fix most likely with casting as you suggested. Got some suggestions in the Phabricator revison so will look over that aswell.

Comment 15

[Chris Peterson [:cpeterson]]

I don't know why a local build would not catch this error.

Do you have ac_add_options --enable-warnings-as-errors in your mozconfig file? Compiling with warnings as errors is strongly recommended, but shouldn't matter in this case because this const/non-const assignment is a C++ compilation error, not just a warning.

Comment 16

[Nicklas Boman [:smurfd]]

Hey Chris, do you think its okey to mark https://phabricator.services.mozilla.com/D89326 as "checkin-needed" since i made a small change on it to avoid breakage?

Comment 17

[Chris Peterson [:cpeterson]]

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

Hey Chris, do you think its okey to mark https://phabricator.services.mozilla.com/D89326 as "checkin-needed" since i made a small change on it to avoid breakage?

LGTM. You can mark your revision as "checkin-needed".

Heads up, though: since your patch was last updated over three months ago and mozilla-central code changes a lot, your checkin might hit a merge conflict if it overlaps with someone else's code change. This probably won't happen and if it does, nothing bad will happen. You'll just get a Phabricator message ask you to rebase your revision, manually fix the merge conflict, and submit the update to Phabricator.

Comment 18

[Nicklas Boman [:smurfd]]

Thanks for the heads up, i tried to apply it locally first, and it did indeed complain :)
So i will fix that, it was a #include in netwerk/base/nsStandardURL.cpp that tried to jump in on a now occupied line. so it was close.
Then when i see that i dont get any complaints from the Bots, i will mark it as checkin-needed

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16f5de55a746
Replace PL_strchr with a safer Gecko string class or function netwerk/ r=necko-reviewers,dragana,valentin

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/16f5de55a746