1724649 - Followup to remove #include "plstr.h" after Replace PL_ bugs have landed

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Nicklas Boman [:smurfd]]

Hey,

After these bugs land :
https://bugzilla.mozilla.org/show_bug.cgi?id=1308094
https://bugzilla.mozilla.org/show_bug.cgi?id=1308103
https://bugzilla.mozilla.org/show_bug.cgi?id=1724526
https://bugzilla.mozilla.org/show_bug.cgi?id=1308105

would it be ok to remove the #include "plstr.h" from:
https://searchfox.org/mozilla-central/search?q=%23include+%22plstr.h%22&path=&case=false&regexp=false
outside of nsprpub/ and security/

Comment 1

[Chris Peterson [:cpeterson]]

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

After these bugs land :
...
would it be ok to remove the #include "plstr.h" from:
https://searchfox.org/mozilla-central/search?q=%23include+%22plstr.h%22&path=&case=false&regexp=false
outside of nsprpub/ and security/

Thanks for moving these bugs forward! :)

Looks like more work will still be needed after those bugs are fixed.

We will need to replace PL_strcasecmp, PL_strncasecmp, and PL_strcaserstr:

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

Unfortunately, there is no standard C library function for strcasecmp or strcasestr. PL_strcasecmp can be replaced with strcasecmp on Linux and macOS, but Windows uses the nonstandard function _stricmp instead. So we will still need a Gecko function that delegates to strcasecmp on Linux and macOS and _stricmp on Windows. Fortunately, we already have wrappers nsCRT::strcasecmp, nsCRT::strncasecmp, and nsCRT::strcasestr in XPCOM:

https://searchfox.org/mozilla-central/rev/f71cb98fc35da418d2cb9ce31a0416d532dc9d69/xpcom/ds/nsCRT.h#43-78

• Can you please file a bug to replace PL_strcasecmp, PL_strncasecmp, and PL_strcaserstr with nsCRT::strcasecmp, nsCRT::strncasecmp and nsCRT::strcasestr? And mention that those nsCRT functions' implementations can probably be changed to call the standard C library functions instead of the PL_str* functions.

We'll need plstr.h for these calls to PL_strncpyz, which don't have a standard C library equivalent:

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

btw, some of the callers to other nsCRT functions can be replaced with calls to the standard C library functions, such as:

• nsCRT::strtok and strtok
• nsCRT::atoll and atoll

Comment 2

[Nicklas Boman [:smurfd]]

thanks for the feedback. To be honest i first thought just to clear out the "plstr.h" when not being used, but sure this is better :)

i filed these 3 bugs, please have a look and see that they look ok, you got cc:d
https://bugzilla.mozilla.org/show_bug.cgi?id=1725363
https://bugzilla.mozilla.org/show_bug.cgi?id=1725365
https://bugzilla.mozilla.org/show_bug.cgi?id=1725366
Was wondering if we need there aswell to check the return, if it differs between PL_ and nsCRT:: functions.

One thing that confuses me is this :
"And mention that those nsCRT functions' implementations can probably be changed to call the standard C library functions instead of the PL_str* functions"
I can't see anything about windows calls in these
https://searchfox.org/mozilla-central/rev/f71cb98fc35da418d2cb9ce31a0416d532dc9d69/xpcom/ds/nsCRT.h#43-78

they just check if you are doing some fuzzing and if you are on linux, otherwise call the PL_ function which is just standard calls
https://searchfox.org/mozilla-central/rev/3a8091d1c29473a0839ad7a5810028f41363fe2e/nsprpub/lib/libc/src/strcase.c#45-147
The only special thing i can see is PR_IMPLEMENT and PLUint32

but i assume im missing something :)

Comment 3

[Chris Peterson [:cpeterson]]

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

i filed these 3 bugs, please have a look and see that they look ok, you got cc:d
https://bugzilla.mozilla.org/show_bug.cgi?id=1725363
https://bugzilla.mozilla.org/show_bug.cgi?id=1725365
https://bugzilla.mozilla.org/show_bug.cgi?id=1725366

Thanks!

Was wondering if we need there aswell to check the return, if it differs between PL_ and nsCRT:: functions.

