Return-Path: MIME-Version: 1.0 In-Reply-To: References: Date: Fri, 20 Mar 2015 17:05:04 +0800 Message-ID: Subject: Re: Bluetoothd crash/segfault when Chrombook creates connection with Samsung gear circle From: Ethan To: Luiz Augusto von Dentz Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Thanks, I found this patch in upstream. 2015-03-19 10:57 GMT+08:00 Ethan : > 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 : >> 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 : >>> >>> Hi Ethan, >>> >>> On Tue, Mar 17, 2015 at 12:51 PM, Luiz Augusto von Dentz >>> wrote: >>> > Hi Ethan, >>> > >>> > On Tue, Mar 17, 2015 at 12:13 PM, Ethan 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 >> >> >>