Skip to content
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.

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

Fix a bug in bluedroid when multiple SDP records are created (IDFGH-9219) #10607

Merged
merged 1 commit into from Feb 3, 2023

Conversation

AlbertWDev
Copy link
Contributor

@AlbertWDev AlbertWDev commented Jan 24, 2023

Definition of the bug fixed by this PR

When creating multiple SDP records using the esp_sdp_create_record() function, the first call raises the ESP_SDP_CREATE_RECORD_COMP_EVT event with an status of ESP_SDP_SUCCESS, as expected.
However, the following calls to register other SDP records generate events where the status and the handle parameters of the esp_sdp_cb_param_t->create_record struct have the same value, so the status is not considered a success and SDP records are not actually created, making it impossible to define more than one SDP record.
This happens because those values are set in the function bta_sdp_create_record(), where a tBTA_SDP union is declared and two of its possible values are written at the same time (status and handle).

For reference, this was the tBTA_SDP definition:

typedef union {
    tBTA_SDP_STATUS              status;            /* BTA_SDP_SEARCH_EVT */
    tBTA_SDP_SEARCH_COMP         sdp_search_comp;   /* BTA_SDP_SEARCH_COMP_EVT */
    int                          handle;
} tBTA_SDP;

And this is the relevant code of the bta_sdp_create_record() function where the status was overwritten:

void bta_sdp_create_record(tBTA_SDP_MSG *p_data)
{
    // [...]
    tBTA_SDP bta_sdp;
    bta_sdp.status = BTA_SDP_SUCCESS;
    bta_sdp.handle = (int)p_data->record.user_data;  // <-- Overwrites the 'bta_sdp.status' value
    // [...]
}

How the bug was fixed

This PR fixes this situation by creating a tBTA_SDP_CREATE_RECORD_USER struct with the two required values (mimicking the already defined tBTA_SDP_SEARCH_COMP struct), so setting the SDP record handle doesn't overwrite the status value.

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2023

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Jan 24, 2023
@github-actions github-actions bot changed the title Fix a bug in bluedroid when multiple SDP records are created Fix a bug in bluedroid when multiple SDP records are created (IDFGH-9219) Jan 24, 2023
@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Jan 30, 2023
@xiongweichao
Copy link
Collaborator

Hi @AlbertWDev ,

Please rebase master.

Thanks

@AlbertWDev
Copy link
Contributor Author

Hi @xiongweichao, I've just rebased master

@@ -1132,7 +1132,7 @@ void btc_sdp_cb_handler(btc_msg_t *msg)
break;
case BTA_SDP_CREATE_RECORD_USER_EVT:
param.create_record.status = p_data->status;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think p_data->sdp_create_record.status is better than p_data->status here.

Copy link
Contributor Author

@AlbertWDev AlbertWDev Feb 1, 2023

Choose a reason for hiding this comment

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

Thanks for the review! I totally agree, I was just mimicking the behaviour on the BTA_SDP_SEARCH_COMP_EVT, were p_data->status is used instead of p_data->sdp_search_comp.status. There should be no difference in terms of accessed memory, but I will update both to make the code clearer.

Edit: I just changed it (amended in the same commit to keep the commit history clean)

@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable labels Feb 2, 2023
@espressif-bot espressif-bot merged commit fb19027 into espressif:master Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants