Last Comment Bug 787814 - Fix errors in SIPCC found by Valgrind of unittests
: Fix errors in SIPCC found by Valgrind of unittests
Status: RESOLVED FIXED
[WebRTC], [blocking-webrtc+] [qa-]
: valgrind
Product: Core
Classification: Components
Component: WebRTC: Signaling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Ethan Hugg [:ehugg]
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-02 10:03 PDT by Ethan Hugg [:ehugg]
Modified: 2012-10-21 22:08 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix errors in SIPCC found by Valgrind of unittests (4.53 KB, patch)
2012-09-02 12:34 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Fix errors in SIPCC found by Valgrind of unittests (4.54 KB, patch)
2012-09-02 13:10 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Fix errors in SIPCC found by Valgrind of unittests (4.74 KB, patch)
2012-09-02 22:37 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Patch 2 Fix errors in SIPCC found by Valgrind of unittests (2.38 KB, patch)
2012-09-17 11:47 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Patch 2 Fix errors in SIPCC found by Valgrind of unittests (1.66 KB, patch)
2012-09-17 13:14 PDT, Ethan Hugg [:ehugg]
rjesup: feedback+
Details | Diff | Splinter Review

Description Ethan Hugg [:ehugg] 2012-09-02 10:03:22 PDT
Valgrind results for signaling_unittests on OSX