If I am reading the code correctly, the only difference is that nsCRT::strncasecmp will return -1 in some cases where [PL_strcasecmp will return large negative numbers](https://searchfox.org/mozilla-central/rev/d3683dbb252506400c71256ef3994cdbdfb71ada/nsprpub/lib/libc/src/strcase.c#65

I don't think we need to worry about that difference.

Was there another difference in return values that I missed?

One thing that confuses me is this :
"And mention that those nsCRT functions' implementations can probably be changed to call the standard C library functions instead of the PL_str* functions"
I can't see anything about windows calls in these
https://searchfox.org/mozilla-central/rev/f71cb98fc35da418d2cb9ce31a0416d532dc9d69/xpcom/ds/nsCRT.h#43-78

they just check if you are doing some fuzzing and if you are on linux, otherwise call the PL_ function which is just standard calls
https://searchfox.org/mozilla-central/rev/3a8091d1c29473a0839ad7a5810028f41363fe2e/nsprpub/lib/libc/src/strcase.c#45-147
The only special thing i can see is PR_IMPLEMENT and PLUint32

Looks like Windows has no functions equivalent to strncasecmp or strcasestr, so nsCRT::strncasecmp and nsCRT::strcasestr would still need to call the PL_ functions.

However, Windows has a non-standard function for case-insensitive string compares: _strcmpi. So nsCRT::strcasecmp's implementation can be changed to call _strcmpi on Windows and strcasecmp on other OSes.

Comment 4

[Nicklas Boman [:smurfd]]

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

If I am reading the code correctly, the only difference is that nsCRT::strncasecmp will return -1 in some cases where [PL_strcasecmp will return large negative numbers](https://searchfox.org/mozilla-central/rev/d3683dbb252506400c71256ef3994cdbdfb71ada/nsprpub/lib/libc/src/strcase.c#65

I don't think we need to worry about that difference.

It seems that most calls to PL_strcasecmp just check wether its zero or not. So we should be good.

Was there another difference in return values that I missed?

No i dont think so.

Looks like Windows has no functions equivalent to strncasecmp or strcasestr, so nsCRT::strncasecmp and nsCRT::strcasestr would still need to call the PL_ functions.

However, Windows has a non-standard function for case-insensitive string compares: _strcmpi. So nsCRT::strcasecmp's implementation can be changed to call _strcmpi on Windows and strcasecmp on other OSes.

Im thinking i might file a separate bug for replacing the PL_ calls in the nsCRT:: functions, just to avoid any kind of confusion.

Comment 5

[Nicklas Boman [:smurfd]]

Hmm i ran into some problems replacing the PL_ functions in nsCRT.h saying something like
all paths through this function will call itself
I replaced the #if defined(LIBFUZZER) && defined(LINUX) with a #if defined(XP_WIN) and called that _strcmpi() there and in the else i had the nsCRT::strcasecmp() call...

not sure how to go about doing that best?

Comment 6

[Chris Peterson [:cpeterson]]

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

Hmm i ran into some problems replacing the PL_ functions in nsCRT.h saying something like
all paths through this function will call itself

Are you seeing that build error on Windows, Linux, or macOS? Did you keep the :: namespace path before ::strcasecmp? This error message sounds like the compiler thinks the strcasecmp call inside nsCRT::strcasecmp is going to recursively call nsCRT::strcasecmp instead of the function strcasecmp of the same name in the global namespace ::.

I replaced the #if defined(LIBFUZZER) && defined(LINUX) with a #if defined(XP_WIN) and called that _strcmpi() there and in the else i had the nsCRT::strcasecmp() call...

So you code looks something like this?

  static int32_t strcasecmp(const char* aStr1, const char* aStr2) {
#if defined(XP_WIN)
    return int32_t(_strcmpi(aStr1, aStr2));
#else
    return int32_t(::strcasecmp(aStr1, aStr2));
#endif
  }

btw, I don't know if we still need the int32_t cast. nsCRT::strcasecmp returns int32_t and _strcmpi and ::strcasecmp both return (signed) int.

Comment 7

[Nicklas Boman [:smurfd]]

Thanks, i must have mixed something together, that works :) filing bug + patch later

Comment 8

[Nicklas Boman [:smurfd]]

Hey,
so i noticed i still had this opened.
Submitted a patch to remove some #include "plstr.h" from a bunch of files, where they are no longer needed.
since those files no longer have PL_ functions in them.

However the revsion in Phabricator is flagged as a Secure revision.
Not sure if i touched some file that flagged it that way or if it is a waiting game?

and please if it is a wasted effort please let me know, ill close this bug. no hard feelings :)

Comment 9

[Nicklas Boman [:smurfd]]

Attachment: Bug 1724649 - Followup to remove #include plstr.h r?#necko-reviewers

Comment 10

[Andrew McCreight [:mccr8]]

There's a bot that is supposed to remove the secure revision. It has been a bit slow lately, but it looks like it is unhidden now.

Comment 11

[Chris Peterson [:cpeterson]]

Nicklas, have you confirmed that all platforms still compile successfully on Try with your patch?

You should remove my name as reviewer. Removing unnecessary header files is appreciated, but I'm not a reviewer for any of this code. :)

Comment 12

[Nicklas Boman [:smurfd]]

Thanks Chris

removing you from reviewer :)
(i have build it locally on mac & linux)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e81517cf420
Followup to remove #include plstr.h r=necko-reviewers,xpcom-reviewers,valentin,nika

Comment 14

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/6e81517cf420