790194 - Almost all uses of atoi/strtol/strtoul() in dom/media/webrtc should be replaced

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P5)

Description

[Randell Jesup [:jesup] (needinfo me)]

atoi() is incorrect in almost all cases due to no error checking.

It also accepts alternative bases, which are typically invalid in sdp (for example).

For example, if 0 is a valid value for (say) a payload type, and you call atoi(tok) and get 0 - great! Except tok pointed to "help_me"....

Also no overflow/underflow notification.

Almost all cases should be replaced with xxx = strtol(tok, newtok, 10); and error-checked with "if (xxx < min_val || xxx > max_val || errno != 0 || tok == newtok)"
(min/max vals are item-dependent and are typically already there).

You can't count on EINVAL if tok == newtok it appears.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: replace instances of atoi

Comment 2

[Ethan Hugg [:ehugg]]

Comment on attachment 662346 [details] [diff] [review]
replace instances of atoi


This is a work in progress.  I'll be taking another pass to check some of the boundaries and fix a bit of this insanely duplicated code.

Comment 3

[Ethan Hugg [:ehugg]]

Attachment: replace instances of atoi

Comment 4

[Ethan Hugg [:ehugg]]

Attachment: replace instances of atoi

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 662577 [details] [diff] [review]
replace instances of atoi


All instances of atoi() have been removed.

I also added a couple subroutines to do error reporting for sdp_parse_attr_fmtp().  That function is full of duplicated code and could be simplified further, but I didn't want to spread tho scope of this bug too much.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 662577 [details] [diff] [review]
replace instances of atoi

