Closed Bug 1308100 Opened 4 years ago Closed 4 years ago

Replace PL_strlen/PL_strnlen with strlen/strnlen

Categories

(Core :: General, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: cpeterson, Assigned: adamtomasv, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=c++] [good first bug])

Attachments

(1 file)

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.
Mentor: cpeterson
Mentor: erahm
Whiteboard: [lang=c++] → [lang=c++] [good first bug]
I would like to fix this. Can you assign me?
Flags: needinfo?(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.
Flags: needinfo?(erahm)
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. :)
Assignee: nobody → adamtomasv
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)
```
Attachment #8857331 - Flags: review?(erahm) → review-
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;
The try build indicates that we need to keep plstr.h around in sdp_attr.c.
(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.
Flags: needinfo?(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!
Attachment #8857331 - Flags: review?(erahm) → review+
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0980cf6ea213
Replace PL_strlen/PL_strnlen with strlen/strnlen;r=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.
Flags: needinfo?(erahm)
(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.
https://hg.mozilla.org/mozilla-central/rev/0980cf6ea213
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.