==49326== Thread 9:
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0xDFE7CA2: strcasecmp_l (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0x1000B2A71: CCAPI_CallInfo_getIsConference
(ccapi_call_info.c:522)
==49326==    by 0x1000BDCF8: ccappUpdateSessionData (ccprovider.c:1572)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Use of uninitialised value of size 8
==49326==    at 0xDFE7CA4: strcasecmp_l (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0x1000B2A71: CCAPI_CallInfo_getIsConference
(ccapi_call_info.c:522)
==49326==    by 0x1000BDCF8: ccappUpdateSessionData (ccprovider.c:1572)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC59C: partyInfoPassedTheNameFilter (call_logger.c:171)
==49326==    by 0x1000AC8B7: handlePlacedCall (call_logger.c:224)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDF05: ccappUpdateSessionData (ccprovider.c:1597)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC5B0: partyInfoPassedTheNameFilter (call_logger.c:171)
==49326==    by 0x1000AC8B7: handlePlacedCall (call_logger.c:224)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDF05: ccappUpdateSessionData (ccprovider.c:1597)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC48C: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDF05: ccappUpdateSessionData (ccprovider.c:1597)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4A0: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDF05: ccappUpdateSessionData (ccprovider.c:1597)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4B4: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDF05: ccappUpdateSessionData (ccprovider.c:1597)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4C8: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDF05: ccappUpdateSessionData (ccprovider.c:1597)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4DC: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDF05: ccappUpdateSessionData (ccprovider.c:1597)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4F0: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDF05: ccappUpdateSessionData (ccprovider.c:1597)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0xDFACBA8: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFABEDA: vsnprintf_l (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0x1001DDBDC: notice_msg (cpr_darwin_stdio.c:181)
==49326==    by 0x1000BD9A1: ccappUpdateSessionData (ccprovider.c:1487)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0xDFADD7D: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFABEDA: vsnprintf_l (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0x1001DDBDC: notice_msg (cpr_darwin_stdio.c:181)
==49326==    by 0x1000BD9A1: ccappUpdateSessionData (ccprovider.c:1487)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0xDFAC20F: __ultoa (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFADDB1: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFABEDA: vsnprintf_l (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0x1001DDBDC: notice_msg (cpr_darwin_stdio.c:181)
==49326==    by 0x1000BD9A1: ccappUpdateSessionData (ccprovider.c:1487)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0xE579: strlen (in
/usr/local/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==49326==    by 0xDFAD8C2: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFABEDA: vsnprintf_l (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0x100044952: CSFLogV (CSFLog.cpp:61)
==49326==    by 0x100044C1B: CSFLog (CSFLog.cpp:93)
==49326==    by 0x1001DDC32: notice_msg (cpr_darwin_stdio.c:187)
==49326==    by 0x1000BD9A1: ccappUpdateSessionData (ccprovider.c:1487)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC59C: partyInfoPassedTheNameFilter (call_logger.c:171)
==49326==    by 0x1000AC8B7: handlePlacedCall (call_logger.c:224)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDA66: ccappUpdateSessionData (ccprovider.c:1509)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC5B0: partyInfoPassedTheNameFilter (call_logger.c:171)
==49326==    by 0x1000AC8B7: handlePlacedCall (call_logger.c:224)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDA66: ccappUpdateSessionData (ccprovider.c:1509)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC48C: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDA66: ccappUpdateSessionData (ccprovider.c:1509)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4A0: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDA66: ccappUpdateSessionData (ccprovider.c:1509)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4B4: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDA66: ccappUpdateSessionData (ccprovider.c:1509)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4C8: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDA66: ccappUpdateSessionData (ccprovider.c:1509)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4DC: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDA66: ccappUpdateSessionData (ccprovider.c:1509)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
==49326== Conditional jump or move depends on uninitialised value(s)
==49326==    at 0x1000AC4F0: partyInfoPassedTheNumberFilter (call_logger.c:146)
==49326==    by 0x1000AC8D3: handlePlacedCall (call_logger.c:225)
==49326==    by 0x1000ACF80: calllogger_update (call_logger.c:334)
==49326==    by 0x1000BDA66: ccappUpdateSessionData (ccprovider.c:1509)
==49326==    by 0x1000BB0BA: ccp_handler (ccprovider.c:1886)
==49326==    by 0x1000BA511: CCApp_task (ccapp_task.c:199)
==49326==    by 0xDFB88BE: _pthread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==    by 0xDFBBB74: thread_start (in /usr/lib/system/libsystem_c.dylib)
==49326==
Comment 1 Ethan Hugg [:ehugg] 2012-09-02 12:34:57 PDT
Created attachment 657701 [details] [diff] [review]
Fix errors in SIPCC found by Valgrind of unittests
Comment 2 Ethan Hugg [:ehugg] 2012-09-02 13:10:43 PDT
Created attachment 657707 [details] [diff] [review]
Fix errors in SIPCC found by Valgrind of unittests
Comment 3 Eric Rescorla (:ekr) 2012-09-02 21:07:04 PDT
Comment on attachment 657707 [details] [diff] [review]
Fix errors in SIPCC found by Valgrind of unittests

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

::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c
@@ +515,5 @@
>    CCAPP_DEBUG(DEB_F_PREFIX"Entering\n", DEB_F_PREFIX_ARGS(SIP_CC_PROV, fname));
>  
> +  memset(isConf_buf, 0, sizeof(isConf_buf));
> +
> +  if(platGetPhraseText(CONFERENCE_LOCALE_CODE, isConf, sizeof(isConf_buf)) == CC_FAILURE){

I'm trying to figure out how the isConf variable is used... it looks like we just write to it. What have I missed?

::: media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c
@@ +987,5 @@
>  {
>     session_data_t *newData = (session_data_t *) cpr_malloc(sizeof(session_data_t));
>  
>     if ( newData != NULL ) {
> +       memset(newData, 0, sizeof(session_data_t));

Do you know which variable is the UMR here? I wonder if we need to give it a more specific initializer.
Comment 4 Ethan Hugg [:ehugg] 2012-09-02 21:43:47 PDT
>::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c
>@@ +515,5 @@
>>    CCAPP_DEBUG(DEB_F_PREFIX"Entering\n", DEB_F_PREFIX_ARGS(SIP_CC_PROV, fname));
>>  
>> +  memset(isConf_buf, 0, sizeof(isConf_buf));
>> +
>> +  if(platGetPhraseText(CONFERENCE_LOCALE_CODE, isConf, sizeof(isConf_buf)) == >CC_FAILURE){
>
>I'm trying to figure out how the isConf variable is used... it looks like we just >write to it. What have I missed?

It's compared right below with a strcasecmp.  What is silly though is that isConf_bug and isConf are essentially the same, I should probably get rid of the pointer version.

Also down the rabbit-hole is the fact that platGetPhraseText is being pulled from the stub version which doesn't set isConf which is why it was uninitialized.  Adding to the silliness, is the version that Linux uses in ccapi_plat_api_impl.cpp doesn't do anything either except set it to a bunch of question marks.  It's called 20 times or so, I'm tempted to go in and tear it out, but thought that might be out-of-scope for this bug.



>::: media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c
>@@ +987,5 @@
>>  {
>>     session_data_t *newData = (session_data_t *) >cpr_malloc(sizeof(session_data_t));
>>  
>>     if ( newData != NULL ) {
>> +       memset(newData, 0, sizeof(session_data_t));
>
>Do you know which variable is the UMR here? I wonder if we need to give it a more >specific initializer.

attr and inst are the two integer values that are not being set here that throw the valgrind error in a debug message.  There are other int values as well in this data structure that were not being set on copy.  Despite the name of the function saying 'getDeepCopy' it doesn't copy everything, and the init version already had the memset.  

This session_data_t has 44 data members, I could copy them all, but I'm not sure yet if there was some reason for not copying them all, with the obvious exception of ref_count.  I suppose I could try it and see what happens.
Comment 5 Eric Rescorla (:ekr) 2012-09-02 22:00:40 PDT
OK. This all sounds reasonable.
Comment 6 Ethan Hugg [:ehugg] 2012-09-02 22:37:22 PDT
Created attachment 657770 [details] [diff] [review]
Fix errors in SIPCC found by Valgrind of unittests
Comment 7 Ethan Hugg [:ehugg] 2012-09-02 22:51:20 PDT
Comment on attachment 657770 [details] [diff] [review]
Fix errors in SIPCC found by Valgrind of unittests


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/aaaff7c6a473
Comment 8 Randell Jesup [:jesup] 2012-09-05 11:32:04 PDT
This appears resolved and is in alder
Comment 9 Ethan Hugg [:ehugg] 2012-09-12 13:22:40 PDT
More Valgrind errors still in:

(Patch fixed partyInfoPassedTheNameFilter only, missed this one)
==13812== Conditional jump or move depends on uninitialised value(s)
==13812==    at 0x4C21D4: partyInfoPassedTheNumberFilter (media/webrtc/signaling/src/sipcc/core/ccapp/call_logger.c:146)
==13812==    by 0x4C252F: handlePlacedCall (media/webrtc/signaling/src/sipcc/core/ccapp/call_logger.c:226)
==13812==    by 0x4C2924: calllogger_update (media/webrtc/signaling/src/sipcc/core/ccapp/call_logger.c:335)
==13812==    by 0x4CCF26: ccappUpdateSessionData (media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1594)
==13812==    by 0x4CF13D: ccp_handler (media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1883)
==13812==    by 0x4CB5B5: CCApp_task (media/webrtc/signaling/src/sipcc/core/ccapp/ccapp_task.c:199)
==13812==    by 0x4E3AE99: start_thread (/build/buildd/eglibc-2.15/nptl/pthread_create.c:308)
==13812==    by 0x77984BC: clone (/build/buildd/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112)
==13812==  Uninitialised value was created by a heap allocation
==13812==    at 0x4C2C62F: malloc (/home/sewardj/Vg38BRANCH/branch38/coregrind/m_replacemalloc/vg_replace_malloc.c:270)
==13812==    by 0x585ECE: cpr_malloc (media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_stdlib.c:485)
==13812==    by 0x5817BE: strlib_malloc (media/webrtc/signaling/src/sipcc/core/src-common/string_lib.c:88)
==13812==    by 0x581A36: strlib_empty (media/webrtc/signaling/src/sipcc/core/src-common/string_lib.c:409)
==13812==    by 0x4C98E4: ccsnap_set_line_label (media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_snapshot.c:74)
==13812==    by 0x4D1EE0: config_setup_elements (media/webrtc/signaling/src/sipcc/core/common/config_parser.c:492)
==13812==    by 0x4D2417: config_setup_main (media/webrtc/signaling/src/sipcc/core/common/config_parser.c:662)
==13812==    by 0x4C6DB9: parse_setup_properties (media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_config.c:90)
==13812==    by 0x4C6EB7: CCAPI_Start_response (media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_config.c:76)
==13812==    by 0x4ABD34: configCtlFetchReq (media/webrtc/signaling/src/softphonewrapper/CC_SIPCCService.cpp:138)
==13812==    by 0x4AC5DD: configFetchReq (media/webrtc/signaling/src/softphonewrapper/CC_SIPCCService.cpp:162)
==13812==    by 0x4C4BD3: registration_processEvent (media/webrtc/signaling/src/sipcc/core/ccapp/cc_device_manager.c:298)
==13812==

(This one appears to be new)

==13812== Syscall param msgsnd(msgp->mtype) points to uninitialised byte(s)
==13812==    at 0x779A4A3: ??? (/build/buildd/eglibc-2.15/sysvipc/../sysdeps/unix/syscall-template.S:82)
==13812==    by 0x583B5B: cprPostMessage (media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_ipc.c:808)
==13812==    by 0x5844AD: cprSendMessage (media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_ipc.c:669)
==13812==    by 0x4CB468: ccappTaskSendMsg (media/webrtc/signaling/src/sipcc/core/ccapp/ccapp_task.c:162)
==13812==    by 0x4CB4F5: ccappTaskPostMsg (media/webrtc/signaling/src/sipcc/core/ccapp/ccapp_task.c:129)
==13812==    by 0x4C452C: cc_invokeDeviceFeature (media/webrtc/signaling/src/sipcc/core/ccapp/cc_device_feature.c:52)
==13812==    by 0x4C45D2: CC_DeviceFeature_enableVideo (media/webrtc/signaling/src/sipcc/core/ccapp/cc_device_feature.c:84)
==13812==    by 0x4C706A: CCAPI_Device_enableVideo (media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_device.c:161)
==13812==    by 0x49F0B1: CSF::CC_SIPCCDevice::enableVideo(bool) (media/webrtc/signaling/src/softphonewrapper/CC_SIPCCDevice.cpp:114)
==13812==    by 0x49F1EA: CSF::CC_SIPCCDevice::CC_SIPCCDevice(unsigned int) (media/webrtc/signaling/src/softphonewrapper/CC_SIPCCDevice.cpp:75)
==13812==    by 0x49F304: CSF::CC_SIPCCDevice::wrap(unsigned int) (media/webrtc/signaling//./src/common/Wrapper.h:98)
==13812==    by 0x4AACD5: CSF::CC_SIPCCService::onDeviceEvent(ccapi_device_event_e, unsigned int, cc_device_info_t_*) (media/webrtc/signaling/src/softphonewrapper/CC_SIPCCService.cpp:645)
==13812==  Address 0x7feffef24 is on thread 1's stack
==13812==  Uninitialised value was created by a stack allocation
==13812==    at 0x583B26: cprPostMessage (media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_ipc.c:788)
==13812==

(this one must not have been fixed completely)

==13812== Thread 7:
==13812== Conditional jump or move depends on uninitialised value(s)
==13812==    at 0x76ED98E: vfprintf (/build/buildd/eglibc-2.15/stdio-common/vfprintf.c:1623)
==13812==    by 0x77AD57F: __vsnprintf_chk (/build/buildd/eglibc-2.15/debug/vsnprintf_chk.c:65)
==13812==    by 0x58583D: notice_msg (/usr/include/x86_64-linux-gnu/bits/stdio2.h:78)
==13812==    by 0x4CCFF3: ccappUpdateSessionData (media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1484)
==13812==    by 0x4CF13D: ccp_handler (media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1883)
==13812==    by 0x4CB5B5: CCApp_task (media/webrtc/signaling/src/sipcc/core/ccapp/ccapp_task.c:199)
==13812==    by 0x4E3AE99: start_thread (/build/buildd/eglibc-2.15/nptl/pthread_create.c:308)
==13812==    by 0x77984BC: clone (/build/buildd/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112)
==13812==  Uninitialised value was created by a stack allocation
==13812==    at 0x4D8573: ui_create_offer (media/webrtc/signaling/src/sipcc/core/common/ui.c:1610)
==13812==
==13812== Use of uninitialised value of size 8
==13812==    at 0x76ED4AB: _itoa_word (/build/buildd/eglibc-2.15/stdio-common/_itoa.c:195)
==13812==    by 0x76F1FDA: vfprintf (/build/buildd/eglibc-2.15/stdio-common/vfprintf.c:1623)
==13812==    by 0x77AD57F: __vsnprintf_chk (/build/buildd/eglibc-2.15/debug/vsnprintf_chk.c:65)
==13812==    by 0x58583D: notice_msg (/usr/include/x86_64-linux-gnu/bits/stdio2.h:78)
==13812==    by 0x4CCFF3: ccappUpdateSessionData (media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1484)
==13812==    by 0x4CF13D: ccp_handler (media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1883)
==13812==    by 0x4CB5B5: CCApp_task (media/webrtc/signaling/src/sipcc/core/ccapp/ccapp_task.c:199)
==13812==    by 0x4E3AE99: start_thread (/build/buildd/eglibc-2.15/nptl/pthread_create.c:308)
==13812==    by 0x77984BC: clone (/build/buildd/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112)
==13812==  Uninitialised value was created by a stack allocation
==13812==    at 0x4D8573: ui_create_offer (media/webrtc/signaling/src/sipcc/core/common/ui.c:1610)
==13812==
==13812== Conditional jump or move depends on uninitialised value(s)
==13812==    at 0x76ED4B5: _itoa_word (/build/buildd/eglibc-2.15/stdio-common/_itoa.c:195)
==13812==    by 0x76F1FDA: vfprintf (/build/buildd/eglibc-2.15/stdio-common/vfprintf.c:1623)
==13812==    by 0x77AD57F: __vsnprintf_chk (/build/buildd/eglibc-2.15/debug/vsnprintf_chk.c:65)
==13812==    by 0x58583D: notice_msg (/usr/include/x86_64-linux-gnu/bits/stdio2.h:78)
==13812==    by 0x4CCFF3: ccappUpdateSessionData (media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1484)
==13812==    by 0x4CF13D: ccp_handler (media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c:1883)
==13812==    by 0x4CB5B5: CCApp_task (media/webrtc/signaling/src/sipcc/core/ccapp/ccapp_task.c:199)
==13812==    by 0x4E3AE99: start_thread (/build/buildd/eglibc-2.15/nptl/pthread_create.c:308)
==13812==    by 0x77984BC: clone (/build/buildd/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112)
==13812==  Uninitialised value was created by a stack allocation
==13812==    at 0x4D8573: ui_create_offer (media/webrtc/signaling/src/sipcc/core/common/ui.c:1610)
==13812==

(Possibly an artifact of the test framework)
==13812== Mismatched free() / delete / delete []
==13812==    at 0x4C2B599: free (/home/sewardj/Vg38BRANCH/branch38/coregrind/m_replacemalloc/vg_replace_malloc.c:446)
==13812==    by 0x4052012: moz_free (memory/mozalloc/mozalloc.cpp:51)
==13812==    by 0x457985: test::SignalingTest::OfferAnswer(char const*, char const*) (ff-opt/media/webrtc/signaling/test/../../../../dist/include/mozilla/mozalloc.h:236)
==13812==    by 0x457E7E: test::SignalingTest_OfferAnswer_Test::TestBody() (media/webrtc/signaling/test/signaling_unittests.cpp:698)
==13812==    by 0xB1F0C1: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2090)
==13812==    by 0xB1FD3A: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2142)
==13812==    by 0xB28A74: testing::Test::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2162)
==13812==    by 0xB28B4F: testing::TestInfo::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2338)
==13812==    by 0xB28C89: testing::TestCase::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2445)
==13812==    by 0xB28FA7: testing::internal::UnitTestImpl::RunAllTests() (media/webrtc/trunk/testing/gtest/src/gtest.cc:4237)
==13812==    by 0xB1F0C1: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2090)
==13812==    by 0xB1FD3A: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2142)
==13812==  Address 0x154c4160 is 0 bytes inside a block of size 45 alloc'd
==13812==    at 0x4C2BA97: operator new[](unsigned long) (/home/sewardj/Vg38BRANCH/branch38/coregrind/m_replacemalloc/vg_replace_malloc.c:363)
==13812==    by 0xB111BA: testing::internal::String::String(char const*) (media/webrtc/trunk/testing//gtest/include/gtest/internal/gtest-string.h:303)
==13812==    by 0xB2199F: testing::internal::StringStreamToString(std::basic_stringstream<char, std::char_traits<char>, std::allocator<char> >*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:1751)
==13812==    by 0xB24FB3: testing::internal::GetBoolAssertionFailureMessage(testing::AssertionResult const&, char const*, char const*, char const*) (media/webrtc/trunk/testing//gtest/include/gtest/gtest-message.h:191)
==13812==    by 0x457931: test::SignalingTest::OfferAnswer(char const*, char const*) (media/webrtc/signaling/test/signaling_unittests.cpp:618)
==13812==    by 0x457E7E: test::SignalingTest_OfferAnswer_Test::TestBody() (media/webrtc/signaling/test/signaling_unittests.cpp:698)
==13812==    by 0xB1F0C1: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2090)
==13812==    by 0xB1FD3A: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2142)
==13812==    by 0xB28A74: testing::Test::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2162)
==13812==    by 0xB28B4F: testing::TestInfo::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2338)
==13812==    by 0xB28C89: testing::TestCase::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2445)
==13812==    by 0xB28FA7: testing::internal::UnitTestImpl::RunAllTests() (media/webrtc/trunk/testing/gtest/src/gtest.cc:4237)

==13812== Conditional jump or move depends on uninitialised value(s)
==13812==    at 0x9A2022: char* std::string::_S_construct<char const*>(char const*, char const*, std::allocator<char> const&, std::forward_iterator_tag) (/usr/include/c++/4.6/bits/basic_string.tcc:130)
==13812==    by 0x6F365E2: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.16)
==13812==    by 0x457752: test::SignalingTest::OfferAnswer(char const*, char const*) (media/webrtc/signaling/test/signaling_unittests.cpp:613)
==13812==    by 0x457B37: test::SignalingTest_FullCall_Test::TestBody() (media/webrtc/signaling/test/signaling_unittests.cpp:704)
==13812==    by 0xB1F0C1: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2090)
==13812==    by 0xB1FD3A: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2142)
==13812==    by 0xB28A74: testing::Test::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2162)
==13812==    by 0xB28B4F: testing::TestInfo::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2338)
==13812==    by 0xB28C89: testing::TestCase::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2445)
==13812==    by 0xB28FA7: testing::internal::UnitTestImpl::RunAllTests() (media/webrtc/trunk/testing/gtest/src/gtest.cc:4237)
==13812==    by 0xB1F0C1: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2090)
==13812==    by 0xB1FD3A: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2142)
==13812==  Uninitialised value was created by a heap allocation
==13812==    at 0x4C2C62F: malloc (/home/sewardj/Vg38BRANCH/branch38/coregrind/m_replacemalloc/vg_replace_malloc.c:270)
==13812==    by 0x4052028: moz_xmalloc (memory/mozalloc/mozalloc.cpp:57)
==13812==    by 0x455CE4: testing::internal::TestFactoryImpl<test::SignalingTest_FullCall_Test>::CreateTest() (ff-opt/media/webrtc/signaling/test/../../../../dist/include/mozilla/mozalloc.h:200)
==13812==    by 0xB1F0C1: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2090)
==13812==    by 0xB1FD3A: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2142)
==13812==    by 0xB28B36: testing::TestInfo::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2331)
==13812==    by 0xB28C89: testing::TestCase::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:2445)
==13812==    by 0xB28FA7: testing::internal::UnitTestImpl::RunAllTests() (media/webrtc/trunk/testing/gtest/src/gtest.cc:4237)
==13812==    by 0xB1F0C1: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2090)
==13812==    by 0xB1FD3A: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (media/webrtc/trunk/testing/gtest/src/gtest.cc:2142)
==13812==    by 0xB1FE15: testing::UnitTest::Run() (media/webrtc/trunk/testing/gtest/src/gtest.cc:3874)
==13812==    by 0x4511E5: main (media/webrtc/signaling/test/signaling_unittests.cpp:761)
==13812==
Comment 10 Ethan Hugg [:ehugg] 2012-09-17 11:47:59 PDT
Created attachment 661858 [details] [diff] [review]
Patch 2 Fix errors in SIPCC found by Valgrind of unittests
Comment 11 Ethan Hugg [:ehugg] 2012-09-17 11:52:03 PDT
Comment on attachment 661858 [details] [diff] [review]
Patch 2 Fix errors in SIPCC found by Valgrind of unittests