Review of attachment 662577 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/callcontrol/CallControlManagerImpl.cpp
@@ +349,3 @@
>  
> +  if (key == ConfigPropertyKeysEnum::eLocalVoipPort) {
> +    errno = 0;

Good catch that strtol/strtoul don't change errno if successful

::: media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_pmh.c
@@ -3343,5 @@
> -        value = atoi(inputValue);
> -    } else if (isupper((int) inputChar)) {
> -        value = 9 + (inputChar - 'A' + 1);
> -    } else if (islower((int) inputChar)) {
> -        value = 9 + (inputChar - 'a' + 1);

Cool, nice bug averted by switching this to strtol(....,16)

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 662577 [details] [diff] [review]
replace instances of atoi


Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/162ac0d51377

Comment 8

[Ethan Hugg [:ehugg]]

Re-opening this bug.  It turns out that strtol/strtoul are not safe to use because of issues with locale.

To quote the GNU libc manual:
"In a locale other than the standard "C" locale, this function may recognize additional implementation-dependent syntax."

Firefox does not necessarily run in the "C" locale, so we need to change to use PR_sscanf();

I will probably create a helper functions in signaling to parse long/unsigned long using PR_sscanf() and replace uses of strtol/strtoul with those.

Leaving the patch applied - still better than atoi and half of it is cleanup, and there are more strtol/strtoul instances elsewhere is signaling as well.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Doesn't block landing

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

In glib, g_ascii_strtoll/etc will do what we need:
https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-ascii-strtoll

Or we can use PR_sscanf().  strtol/etc (like atoi) can be affected by locale (for example thousands separators may be skipped over).

Resetting owner; looking for someone to work on this.

Comment 11

[Vijendra Singh]

Hi!
Could you please assign me this bug?

Comment 12

[Vijendra Singh]

Randell, I looked up the directory media/webrtc/signaling and found that it has been re-structured and pruned (For instance as per https://bugzilla.mozilla.org/show_bug.cgi?id=1078430. smdef.c and ccsip_pmh.c were removed. The other two (sdp_access.c, sdp_token.c) were merely relocated to a different place in the tree (media/webrtc/signaling/src/sdp/sipcc)).

I however found one instance of strtoul() in /media/webrtc/signaling/src/jsep/JsepCodecDescription.h:51:    unsigned long pt = strtoul(ptString.c_str(), &end, 10);
and 45 other uses of strtoul() in /media/webrtc/signaling/src/sdp  (more info http://txs.io/kq3b). I was not able to find any use of atoi() or strtol() in media/webrtc/signaling/.

In order to use g_ascii_strtoll(), could you please tell me if the mozilla codebase supports glib (https://goo.gl/fy7YMp) or it will give me errors when I will write #include <glib.h> and #include <glib/gprintf.h> ?

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

glib can't be used in windows, uncertain about mac or android but I wouldn't assume it.  
PR_sscanf() will work and is unaffected by locale.  As per comment 8, you could provide strtoul() equivalents using PR_sscanf() internally, which will make it easier to read and understand.

NI-ing nathan in case he has a pattern we should follow instead (or we missed a simpler alternative to PR_sscanf()).  Nathan: note comment 8: "It turns out that strtol/strtoul are not safe to use because of issues with locale."

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Randell Jesup [:jesup] from comment #13)
> PR_sscanf() will work and is unaffected by locale.  As per comment 8, you
> could provide strtoul() equivalents using PR_sscanf() internally, which will
> make it easier to read and understand.
> 
> NI-ing nathan in case he has a pattern we should follow instead (or we
> missed a simpler alternative to PR_sscanf()).  Nathan: note comment 8: "It
> turns out that strtol/strtoul are not safe to use because of issues with
> locale."

Bleh.  Looks like sscanf has the same issue, since the conversion characters are specified in terms of strtol or similar.

I would prefer not to use PR_sscanf, since we don't get format string checking when we use it, and we're trying to move away from NSPR functionality.  If we absolutely have to use it, let's use wrapper functions so we only have to fix one place when we can move away from PR_sscanf.  (Wrapper functions would also make it easier to mass-convert away from those wrapper functions.)

A alternative would be to use strtol_l and friends, which accept an explicit locale.  Those look like relatively recent POSIX functions, though, and I'm not sure they're directly available on all the version of OS X we support.  They also require some fiddling on Android, and they're named slightly differently on Windows.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 16

[kokopo]

Hello,

I would like to take up this issue . Restating the issue for my understand:

• instances of atoi() have been removed. (#comment 7)
• need to change strtol/strtoul -> create a helper functions in signaling to parse long/unsigned long using PR_sscanf() and replace uses of strtol/strtoul with those. (#comment 8)

Please confirm if I understood it rightly and assign me this issue

Comment 17

[Paul Bone [:pbone]]

Also NI'ing Nico in case jesup hasn't looked at this code in a while.

To share some context, kokopo is an Outreachy applicant and is looking at this good-first-bug as part of their application round.

Thanks.

Comment 18

[Paul Bone [:pbone]]

Forgot to ask, is this still a problem? The bug was last touched 8 years ago.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

It still should be. strtoll/strtoull are dependent on LOCALE, and virtually all our uses of it should not be.

If we can use glib-specific functions, we can use ascii_strtoll/ascii_strtoull(). Or use PR_sscanf() (likely by making a wrapper).

https://docs.gtk.org/glib/func.ascii_strtoll.html

Comment 20

[VK]

Hi,
I would like to take up this issue. Can you assign me to it ?

Comment 21

[Ranjan]

Is this issue still active? I would love to take up this issue as my first bug! Can someone assign me to it?

Comment 22

[Byron Campen [:bwc]]

Sure! There are still some cases of strtoul around in dom/media/webrtc, and a few atoi in nicer. I don't see strtol anywhere.

Comment 23

[Byron Campen [:bwc]]

Here's the quick-start guide, happy patching!

https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#firefox-contributors-quick-reference

Comment 24

[Ranjan]

Ok, thank you for assigning Byron. Working on it...

Comment 25

[Ranjan]

Hey, Byron went through all lines of code in webrctc/signaling folder in the code base...But found no occurence of atoi/strtol/strtoul()

Comment 26

[Byron Campen [:bwc]]

All that code has moved to dom/media/webrtc:

https://searchfox.org/mozilla-central/search?q=strtoul&path=dom%2Fmedia%2Fwebrtc&case=false&regexp=false
https://searchfox.org/mozilla-central/search?q=atoi&path=dom%2Fmedia%2Fwebrtc&case=false&regexp=false

Comment 27

[Ranjan]

Ok thank you very much. Working on it

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 29

[B. Dahse]

Are we sure PR_sscanf is locale-independent? I looked at the implementation of PR_sscanf and it uses isspace and isdigit. Technically those are also locale-dependent. POSIX-2008 specifies _l variants. Also: does PR_sscanf check for underflow/overflow? I don't think it does??!

Source: https://searchfox.org/mozilla-central/source/nsprpub/pr/src/io/prscanf.c#618

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Using a gecko wrapper around PR_sscanf is a valid step forward; while in theory it's locale-dependent, it avoids most or all of the current issues. See Froyd's comment

Comment 31

[vinitjangir]

Good evening. I have found 5 places where atoi, strtoul, and strtoull are used in the WebRTC folders:

1. dom/media/webrtc/transport/test/turn_unittest.cpp:203: r = nr_str_port_to_transport_addr(host.c_str(), atoi(port.c_str()),
2. dom/media/webrtc/transport/test/turn_unittest.cpp:283: r = nr_str_port_to_transport_addr(host.c_str(), atoi(port.c_str()),
3. dom/media/webrtc/sdp/SdpHelper.cpp:630: unsigned long pt = strtoul(ptString.c_str(), &end, 10);
4. dom/media/webrtc/sdp/SipccSdp.cpp:77: uint64_t sessId = strtoull(sdp_get_owner_sessionid(sdp), nullptr, 10);
5. dom/media/webrtc/sdp/SipccSdp.cpp:78: uint64_t sessVer = strtoull(sdp_get_owner_version(sdp), nullptr, 10);

I am new to this code and I want to update these correctly. Could you please show me an example of the code I should use and how to handle errors? Thank you.

Comment 32

[Byron Campen [:bwc]]

I think we want to replace these with std::from_chars. Some of these cases start with a std::string, so passing first and last will be trivial. There are a couple of others that are based on a c-string, so you'll just need to use strlen to find the end.

Comment 33

[vinitjangir]

Good evening, Byron,

Thank you for the guidance! I have understood.

I have successfully changed all the instances across the dom/media/webrtc folder and verified everything builds cleanly. Submitting the patch via Phabricator now.

Comment 34

[vinitjangir]

Attachment: Bug 790194 - Changed all instances in dom/media/webrtc to std::from_chars

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/61c84c2e3eb2
https://hg.mozilla.org/integration/autoland/rev/f88252862f07
Changed all instances in dom/media/webrtc to std::from_chars r=bwc

Comment 36

[Norisz Fay [:noriszfay]]

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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Reverted from Beta for 152.0b8 for causing bug 2044894.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3c04760be816
https://hg.mozilla.org/releases/mozilla-beta/rev/720d007a4b0a
Revert "Bug 790194 - Changed all instances in dom/media/webrtc to std::from_chars r=bwc" for causing bug 2044894.

Comment 39

[Byron Campen [:bwc]]

The better error checking caught an internal error, breaking Vonage, although that error is of no consequence.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

This was only backed out from Beta, so the bug should remain closed.