PJSUA2 Crash Fix with CallSetting::fromPj

[austinkottke]

@nanangizz Crash occured with pjsua2 cpp api.

It appears bzero does not do any allocation of the string -- so pj2Str() is crashing since it does not check the length of the string. It simply checks the pointer.

inline string pj2Str(const pj_str_t &input_str)
{
    if (input_str.ptr)
	return string(input_str.ptr, input_str.slen);
    return string();
}

During the review process my initialization of pjsua_call_setting included

opt->custom_call_id = pj_str("");

which would set the pointer.

This was removed to due to an optimization since bzero was being applied to the pjsua_call_setting structure and it was thought unnecessary. However, this is required if you are calling pj2Str.

Simple fix applied to call.cpp is check the length before calling pj2Str:

void CallSetting::fromPj(const pjsua_call_setting &prm)
{
   ...

    if( prm.custom_call_id.slen > 0 )
    this->customCallId        = pj2Str(prm.custom_call_id);

}

FROM:

void CallSetting::fromPj(const pjsua_call_setting &prm)
{
   ...

    // Unsafe code -- custom_call_id has an unassigned nullptr 
   this->customCallId        = pj2Str(prm.custom_call_id);

}

Crash:

0 _platform_memmove + 420
7   0x1032da624 char* std::__1::copy[abi:ue170006]<char const*, char*>(char const*, char const*, char*) + 16 (copy.h:117) [inlined]
8   0x1032da624 std::__1::enable_if<__has_random_access_iterator_category<char const*>::value, char*>::type std::__1::copy_n[abi:ue170006]<char const*, unsigned long, char*>(char const*, unsigned long, char*) + 16 (copy_n.h:62) [inlined]
9    0x1032da624 std::__1::char_traits<char>::copy[abi:ue170006](char*, char const*, unsigned long) + 16 (char_traits.h:242) [inlined]
10  0x1032da624 std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::__init(char const*, unsigned long) + 48 (string:2168) [inlined]
11  0x1032da624 std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::basic_string[abi:ue170006](char const*, unsigned long) + 48 (string:957) [inlined]
12  0x1032da624 std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::basic_string[abi:ue170006](char const*, unsigned long) + 48 (string:955) [inlined]
13             	       0x1032da624 pj::pj2Str(pj_str_t const&) + 112 (util.hpp:40) [inlined]
14          	       0x1032da624 pj::CallSetting::fromPj(pjsua_call_setting const&) + 176 (call.cpp:237)
15             	       0x1032dae04 pj::CallInfo::fromPj(pjsua_call_info const&) + 816 (call.cpp:313)
16  0x1032dbdf8 pj::Call::getInfo() const + 164 (call.cpp:508)

Please let me know if you'd like for me to do a separate fork with the single change.

[nanangizz]

Since PJSIP 2.7, actually pj2Str() checks for length too (the commit here):

pjproject/pjsip/src/pjsua2/util.hpp

Lines 37 to 42 in aa56436

	inline string pj2Str(const pj_str_t &input_str)
	{
	if (input_str.ptr && input_str.slen>0)
	return string(input_str.ptr, input_str.slen);
	return string();
	}

[nanangizz]

Btw, the bzero() should also set the pointer to NULL, so even if pj2Str() didn't have the length check, it should not crash too. So the crash cause may be something else?

[austinkottke]

@nanangizz Yes - looking at it now. Possibly I am incorrect. I am stumped. After a pjsua_call_hangup it occurs.

0   libsystem_platform.dylib      	       0x1927b7344 _platform_memmove + 420
1   Softphone             	       0x102f86e18 pj::CallSetting::fromPj(pjsua_call_setting const&) + 176
2   Softphone              	       0x102f875f8 pj::CallInfo::fromPj(pjsua_call_info const&) + 816
3   Softphone              	       0x102f885ec pj::Call::getInfo() const + 164
4   Softphone              	       0x102d0d280 softphone::sip::CMSipCall::onCallState(pj::OnCallStateParam&) + 40 (cmsipcall.cpp:49)

It is intermittent. Not consistently reproducible. Trying to track it down.

[austinkottke]

So the pj_str prm.custom_call_id.ptr at some point starts pointing to a bad address. During the outbound invite it is there -- however as the callstate changes a memory dealloc occurs.

However, the length of the string prm.custom_call_id.s_len is staying the same since its a copy. Which results in potentially accessing restricted memory when pj2Str is called.

Technically to do an outbound call you can remove this->customCallId = pj2Str(prm.custom_call_id); from CallSetting::fromPj its not even required but is there for completeness.

Still looking at the call flow and trying to figure out where the release is.

[austinkottke]

@nanangizz At some point call->opt.custom_call_id.ptr points to a garbage memory location after the invite goes out and pjsua_call_on_tsx_state_changed is called.

However, call->opt.custom_call_id.s_len remains the correct string length which is where the problem lies since now you could potentially access restricted memory.

Any thoughts on where call->opt might be getting modified to free the pj_str ?

[nanangizz]

Ah right, the opt->custom_call_id will be copied to call->opt.custom_call_id in apply_call_setting(), but the string is shallow copied (only copy the pointer, not the buffer). As the string buffer is managed by app, app may free/invalidate it anytime and call->opt.custom_call_id.ptr will point to a garbage. The easy fix for this issue is perhaps by resetting/bzero-ing call->opt.custom_call_id after copied to dlg->call_id->id around here:

pjproject/pjsip/src/pjsua-lib/pjsua_call.c

Lines 962 to 965 in aa56436

	if( opt->custom_call_id.slen > 0 ){
	pj_strdup(dlg->pool, &dlg->call_id->id, &opt->custom_call_id);
	PJ_LOG(4,(THIS_FILE, "Set user defined Call-ID (%.*s)", (int)dlg->call_id->id.slen, dlg->call_id->id.ptr ));
	}

Btw, while here, please also fix the log printing line, the column width is > 80. Thanks :)

[austinkottke]

Thanks @nanangizz for the information on the shallow copy. This definitely explains it.

Ive done the requested changes. Ran a few tests and this seems to solve the issue. bzero is applied to call->opt.custom_call_id to clear the memory immediately and the call->opt.custom_call_id.s_len is explicitly set to 0.

This prevents the crash if custom_call_id is tried to be accessed later.

[austinkottke]

@nanangizz Great - Thank you - bzero is taking care of setting call->opt.custom_call_id.s_len to zero and now it removes redundant code.

When pj::CallSetting::fromPj() calls pj2Str it is now checking against a length of 0 which prevents the nullptr from being accessed. My pjsua and pjsua2 cpp tests are looking good.