1308100 - Replace PL_strlen/PL_strnlen with strlen/strnlen

Status: RESOLVED FIXED

(Core :: General, defect, P5)

Description

[Chris Peterson [:cpeterson]]

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

1. Replace PL_strlen() and PL_strnlen() calls with strlen() and strnlen(), respectively. Note that the calls in security/manager/* files are being fixed in bug 1305930.

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

http://searchfox.org/mozilla-central/search?q=%5CbPL_strn%3Flen%5Cb&regexp=true&path=media%2Fwebrtc%2Fsignaling

*** CAUTION: PL_strlen() and PL_strlen() will return 0 when passed a null pointer, but strlen() and strnlen() will crash! Any calls to PL_strlen() and PR_strnlen() 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 strlen() or strnlen() function declarations.

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

Comment 1

[Adam Velebil]

I would like to fix this. Can you assign me?

Comment 2

[Eric Rahm [:erahm]]

(In reply to adamtomasv from comment #1)
> I would like to fix this. Can you assign me?

adamtomasv thanks for your interest! I'll assign the bug to you once we have a patch up, please let me know if you need any help getting started.

Comment 3

[Adam Velebil]

Attachment: Bug 1308100 - Replace PL_strlen/PL_strnlen with strlen/strnlen;

Review commit: https://reviewboard.mozilla.org/r/129310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129310/

Comment 4

[Chris Peterson [:cpeterson]]

Thanks, Adam! Your patch looks good and you've taken care to avoid crashing strlen() with a null pointer argument.

Have you confirmed that sdp_attr.c still compiles successfully without #include "plstr.h"? I assume you don't have a Solaris box handy to test nsWifiScannerSolaris.cpp, so we'll just assume it still compiles until someone reports otherwise. :)

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8857331 [details]
Bug 1308100 - Replace PL_strlen/PL_strnlen with strlen/strnlen;

https://reviewboard.mozilla.org/r/129310/#review132162

This looks good! Just one small change needed. I'll go ahead and push to try as well to see if the signaling code still builds without "plstr.h"

::: netwerk/wifi/nsWifiScannerSolaris.cpp:52
(Diff revision 1)
>  
>    nsWifiAccessPoint *ap = new nsWifiAccessPoint();
>    if (ap) {
>      ap->setMac(mac_as_int);
>      ap->setSignal(signal);
> -    ap->setSSID(essid_str, PL_strnlen(essid_str, DLADM_STRSIZE));
> +    if (essid_str) {

I think we still need to call `ap->setSSID` even if `essid_str` is null.

Similar to what you did above, something like the following would probably suffice:
```
size_t len = essid_str ? strlen(essid_str) : 0;
ap->setSSID(essid_str, len)
```

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8857331 [details]
Bug 1308100 - Replace PL_strlen/PL_strnlen with strlen/strnlen;

https://reviewboard.mozilla.org/r/129310/#review132162

> I think we still need to call `ap->setSSID` even if `essid_str` is null.
> 
> Similar to what you did above, something like the following would probably suffice:
> ```
> size_t len = essid_str ? strlen(essid_str) : 0;
> ap->setSSID(essid_str, len)
> ```

Err sorry, that's `strnlen`:

> size_t len = essid_str ? strnlen(essid_str, DLADM_STRSIZE) : 0;

Comment 7

[Eric Rahm [:erahm]]

The try build indicates that we need to keep plstr.h around in sdp_attr.c.

Comment 8

[Adam Velebil]

Comment on attachment 8857331 [details]
Bug 1308100 - Replace PL_strlen/PL_strnlen with strlen/strnlen;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129310/diff/1-2/

Comment 9

[Adam Velebil]

(In reply to Eric Rahm [:erahm] from comment #7)
> The try build indicates that we need to keep plstr.h around in sdp_attr.c.

I updated the code. Could you rerun the trybuild, as I am lost in the treeherder interface. Also, if it is possible, build for solaris.

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8857331 [details]
Bug 1308100 - Replace PL_strlen/PL_strnlen with strlen/strnlen;

https://reviewboard.mozilla.org/r/129310/#review132670

Looks great, thank you Adam!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0980cf6ea213
Replace PL_strlen/PL_strnlen with strlen/strnlen;r=erahm

Comment 12

[Eric Rahm [:erahm]]

(In reply to adamtomasv from comment #9)
> (In reply to Eric Rahm [:erahm] from comment #7)
> > The try build indicates that we need to keep plstr.h around in sdp_attr.c.
> 
> I updated the code. Could you rerun the trybuild, as I am lost in the
> treeherder interface. Also, if it is possible, build for solaris.

The code looks good, the only build failure previously was from the missing header so we should be good. I've gone ahead and landed it.

For future reference you can initiate a try build from the "Automation" drop down menu in the mozreview interface.

Comment 13

[Adam Velebil]

(In reply to Eric Rahm [:erahm] from comment #12)
> (In reply to adamtomasv from comment #9)
> > (In reply to Eric Rahm [:erahm] from comment #7)
> > > The try build indicates that we need to keep plstr.h around in sdp_attr.c.
> > 
> > I updated the code. Could you rerun the trybuild, as I am lost in the
> > treeherder interface. Also, if it is possible, build for solaris.
> 
> The code looks good, the only build failure previously was from the missing
> header so we should be good. I've gone ahead and landed it.
> 
> For future reference you can initiate a try build from the "Automation" drop
> down menu in the mozreview interface.

Thanks Eric, now I see it.

Comment 14

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/0980cf6ea213