The AVDTP spec allows for a race condition between remote and local
device when issuing an AVDTP_START cmd on a stream in the OPEN state.
However, the internal state must continue to be consistent. For example,
suppose that avdtp_start() has been called while in the OPEN state and
a AVDTP_START cmd is sent. Now before we have received a response (and
thus entered the STREAMING state), we *receive* a START cmd. Prior to
this fix, since the sep is still in the OPEN state, we would accept
the new START cmd. This will leads us to send both a Start_Ind and
Start_Cfm - not good.
Now, we track this transitional state (starting == TRUE).
NB - 'starting' is only in a valid state while the sep is in the
OPEN state. 'starting' is reset when we return to the OPEN state.
This is probably a glib bug on g_slist_free_full which doesn't handle the
case where the list is modified inside the callback:
Invalid read of size 8
at 0x50AD5B2: g_slice_free_chain_with_offset (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x13057B: a2dp_unregister (a2dp.c:1550)
by 0x12144C: a2dp_server_remove (manager.c:1032)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x178B55: adapter_remove (adapter.c:2326)
by 0x175205: manager_remove_adapter (manager.c:290)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x50ADF3A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x175086: manager_cleanup (manager.c:298)
by 0x11D7A8: main (main.c:305)
Address 0x637a5e8 is 8 bytes inside a block of size 16 free'd
at 0x4C27D6E: free (vg_replace_malloc.c:366)
by 0x50AD9FC: g_slist_remove (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x12E5C6: a2dp_remove_sep (a2dp.c:1667)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x50ADF3A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x13057B: a2dp_unregister (a2dp.c:1550)
by 0x12144C: a2dp_server_remove (manager.c:1032)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x178B55: adapter_remove (adapter.c:2326)
by 0x175205: manager_remove_adapter (manager.c:290)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x50ADF3A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.2800.8)
Invalid free() / delete / delete[]
at 0x4C27D6E: free (vg_replace_malloc.c:366)
by 0x50AD5A3: g_slice_free_chain_with_offset (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x13057B: a2dp_unregister (a2dp.c:1550)
by 0x12144C: a2dp_server_remove (manager.c:1032)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x178B55: adapter_remove (adapter.c:2326)
by 0x175205: manager_remove_adapter (manager.c:290)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x50ADF3A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x175086: manager_cleanup (manager.c:298)
by 0x11D7A8: main (main.c:305)
Address 0x637a5e0 is 0 bytes inside a block of size 16 free'd
at 0x4C27D6E: free (vg_replace_malloc.c:366)
by 0x50AD9FC: g_slist_remove (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x12E5C6: a2dp_remove_sep (a2dp.c:1667)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x50ADF3A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x13057B: a2dp_unregister (a2dp.c:1550)
by 0x12144C: a2dp_server_remove (manager.c:1032)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x178B55: adapter_remove (adapter.c:2326)
by 0x175205: manager_remove_adapter (manager.c:290)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x50ADF3A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.2800.8)
Invalid read of size 8
at 0x50AD5B2: g_slice_free_chain_with_offset (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x175086: manager_cleanup (manager.c:298)
by 0x11D7A8: main (main.c:305)
Address 0x62b7ea8 is 8 bytes inside a block of size 16 free'd
at 0x4C27D6E: free (vg_replace_malloc.c:366)
by 0x50AD9FC: g_slist_remove (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x1751AE: manager_remove_adapter (manager.c:275)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x50ADF3A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x175086: manager_cleanup (manager.c:298)
by 0x11D7A8: main (main.c:305)
Invalid free() / delete / delete[]
at 0x4C27D6E: free (vg_replace_malloc.c:366)
by 0x50AD5A3: g_slice_free_chain_with_offset (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x175086: manager_cleanup (manager.c:298)
by 0x11D7A8: main (main.c:305)
Address 0x62b7ea0 is 0 bytes inside a block of size 16 free'd
at 0x4C27D6E: free (vg_replace_malloc.c:366)
by 0x50AD9FC: g_slist_remove (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x1751AE: manager_remove_adapter (manager.c:275)
by 0x50ADF16: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x50ADF3A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.2800.8)
by 0x175086: manager_cleanup (manager.c:298)
by 0x11D7A8: main (main.c:305)
To fix this now adapter_remove and a2dp_unregister_sep are passed
directly as a callbacks so g_slist_remove is not triggered.
When a vendor dependent command is requested but target does not
implement it, the correct return value is CTYPE_NOT_IMPLEMENTED instead
of CTYPE_REJECTED.
AVRCP 1.3 spec clearly says so on section 4.5.1:
[ It is assumed that devices that do not support this metadata
transfer related features shall return a response of NOT
IMPLEMENTED as per AV/C protocol specification ]
And AV/C General Specification, section 8.3.2 talks about legacy
behavior and mandates that NOT_IMPLEMENTED is returned.
Finally, in section 11.6.1 we see that VENDOR-DEPENDENT command frame
depends on the company_ID. Therefore we can't assume it has the same
format as the one specified for metadata transfer (in case the company
ID is 0x001958)
In order to support vendordep pdu as required by AVRCP 1.3 this part
will get very large. So, separate it to a new function like is done for
panel_passthrough.
When handling the discover response, if all stream end points are
in use, then we must finalize discovery, as no GET_CAPABILITIES
command will be issued.
Changing stream state from STREAMING to IDLE can be associated with side
effects under some circumstances (such as terminating bluetoothd during
music is streamed). In this case, after connection is lost, stream state
changes from STREAMING to IDLE - "avdtp_sep_set_state" is triggered which
invokes callback called "stream_state_changed" which internally invokes
"avdtp_sep_set_state" (state of stream doesn't change and stays as IDLE)
second time and then stream object is freed by "stream_free"
at the end of "avdtp_sep_set_state". After returning from callback,
first triggered "avdtp_sep_set_state" attempts to free stream object
again ("if (state == AVDTP_STATE_IDLE)" condition is still satisfied)
and it leads to invalid read/write/free issues (reported by valgrind)
in "stream free" body, since "stream" is "alias" pointer to stream object
which is already out of date (memory for stream object has been already
freed).
This patch prevents from this special case by freeing stream object only
when it is present on streams list and removing from this list when
stream object would be freed.
Changing stream state from STREAMING to IDLE can be associated with side
effects under some circumstances (such as terminating bluetoothd during
music is streamed). In this case, after connection is lost, stream state
changes from STREAMING to IDLE - "avdtp_sep_set_state" is triggered which
invokes callback called "stream_state_changed" which internally invokes
"avdtp_sep_set_state" (state of stream doesn't change and stays as IDLE)
second time and then stream callbacks list is discarded by "stream_free"
("g_slist_free(stream->callbacks)"). After returning from callback,
"stream->callbacks" list (and "l" pointer as well) is already out of date,
so attempting to fetch "l->next" pointer (returned by "g_slist_next(l)"
to be prepared to next iteration of "for" loop) from node on discarded
list leads to invalid read issue (reported by valgrind).
This patch prevents from this issue by moving "l = g_slist_next(l)"
instruction just before invoking callback - loop has been modified and
"while" used instead of "for" loop variant.
Under some circumstances (such as terminating bluetoothd during music is
streamed) endpoint object may be destroyed (memory for endpoint object is
internally freed, directly by "media_endpoint_remove") after invoking
"media_transport_destroy" (in "media_endpoint_clear_configuration") to
destroy transport object (memory for transport object is directly freed by
"media_transport_free"). It leads to invalid write issue (reported by
valgrind) after assignment "endpoint->transport = NULL", since "endpoint"
is "alias" pointer to endpoint object which is already out of date
(memory for endpoint object has been already freed).
This patch prevents from this issue by ensuring that assignment
"endpoint->transport = NULL" would be executed when endpoint object
certainly exists.
Under some circumstances (such as terminating bluetoothd during music is
streamed) sep object may be destroyed (memory for sep object is internally
freed, directly by "a2dp_unregister_sep") after invoking
"media_endpoint_clear_configuration" (in "stream_state_changed").
It leads to invalid write issue (reported by valgrind) after assignment
"sep->stream = NULL", since "sep" is "alias" pointer to sep object which
is already out of date (memory for sep object has been already freed)
This patch prevents from this issue by ensuring that assignment
"sep->stream = NULL" would be executed when sep object certainly exists.
Reply for control message with invalid (not registered for reception
of messages) PID should not have message information field present.
This was affecting AVCTP 1.3 qualification test case TP/NFR/BI-01-C.
Function media_endpoint_create returns pointer to structure. In
conditional expression it is safer to compare returned value with NULL
and not with gboolean as it is done in register_endpoint function.
By patch b9d85c0010 the initialization
of the telephony subsystem is delayed in the bluetoothd startup
procedure. As a result the SupportedFeatures bitmap has not been set
when creating the HFP SDP record. This patch changes the order of
the telephony initialization (via the state_changed function) and
the registration of the record, so that it gets the right value.
Although the corresponding bit in +BRSF is correctly set, the
missing bit for the "Three-way calling" feature in the SDP record
causes some headsets not to send AT+CHLD=? in certain situations.
This results in failed connections since BlueZ does not enter the
"connected" state on the headset interface before that command is
received, if the feature is supported by both sides.
AVRCP TG now returns a REJECTED response with the "Invalid command"
error code for VENDOR DEPENDENT commands. This fixes test case
AVRCP/TG/INV/TC_TG_INV_BI_01_C with recent PTS version.
Fixed incorrect update of transport->owners GSlist in
media_transport_free. Removal of list entries within 'for' loop leads to
invalid read of memory (l = l->next) and memory leaks.
Fixed incorrect update of server->sessions GSlist in avdtp_exit.
Previosly it was leading to invalid read of memory (l = l->next)
(and possible memory leaks) since after invoking avdtp_unref in
connection_lost, l pointer was not valid anymore (previously assignment
l = l->next was used after invoking connection_lost in for loop).
To make sure the SCO link is really disconnected we should wait for
POLLERR since POLLHUP does not necessarily means the link is
completely disconnected just that no further data can be sent/received.
Note that this depend on a fix of SCO socket shutdown in kernel to wait
for disconnect confimation to then kill/destroy the socket indicating
the err/reason using POLLERR.
