Hi Luiz,
Good news, issue is unable to reproduce with your patch. And may I
know why struct avrcp *session got invalid address in this case?
And does this patch will commit to upstream? Thanks.
Regards,
Ethan
2015-03-18 14:08 GMT+08:00 Ethan <ethancsge@xxxxxxxxx>:
> Hi Luiz,
>
> Good news, issue is unable to reproduce with your patch. And may I know why
> struct avrcp *session got invalid address in this case?
>
> Regards,
> Ethan
>
> 2015-03-17 19:44 GMT+08:00 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>:
>>
>> Hi Ethan,
>>
>> On Tue, Mar 17, 2015 at 12:51 PM, Luiz Augusto von Dentz
>> <luiz.dentz@xxxxxxxxx> wrote:
>> > Hi Ethan,
>> >
>> > On Tue, Mar 17, 2015 at 12:13 PM, Ethan <ethancsge@xxxxxxxxx> wrote:
>> >> Hi Luiz,
>> >>
>> >> OK, I will follow the rule.
>> >> And actually, there have three crashes in function as
>> >> "avrcp_player_value_rsp", "avrcp_get_play_status_rsp" and
>> >> "avrcp_get_element_attributes_rsp". I tried to mark all code of
>> >> function
>> >> "avrcp_get_play_status_rsp" and return FALSE while finding first crash.
>> >> Then I built bluetoothd, and it crashed again in
>> >> avrcp_get_play_status_rsp.
>> >> The same way, next crash is in avrcp_get_element_attributes_rsp.
>> >>
>> >> I traced code and check issue log as attached message file, it seems
>> >> that
>> >> code "struct avrcp *session = user_data;" get invalid address in
>> >> function
>> >> avrcp_get_capabilities_resp. Also, I tried to create a same type
>> >> structure
>> >> and assign to session as below, and issue can not be reproduced. Hope
>> >> these
>> >> information can help you to find root cause. Thanks.
>> >>
>> >> --- a/profiles/audio/avrcp.c
>> >> +++ b/profiles/audio/avrcp.c
>> >> @@ -3222,10 +3222,12 @@ static gboolean
>> >> avrcp_get_capabilities_resp(struct
>> >> avctp *conn,
>> >> uint8_t *operands, size_t operand_count,
>> >> void *user_data)
>> >> {
>> >> - struct avrcp *session = user_data;
>> >> + struct avrcp test;
>> >> + struct avrcp *session = &test;
>> >>
>> >>
>> >> The attached file is backtrace for three crash by GDB
>> >>
>> >>
>> >> static gboolean avrcp_get_play_status_rsp(struct avctp *conn,
>> >> uint8_t code, uint8_t subunit,
>> >> uint8_t *operands, size_t operand_count,
>> >> void *user_data)
>> >> {
>> >> struct avrcp *session = user_data;
>> >> struct avrcp_player *player = session->controller->player;
>> >> struct media_player *mp = player->user_data; /*
>> >> --->crash */
>> >>
>> >>
>> >>
>> >> static gboolean avrcp_get_element_attributes_rsp(struct avctp *conn,
>> >> uint8_t code, uint8_t subunit,
>> >> uint8_t *operands,
>> >> size_t operand_count,
>> >> void *user_data)
>> >> {
>> >> struct avrcp *session = user_data;
>> >> struct avrcp_player *player = session->controller->player; /*
>> >> --->crash */
>> >>
>> >> static gboolean avrcp_player_value_rsp(struct avctp *conn,
>> >> uint8_t code, uint8_t subunit,
>> >> uint8_t *operands, size_t operand_count,
>> >> void *user_data)
>> >> {
>> >> struct avrcp *session = user_data;
>> >> struct avrcp_player *player = session->controller->player;
>> >> struct media_player *mp = player->user_data; /*
>> >> --->crash */
>> >>
>> >>
>> >> 2015-03-17T20:52:23.347640+11:00 DEBUG bluetoothd[21717]:
>> >> profiles/audio/avctp.c:avctp_connect_cb() AVCTP: connected to
>> >> A0:B4:A5:1F:56:B9
>> >> 2015-03-17T20:52:23.348292+11:00 DEBUG bluetoothd[21717]:
>> >> profiles/audio/avctp.c:init_uinput() AVRCP: uinput initialized for
>> >> A0:B4:A5:1F:56:B9
>> >> 2015-03-17T20:52:23.348337+11:00 DEBUG bluetoothd[21717]:
>> >> profiles/audio/avrcp.c:target_init() 0x7f601c964a20 version 0x0105
>> >
>> > Here seems to be the problem, it seems we only are initiating the
>> > target not the controller, which should be fine except that the remote
>> > will not be able to qualify with support of absolute volume control
>> > since that requires both records. Anyway there is no reason for us to
>> > crash even if the remote device is doing some strange stuff, we might
>> > need to check if controller is not initialized just volume control
>> > should be enabled.
>>
>> Could you try with these changes:
>>
>> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
>> index 414ee25..cc26eed 100644
>> --- a/profiles/audio/avrcp.c
>> +++ b/profiles/audio/avrcp.c
>> @@ -3252,12 +3252,18 @@ static gboolean
>> avrcp_get_capabilities_resp(struct avctp *conn,
>> case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
>> case AVRCP_EVENT_UIDS_CHANGED:
>> case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
>> + /* These events above are controller specific */
>> + if (!session->controller)
>> + break;
>> case AVRCP_EVENT_VOLUME_CHANGED:
>> avrcp_register_notification(session, event);
>> break;
>> }
>> }
>>
>> + if (!session->controller)
>> + return FALSE;
>> +
>> if (!(events & (1 << AVRCP_EVENT_SETTINGS_CHANGED)))
>> avrcp_list_player_attributes(session);
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
