From 882aec320e88b0860a3d53db828bb099c7ca2409 Mon Sep 17 00:00:00 2001 From: Jack He Date: Mon, 14 Aug 2017 23:02:16 -0700 Subject: [PATCH] Fix errors in handling RawAddresses * In change I8d1bd6914aec55bb53495b1d0d5e3d37b86865e6 memcmp(a, b, LEN) != 0 should be translated to A != B * memcpy should not be applied to RawAddress objects. Assignment operator should be used instead. * memset should not be applied to RawAddress objects. Assignment to RawAddress::kEmpty should be used. * Fixed a crash in GATT Bug: 64316340 Test: Unit test, pair with device and transmit Change-Id: Iceefab821c1d45a88194d87a43a192afa5f263fd --- btif/src/btif_av.cc | 30 +++++++++++++----------------- btif/src/btif_core.cc | 6 +++--- btif/src/btif_profile_queue.cc | 2 +- service/low_energy_client.h | 2 +- stack/btm/btm_sec.cc | 2 +- stack/gatt/gatt_sr.cc | 1 + stack/l2cap/l2c_api.cc | 4 ++-- test/suite/rfcomm/rfcomm_unittest.cc | 5 ++--- 8 files changed, 24 insertions(+), 28 deletions(-) diff --git a/btif/src/btif_av.cc b/btif/src/btif_av.cc index bc9c282d5..8074f1fa0 100644 --- a/btif/src/btif_av.cc +++ b/btif/src/btif_av.cc @@ -333,7 +333,7 @@ static bool btif_av_state_idle_handler(btif_sm_event_t event, void* p_data) { switch (event) { case BTIF_SM_ENTER_EVT: /* clear the peer_bda */ - memset(&btif_av_cb.peer_bda, 0, sizeof(RawAddress)); + btif_av_cb.peer_bda = RawAddress::kEmpty; btif_av_cb.flags = 0; btif_av_cb.edr = 0; bta_av_co_init(btif_av_cb.codec_priorities); @@ -353,12 +353,10 @@ static bool btif_av_state_idle_handler(btif_sm_event_t event, void* p_data) { case BTA_AV_PENDING_EVT: case BTIF_AV_CONNECT_REQ_EVT: { if (event == BTIF_AV_CONNECT_REQ_EVT) { - memcpy(&btif_av_cb.peer_bda, - ((btif_av_connect_req_t*)p_data)->target_bda, - sizeof(RawAddress)); + btif_av_connect_req_t* connect_req_p = (btif_av_connect_req_t*)p_data; + btif_av_cb.peer_bda = *connect_req_p->target_bda; BTA_AvOpen(btif_av_cb.peer_bda, btif_av_cb.bta_handle, true, - BTA_SEC_AUTHENTICATE, - ((btif_av_connect_req_t*)p_data)->uuid); + BTA_SEC_AUTHENTICATE, connect_req_p->uuid); } else if (event == BTA_AV_PENDING_EVT) { btif_av_cb.peer_bda = ((tBTA_AV*)p_data)->pend.bd_addr; if (bt_av_src_callbacks != NULL) { @@ -599,25 +597,23 @@ static bool btif_av_state_opening_handler(btif_sm_event_t event, void* p_data) { } } break; - case BTIF_AV_CONNECT_REQ_EVT: + case BTIF_AV_CONNECT_REQ_EVT: { // Check for device, if same device which moved to opening then ignore // callback - if (memcmp(((btif_av_connect_req_t*)p_data)->target_bda, - &(btif_av_cb.peer_bda), sizeof(btif_av_cb.peer_bda)) == 0) { + btif_av_connect_req_t* connect_req_p = (btif_av_connect_req_t*)p_data; + if (btif_av_cb.peer_bda == *connect_req_p->target_bda) { BTIF_TRACE_DEBUG( "%s: Same device moved to Opening state,ignore Connect Req", __func__); - btif_queue_advance(); - break; } else { BTIF_TRACE_DEBUG("%s: Moved from idle by Incoming Connection request", __func__); btif_report_connection_state( BTAV_CONNECTION_STATE_DISCONNECTED, ((btif_av_connect_req_t*)p_data)->target_bda); - btif_queue_advance(); - break; } + btif_queue_advance(); + } break; case BTA_AV_PENDING_EVT: // Check for device, if same device which moved to opening then ignore @@ -869,9 +865,9 @@ static bool btif_av_state_opened_handler(btif_sm_event_t event, void* p_data) { } break; - case BTIF_AV_CONNECT_REQ_EVT: - if (memcmp((RawAddress*)p_data, &(btif_av_cb.peer_bda), - sizeof(btif_av_cb.peer_bda)) == 0) { + case BTIF_AV_CONNECT_REQ_EVT: { + btif_av_connect_req_t* connect_req_p = (btif_av_connect_req_t*)p_data; + if (btif_av_cb.peer_bda == *connect_req_p->target_bda) { BTIF_TRACE_DEBUG("%s: Ignore BTIF_AV_CONNECT_REQ_EVT for same device", __func__); } else { @@ -881,7 +877,7 @@ static bool btif_av_state_opened_handler(btif_sm_event_t event, void* p_data) { (RawAddress*)p_data); } btif_queue_advance(); - break; + } break; case BTIF_AV_OFFLOAD_START_REQ_EVT: BTIF_TRACE_ERROR( diff --git a/btif/src/btif_core.cc b/btif/src/btif_core.cc index 43da993fa..1ac2ae62f 100644 --- a/btif/src/btif_core.cc +++ b/btif/src/btif_core.cc @@ -992,7 +992,7 @@ bt_status_t btif_set_adapter_property(const bt_property_t* property) { if (storage_req_id != BTIF_CORE_STORAGE_NO_ACTION) { /* pass on to storage for updating local database */ - memset(&(req.write_req.bd_addr), 0, sizeof(RawAddress)); + req.write_req.bd_addr = RawAddress::kEmpty; memcpy(&(req.write_req.prop), property, sizeof(bt_property_t)); return btif_transfer_context(execute_storage_request, storage_req_id, @@ -1019,7 +1019,7 @@ bt_status_t btif_get_remote_device_property(RawAddress* remote_addr, if (!btif_is_enabled()) return BT_STATUS_NOT_READY; - memcpy(&(req.read_req.bd_addr), remote_addr, sizeof(RawAddress)); + req.read_req.bd_addr = *remote_addr; req.read_req.type = type; return btif_transfer_context(execute_storage_remote_request, BTIF_CORE_STORAGE_REMOTE_READ, (char*)&req, @@ -1040,7 +1040,7 @@ bt_status_t btif_get_remote_device_properties(RawAddress* remote_addr) { if (!btif_is_enabled()) return BT_STATUS_NOT_READY; - memcpy(&(req.read_req.bd_addr), remote_addr, sizeof(RawAddress)); + req.read_req.bd_addr = *remote_addr; return btif_transfer_context(execute_storage_remote_request, BTIF_CORE_STORAGE_REMOTE_READ_ALL, (char*)&req, sizeof(btif_storage_req_t), NULL); diff --git a/btif/src/btif_profile_queue.cc b/btif/src/btif_profile_queue.cc index bdf7fc350..4d2efef5a 100644 --- a/btif/src/btif_profile_queue.cc +++ b/btif/src/btif_profile_queue.cc @@ -122,7 +122,7 @@ bt_status_t btif_queue_connect(uint16_t uuid, const RawAddress* bda, btif_connect_cb_t connect_cb) { connect_node_t node; memset(&node, 0, sizeof(connect_node_t)); - memcpy(&node.bda, bda, sizeof(RawAddress)); + node.bda = *bda; node.uuid = uuid; node.connect_cb = connect_cb; diff --git a/service/low_energy_client.h b/service/low_energy_client.h index ea048bb68..16ef4a5a4 100644 --- a/service/low_energy_client.h +++ b/service/low_energy_client.h @@ -35,7 +35,7 @@ namespace bluetooth { struct ConnComparator { bool operator()(const RawAddress& a, const RawAddress& b) const { - return memcmp(&a, &b, sizeof(RawAddress)) < 0; + return memcmp(a.address, b.address, RawAddress::kLength) < 0; } }; diff --git a/stack/btm/btm_sec.cc b/stack/btm/btm_sec.cc index bf721715e..263db4653 100644 --- a/stack/btm/btm_sec.cc +++ b/stack/btm/btm_sec.cc @@ -4984,7 +4984,7 @@ void btm_sec_pin_code_request(const RawAddress& p_bda) { btsnd_hcic_pin_code_neg_reply(p_bda); return; } else if ((btm_cb.pairing_state != BTM_PAIR_STATE_WAIT_PIN_REQ) || - p_bda == btm_cb.pairing_bda) { + p_bda != btm_cb.pairing_bda) { BTM_TRACE_WARNING("btm_sec_pin_code_request() rejected - state: %s", btm_pair_state_descr(btm_cb.pairing_state)); btsnd_hcic_pin_code_neg_reply(p_bda); diff --git a/stack/gatt/gatt_sr.cc b/stack/gatt/gatt_sr.cc index 37774d8d1..211e57c9b 100644 --- a/stack/gatt/gatt_sr.cc +++ b/stack/gatt/gatt_sr.cc @@ -644,6 +644,7 @@ void gatts_process_primary_service_req(tGATT_TCB& tcb, uint8_t op_code, if (reason != GATT_SUCCESS) { osi_free(p_msg); gatt_send_error_rsp(tcb, reason, op_code, s_hdl, false); + return; } attp_send_sr_msg(tcb, p_msg); diff --git a/stack/l2cap/l2c_api.cc b/stack/l2cap/l2c_api.cc index b921f5133..6e3fd3ce6 100644 --- a/stack/l2cap/l2c_api.cc +++ b/stack/l2cap/l2c_api.cc @@ -1070,7 +1070,7 @@ bool L2CA_SetIdleTimeoutByBdAddr(const RawAddress& bd_addr, uint16_t timeout, tBT_TRANSPORT transport) { tL2C_LCB* p_lcb; - if (RawAddress::kAny == bd_addr) { + if (RawAddress::kAny != bd_addr) { p_lcb = l2cu_find_lcb_by_bd_addr(bd_addr, transport); if ((p_lcb) && (p_lcb->in_use) && (p_lcb->link_state == LST_CONNECTED)) { p_lcb->idle_timeout = timeout; @@ -1422,7 +1422,7 @@ bool L2CA_SetFlushTimeout(const RawAddress& bd_addr, uint16_t flush_tout) { } } - if (RawAddress::kAny == bd_addr) { + if (RawAddress::kAny != bd_addr) { p_lcb = l2cu_find_lcb_by_bd_addr(bd_addr, BT_TRANSPORT_BR_EDR); if ((p_lcb) && (p_lcb->in_use) && (p_lcb->link_state == LST_CONNECTED)) { diff --git a/test/suite/rfcomm/rfcomm_unittest.cc b/test/suite/rfcomm/rfcomm_unittest.cc index cb9cfe78e..eabe73c92 100644 --- a/test/suite/rfcomm/rfcomm_unittest.cc +++ b/test/suite/rfcomm/rfcomm_unittest.cc @@ -49,7 +49,7 @@ TEST_F(RFCommTest, RfcommConnectPairedDevice) { EXPECT_TRUE(len == sizeof(signal)) << "Connection signal not read from RFCOMM socket. Bytes read: " << len; - EXPECT_TRUE(!memcmp(&signal.bd_addr, &bt_remote_bdaddr_, sizeof(RawAddress))) + EXPECT_TRUE(signal.bd_addr == bt_remote_bdaddr_) << "Connected to a different bdaddr than expected."; EXPECT_TRUE(channel == signal.channel) << "Inconsistent channels returned: " << channel << " and " @@ -99,8 +99,7 @@ TEST_F(RFCommTest, RfcommRepeatedConnectPairedDevice) { signal_fail++; } - EXPECT_TRUE( - !memcmp(&signal.bd_addr, &bt_remote_bdaddr_, sizeof(RawAddress))) + EXPECT_TRUE(signal.bd_addr == bt_remote_bdaddr_) << "Connected to a different bdaddr than expected."; EXPECT_TRUE(channel == signal.channel) << "Inconsistent channels returned: " << channel << " and " -- 2.11.0