Here's a patch to the two most common ones.

partyInfoPassedTheNumberFilter was missed earlier when partyInfoPassedTheNameFilter was fixed.

The problem in ccappTaskPostMsg I believe was caused by a recent patch that made signaling use malloc/free directly.  This allocation was effectively changed from calloc to malloc so was not being initialized.
Comment 12 Ethan Hugg [:ehugg] 2012-09-17 11:53:57 PDT
Comment on attachment 661858 [details] [diff] [review]
Patch 2 Fix errors in SIPCC found by Valgrind of unittests


Second patch is wrong, going to re-try.
Comment 13 Ethan Hugg [:ehugg] 2012-09-17 13:14:02 PDT
Created attachment 661891 [details] [diff] [review]
Patch 2 Fix errors in SIPCC found by Valgrind of unittests
Comment 14 Ethan Hugg [:ehugg] 2012-09-17 13:18:18 PDT
Comment on attachment 661891 [details] [diff] [review]
Patch 2 Fix errors in SIPCC found by Valgrind of unittests


Here's a patch for the most common valgrind error.  partyInfoPassedTheNumberFilter was missed earlier when partyInfoPassedTheNameFilter was fixed.

I'm not sure about the others yet.  I'm tempted to take the log message out of ccappUpdateSessionData and see if the problem reappears somewhere else, I don't see out the session_data_t could created without being initialized.
Comment 15 Randell Jesup [:jesup] 2012-09-17 15:59:02 PDT
Comment on attachment 661891 [details] [diff] [review]
Patch 2 Fix errors in SIPCC found by Valgrind of unittests

f+, though that's some darn freaky code.... haven't people heard of '+' or '\t' or even using 0xXX with a comment?  But in any case f+, and I see no reason to improve the code there at this point beyond this.
Comment 16 Ethan Hugg [:ehugg] 2012-09-17 16:48:13 PDT
Yes.  I considered cleaning it up, but I think I'll be able to remove it entirely later.
Comment 17 Ethan Hugg [:ehugg] 2012-09-17 16:50:41 PDT
Comment on attachment 661891 [details] [diff] [review]
Patch 2 Fix errors in SIPCC found by Valgrind of unittests


Pushed patch 2 to Alder branch - http://hg.mozilla.org/projects/alder/rev/5064358f445a
Comment 18 Ethan Hugg [:ehugg] 2012-10-15 15:10:38 PDT
Pushed from Alder to M-I in the review-rollup (Bug 792188).

Note You need to log in before you can comment on or make changes to this bug.