Skip to content

Bluetooth: SDP: Fix issue that RX buf is used for others discovery #94552

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

Merged
merged 1 commit into from
Aug 22, 2025
Merged

Bluetooth: SDP: Fix issue that RX buf is used for others discovery #94552

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: 9 additions & 1 deletion subsys/bluetooth/host/classic/sdp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1705,6 +1705,10 @@ static void sdp_client_params_iterator(struct bt_sdp_client *session)
sys_slist_remove(&session->reqs, NULL, &param->_node);
/* Invalidate cached param in context */
session->param = NULL;
if (session->rec_buf != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

current session rec_buf would be alloc when sdp_client_connected. if do alloc rec_buf when user request every discovery. then rec_buf maybe need to delete within sdp_client_connected.

However I am some doubts.
recv_len would be reset when discovery finished. why rec_buf would be some garbage data？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not get your point.

As my comments said, the received buffer is incorrectly used for other SDP discover requests.

This is why this change created.

net_buf_unref(session->rec_buf);
session->rec_buf = NULL;
}
/* Reset continuation state in current context */
(void)memset(&session->cstate, 0, sizeof(session->cstate));
/* Clear total length */
Expand Down Expand Up @@ -1912,7 +1916,11 @@ static int sdp_client_discover(struct bt_sdp_client *session)
param = session->param;
}

if (!param) {
if (param != NULL && session->rec_buf == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The session->rec_buf should be NULL here, add assert or LOG_ERR here? Or If it is not NULL, release it here?

Copy link
Contributor Author

@lylezhu2012 lylezhu2012 Aug 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The change point is,

  1. release session->rec_buf when removing the current req (it occurs in function sdp_client_params_iterator()),
  2. allocate session->rec_buf when starting a new req (it occurs in function sdp_client_discover())

So at line 1919, the session->rec_buf is not NULL for first request. For other requests, it is NULL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move all the alloc buf to the same place (even for the first request)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For most cases, I think only one request will be performed in the SDP connection session. So I think it is reasonable to allocate the session->rec_buf for the first request after the SDP session connection established.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, moving the alloc buf of the first request to the same place sdp_client_discover doesn't break the original code logic and make the codes more clear.
In sdp_client_connected, session->rec_buf = chan->ops->alloc_buf(chan); is used to alloc buf, I think it is better to use the same way as your change.

Using the same way and putting the same place make the code to be more easy to maintain.

Copy link
Contributor Author

@lylezhu2012 lylezhu2012 Aug 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well. But it is not this PR purpose. Over-fixing is not a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, another pr is needed.

session->rec_buf = net_buf_alloc(param->pool, K_FOREVER);
}

if (param == NULL || session->rec_buf == NULL) {
struct bt_l2cap_chan *chan = &session->chan.chan;

session->state = SDP_CLIENT_DISCONNECTING;
Expand Down