Bluetooth: audio: Fix ASE error response

[szymon-czapracki]

This commits fixes building of ASE response in case of response code 0x01 or 0x02 (ref Table 4.7 ASE_v1.0 spec).

[szymon-czapracki]

@MariuszSkamra FYI

[MariuszSkamra]

It looks OK I guess, but could you add an __ASSERT here to make sure the size matches buffer length?
I guess it should look like

__ASSERT_NO_MSG(rsp_buf.len == (sizeof(*rsp) + sizeof(*ase_rsp) * rsp->num_ase));

[szymon-czapracki]

And why should we do that here? In case of code 0x01 or 0x02 we are clearing all of the specif ASE's information from response, and what we should be left with is only opcode, num of ase which we set to 0xff, and then we add "current" id, code and reason - so in case of those errors we always should have a response with the length of five, of course in other cases (when everything goes well) we can check whether the length is correct but the check should look something like this then:
if (rsp->num_ase == 0xff) {
__ASSERT_NO_MSG(rsp_buf.len == 5);
} else {
__ASSERT_NO_MSG(rsp_buf.len == (sizeof(*rsp) + sizeof(*ase_rsp) * rsp->num_ase));
}

[szymon-czapracki]

I implemented above check in the newest revision, tell me if this looks ok for you.

[jhedberg]

We should never assert in the case of invalid packets from a peer, whenever that can be handled cleanly. Normally the right thing to do is to use BT_ WARN() and discard the packet. Asserts on the other hand should be reserved for catching local bugs in our code.

[szymon-czapracki]

This is a response that we are building, due to an error on our side - the peer did not send us any invalid packets in this case.

[MariuszSkamra]

And why should we do that here? In case of code 0x01 or 0x02 we are clearing all of the specif ASE's information from response, and what we should be left with is only opcode, num of ase which we set to 0xff, and then we add "current" id, code and reason - so in case of those errors we always should have a response with the length of five, of course in other cases (when everything goes well) we can check whether the length is correct but the check should look something like this then: if (rsp->num_ase == 0xff) { __ASSERT_NO_MSG(rsp_buf.len == 5); } else { __ASSERT_NO_MSG(rsp_buf.len == (sizeof(*rsp) + sizeof(*ase_rsp) * rsp->num_ase)); }

@szymon-czapracki That's because you do cleanup here, it would be good IMO to __ASSERT in case the size of the buffer does not match the one you expect here, before removing the data from it.

if (rsp->num_ase) {
	__ASSERT_NO_MSG(rsp_buf.len == (sizeof(*rsp) + sizeof(*ase_rsp) * rsp->num_ase));
	net_buf_simple_remove_mem(&rsp_buf, sizeof(*ase_rsp) * rsp->num_ase);
}

We should never assert in the case of invalid packets from a peer, whenever that can be handled cleanly. Normally the right thing to do is to use BT_ WARN() and discard the packet. Asserts on the other hand should be reserved for catching local bugs in our code.

@jhedberg As @szymon-czapracki mentioned rsp_buf is the buffer that contains control point request response. The request may affect multiple ASE, thus the response buffer contains in fact multiple responses per ASE requested. But if the stack returns BT_ASCS_RSP_NOT_SUPPORTED or BT_ASCS_RSP_TRUNCATED the operation should be aborted and a single response sent. That is why @szymon-czapracki removes the data from it.

[MariuszSkamra]

It looks OK I guess, but could you add an __ASSERT here to make sure the size matches buffer length?
I guess it should look like

__ASSERT_NO_MSG(rsp_buf.len == (sizeof(*rsp) + sizeof(*ase_rsp) * rsp->num_ase));

[MariuszSkamra]

And why should we do that here? In case of code 0x01 or 0x02 we are clearing all of the specif ASE's information from response, and what we should be left with is only opcode, num of ase which we set to 0xff, and then we add "current" id, code and reason - so in case of those errors we always should have a response with the length of five, of course in other cases (when everything goes well) we can check whether the length is correct but the check should look something like this then: if (rsp->num_ase == 0xff) { __ASSERT_NO_MSG(rsp_buf.len == 5); } else { __ASSERT_NO_MSG(rsp_buf.len == (sizeof(*rsp) + sizeof(*ase_rsp) * rsp->num_ase)); }

@szymon-czapracki That's because you do cleanup here, it would be good IMO to __ASSERT in case the size of the buffer does not match the one you expect here, before removing the data from it.

if (rsp->num_ase) {
	__ASSERT_NO_MSG(rsp_buf.len == (sizeof(*rsp) + sizeof(*ase_rsp) * rsp->num_ase));
	net_buf_simple_remove_mem(&rsp_buf, sizeof(*ase_rsp) * rsp->num_ase);
}

We should never assert in the case of invalid packets from a peer, whenever that can be handled cleanly. Normally the right thing to do is to use BT_ WARN() and discard the packet. Asserts on the other hand should be reserved for catching local bugs in our code.

@jhedberg As @szymon-czapracki mentioned rsp_buf is the buffer that contains control point request response. The request may affect multiple ASE, thus the response buffer contains in fact multiple responses per ASE requested. But if the stack returns BT_ASCS_RSP_NOT_SUPPORTED or BT_ASCS_RSP_TRUNCATED the operation should be aborted and a single response sent. That is why @szymon-czapracki removes the data from it.

[MariuszSkamra]

That's not what I meant

[Thalley]

The point of this is to remove all previously added responses, so that the buffer only contains a single bt_ascs_cp_ase_rsp in the end, right? Or is it no bt_ascs_cp_ase_rsp?

I am wondering if it would be simpler to simply recreate the buffer from new in that case, instead of removing the memory here. Performance wise it looks like it'll amount to roughly the same.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.