Skip to content

Bluetooth: audio: Fix ASE error response #50251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Bluetooth: audio: Fix ASE error response #50251

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions subsys/bluetooth/audio/ascs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,10 @@ static void ascs_cp_rsp_add(uint8_t id, uint8_t op, uint8_t code,
*/
case BT_ASCS_RSP_NOT_SUPPORTED:
case BT_ASCS_RSP_TRUNCATED:
if (rsp->num_ase) {
net_buf_simple_remove_mem(&rsp_buf,
sizeof(*ase_rsp) * rsp->num_ase);
}
Comment on lines +644 to +647
Copy link
Contributor

@MariuszSkamra MariuszSkamra Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Contributor Author

@szymon-czapracki szymon-czapracki Sep 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
}

Copy link
Contributor Author

@szymon-czapracki szymon-czapracki Sep 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jhedberg jhedberg Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@szymon-czapracki szymon-czapracki Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MariuszSkamra MariuszSkamra Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +644 to +647
Copy link
Contributor

@Thalley Thalley Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rsp->num_ase = 0xff;
break;
default:
Expand All @@ -652,6 +656,12 @@ static void ascs_cp_rsp_add(uint8_t id, uint8_t op, uint8_t code,
ase_rsp->id = id;
ase_rsp->code = code;
ase_rsp->reason = reason;

if (rsp->num_ase == 0xff) {
__ASSERT_NO_MSG(rsp_buf.len == sizeof(*rsp) + sizeof(*ase_rsp));
} else {
__ASSERT_NO_MSG(rsp_buf.len == (sizeof(*rsp) + sizeof(*ase_rsp) * rsp->num_ase));
}
}

static void ascs_cp_rsp_add_errno(uint8_t id, uint8_t op, int err,
Expand Down