Skip to content

fix error handler segfault when key file is missing #223

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

Closed
wants to merge 1 commit into from

Conversation

CypherGrue
Copy link

When a missing key file error is handled by update_bredr_services, g_error_free was used to clear the error. However, this left the gerr local pointer as non-NULL. The pointer was then re-used in g_file_set_contents, which immediately returns, because it expects its error pointer parameter to be NULL. The next error handler then handles the original (g_error_free'd) error pointer again, incorrectly accessing gerr->message and calling g_error_free on the same pointer a second time, resulting in a segfault.

This change replaces g_error_free with g_clear_error, to ensure that the error pointer is set to NULL once it has been handled. Tested this fixes the issue for my configuration of CSR USB dongle 0a12:001 with Mi Smart Band 4.

When a missing key file error is handled by update_bredr_services, g_error_free was used to clear the error. However, this left the gerr local pointer as non-NULL. The pointer was then re-used in g_file_set_contents, which immediately returns, because it expects its error pointer parameter to be NULL. The next error handler then handles the original (g_error_free'd) error pointer again, incorrectly accessing gerr->message and calling g_error_free on the same pointer a second time, resulting in a segfault.

This change replaces g_error_free with g_clear_error, to ensure that the error pointer is set to NULL once it has been handled.
@Vudentz
Copy link
Contributor

Vudentz commented Oct 28, 2021

@CypherGrue
Copy link
Author

CypherGrue commented Oct 28, 2021

Suggested patch change by @Vudentz still potentially reuses gerr in the last section of the function. If both key files are initially loaded ok and the first write fails (as unlikely as that may be), the second will crash. Please see my original patch for the lines to be replaced (3rd line). Reason for the last of my 4 line changes is to prevent the mistake from being copy-pasted to recreate the problem somewhere else. It would be worthwhile to grep the codebase to see if there are similar existing cases (for those incentivised to do so).

@Vudentz
Copy link
Contributor

Vudentz commented Oct 28, 2021

@BluezTestBot
Copy link

BlueZ Testbot Message #3:

This is an automated message and please do not change or delete.

Dear submitter,

Thanks for submitting the pull request to the BlueZ github repo.
Currently, the BlueZ repo in Github is only for CI and testing purposes,
and not accepting any pull request at this moment.

If you still want us to review your patch and merge them, please send your
patch to the Linux Bluetooth mailing list(linux-bluetooth@vger.kernel.org).

For more detail about submitting a patch to the mailing list,
Please refer "Submitting patches" section in the HACKING file in the source.

Note that this pull request will be closed in the near future.

Best regards,
BlueZ Team

@BluezTestBot
Copy link

BlueZ Testbot Message #4:

This is an automated message and please do not change or delete.

Closing without taking any action.

Best regards,
BlueZ Team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants