1308101 - Replace PL_strcpy/PL_strncpy with a safer Gecko string class or function

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_strcpy() and PL_strncpy() calls with strcpy() and strncpy(), respectively:

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

*** CAUTION: PL_strcpy() and PL_strncpy() will return null without copying any data when passed a null pointer, but strcpy() and strncpy() will crash! Any calls to PL_strcpy() and PR_strncpy() 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 strcpy() or strncpy() 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_strcpy() for strcpy().

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Hello, I'm asking your help with an experiment with making decisions on bugs. You've been needinfo'ed on this bug. I'd like you to take one action to help this bug make progress toward a decision. The things you can do include:

* If you know or have a good guess of which product and component this bug belongs to, change the product and component of the bug
* If you know of the right person to ask about this bug, redirect the needinfo to them
* If you cannot reproduce the bug, close it

All we need you to do is one thing that will help us make a decision on the bug or resolve it.

Thank you for your help with this. If you have questions, please contact [e-mail].

Comment 3

[Chris Peterson [:cpeterson]]

This bug doesn't fit neatly into a Bugzilla component because it is a general code cleanup, not a new feature or bug fix. However, most of the cleanup is in two DOM files, so we can move this to the DOM::General component.

dom/media/webrtc/RTCCertificate.cpp
dom/plugins/base/nsPluginHost.cpp
layout/style/test/TestCSSPropertyLookup.cpp

Comment 4

[Nicklas Boman [:smurfd]]

Hey, I will take this, it looks fairly straight forward.
These 3 calls(or is it just the 1 call from dom/media ?) have data in them, so they should not be Nullptrs in or out, so simply remove PL_ should be ok.

Is it the same here, that dom/plugins/base/nsPluginHost.cpp code will also go away soon?
dom/media/webrtc/RTCCertificate.cpp is there a webrtc reviewer group?

Comment 5

[Chris Peterson [:cpeterson]]

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

Hey, I will take this, it looks fairly straight forward.

Thanks!

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

Is it the same here, that dom/plugins/base/nsPluginHost.cpp code will also go away soon?

Yes. No need to bother updating dom/plugins/base/nsPluginHost.cpp. :)

dom/media/webrtc/RTCCertificate.cpp is there a webrtc reviewer group?

I don't see a webrtc or media group. For future reference, here is a list of all the Phabricator reviewer groups:

https://phabricator.services.mozilla.com/project/query/all/

Checking the Mozilla module owner wiki, looks like Randell Jesup is the WebRTC module owner, so just send your review request to r?jesup:

https://wiki.mozilla.org/Modules/Core#WebRTC

Comment 6

[Nicklas Boman [:smurfd]]

Attachment: Bug 1308101 - Replace PL_strcpy/PL_strncpy with a safer Gecko string class or function r?jesup

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3895daa47180
Replace PL_strcpy/PL_strncpy with a safer Gecko string class or function r=jesup

Comment 8

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 3895daa47180 (bug 1308101) for linux hazard bustages.
https://hg.mozilla.org/integration/autoland/rev/d646a7e2621160609635b056871e2c0a6799eea7

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Crunnable&revision=3895daa47180245ed81510780d2211c290f7044c&selectedTaskRun=O2hau4HoSo-mO8aMvnvGVg.1

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315798299&repo=autoland&lineNumber=14918

Comment 9

[Nicklas Boman [:smurfd]]

Looks like i need to add a :
buf[2] = '\0';

Comment 10

[Chris Peterson [:cpeterson]]

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315798299&repo=autoland&lineNumber=14918

[task 2020-09-16T00:56:59.583Z] 15:34.07 In function 'char* strncpy(char*, const char*, size_t)',
[task 2020-09-16T00:56:59.583Z] 15:34.07     inlined from 'static CERTName* mozilla::dom::GenerateRTCCertificateTask::GenerateRandomName(PK11SlotInfo*)' at /builds/worker/checkouts/gecko/dom/media/webrtc/RTCCertificate.cpp:75:12:
[task 2020-09-16T00:56:59.583Z] 15:34.07 /usr/include/x86_64-linux-gnu/bits/string3.h:120:34: error: 'char* __builtin_strncpy(char*, const char*, long unsigned int)' output truncated before terminating nul copying 3 bytes from a string of the same length [-Werror=stringop-truncation]
[task 2020-09-16T00:56:59.583Z] 15:34.07   120 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));

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

Looks like i need to add a :
buf[2] = '\0';

Looks like gcc is complaining about the source buffer, not the destination buf. It doesn't like that PL_strncpy(buf, "CN=", 3); is not copying the NUL terminator of the "CN=" string literal. Copying 4 characters instead of 3 might address the warning.

Comment 11

[Nicklas Boman [:smurfd]]

Thanks.
funny enough https://en.cppreference.com/w/c/string/byte/strncpy that sites example had that exact same error. so i could easily verify.

Comment 12

[Chris Peterson [:cpeterson]]

Nicklas, do you have a Linux build environment? Or access to Mozilla's "Try" servers? It would be good to verify the Linux strncpy error is fixed before landing on mozilla-central again.

Getting access to the Try Server:
https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

Comment 13

[Nicklas Boman [:smurfd]]

Hey, no i do not have access (that i know :)) to the try server.
i do however have a Linux build env. Is there something i can do then?
And yes i would really like to verify "locally" before making another attempt.

Comment 14

[Chris Peterson [:cpeterson]]

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

i do however have a Linux build env. Is there something i can do then?
And yes i would really like to verify "locally" before making another attempt.

When you build Firefox for Linux, are you using clang or gcc? AFAIK, Mozilla's official Linux builds use gcc (while Windows and macOS use clang). gcc might be reporting warnings about strncpy that clang doesn't. If you're using clang, can you try gcc?

You can override the default compiler in your mozconfig file using something like (this snippet from my mozconfig on macOS):

export CC="/path/to/your/gcc"
export CXX="/path/to/your/g++"

Comment 15

[Nicklas Boman [:smurfd]]

Thanks, yeah i have been using Clang as default on linux.

export CC="/usr/bin/gcc-9"
export CXX="/usr/bin/g++-9"

mk_add_options PYTHON=/usr/bin/python3.6

ac_add_options --enable-optimize
#ac_add_options --enable-linker=lld
ac_add_options --disable-elf-hack

# Debug builds
ac_add_options --enable-debug
ac_add_options --enable-debug-symbols
# Stricter builds
#### ac_add_options --enable-warnings-as-errors

thats pretty much my mozconfig file now, had clang as CC and clang++ as CXX before.
I have also tried to enable the --enable-warnings-as-errors as you suggested earlier but have struggled to get it to build, not becuase of my changes ... will digg deeper into that during the weekend. ;)
-- edit --
also if i wont succeed, ill try in like a Ubuntu vm ... (not opensuse that i have created the bootstrap for :))

Comment 16

[Nicklas Boman [:smurfd]]

Thanks Chris! YES, not sure if it was adding CC=gcc or combined that with the debug build option, now i get to see the errors shown in the try builds :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e15933214601
Replace PL_strcpy/PL_strncpy with a safer Gecko string class or function r=jesup

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e15933214601