806829 - WebRTC possible data race with ccsnap_device_pre_init vs. strlib_malloc

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_snapshot.c:329
    if ((g_deviceInfo.name) && (strlen(g_deviceInfo.name) > 0)) {
        strlib_free(g_deviceInfo.name);
    }

media/webrtc/signaling/src/sipcc/core/src-common/string_lib.c:68
    sstrncpy(temp->data, str, length + 1);
    temp->data[length] = '\0';

    return STRUCT_TO_STR(temp);

Marked as s-s because memory functions are involved, please downgrade if necessary.

Tested with m-c changeset: 111684:e19e170d2f6d

Comment 1

[Daniel Veditz [:dveditz]]

Does this stack make any sense? Why is webrtc signalling code (pretty standalone, imported from external right?) accessing memory allocated as part of a nsTArray deep inside a nsFrame? For that matter why does nsPresArena::State::Free call nsTArray::AppendElement?

Comment 2

[Christoph Diehl [:posidron]]

In retrospect this is very likely a false positive since there are also no locks involved and the run mode was pure-happens-before.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Christophe want to open this bug up?

Comment 4

[Christoph Diehl [:posidron]]

Yes, we can do that.

Comment 5

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

Seems very unlikely to be real

Comment 6

[Ethan Hugg [:ehugg]]

Attachment: Signaling - remove g_deviceInfo.name

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 692446 [details] [diff] [review]
Signaling - remove g_deviceInfo.name


I had a suspicion that g_deviceInfo.name was never set, so in order to prove it I made a patch that removes it all together.  We could either push this patch to simplify the code, or use it as proof that this race won't happen since it's never set.

Comment 8

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

Comment on attachment 692446 [details] [diff] [review]
Signaling - remove g_deviceInfo.name

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

::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_device_info.c
@@ +30,5 @@
>   * gets the device name
>   * @returns - a pointer to the device name
>   */
>  cc_string_t CCAPI_DeviceInfo_getDeviceName (cc_deviceinfo_ref_t handle)
>  {

At least debug here since we're not giving it a real devicename

Comment 9

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce156ff6dfde

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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