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

Fixes HWCDC::end() #8247

Merged
merged 2 commits into from
May 31, 2023
Merged

Fixes HWCDC::end() #8247

merged 2 commits into from
May 31, 2023

Conversation

SuGlider
Copy link
Collaborator

Description of Change

If we call serial.end() followed by serial.begin using JTAG/CDC interface, it will never initialize the semaphore tx_lock and it will reset the board when data is written.
There is a missing tx_lock = NULL; in the HWCDC::end() function.

Tests scenarios

Tested with C3 and S3.

Related links

Fixes #8224

There is a missing `tx_lock = NULL;`
@SuGlider SuGlider added this to the 2.0.9 milestone May 24, 2023
@SuGlider SuGlider requested a review from me-no-dev May 24, 2023 23:32
@SuGlider SuGlider self-assigned this May 24, 2023
@SuGlider SuGlider modified the milestones: 2.0.9, 2.0.10 May 24, 2023
Copy link
Collaborator

@mrengineer7777 mrengineer7777 left a comment

Choose a reason for hiding this comment

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

LGTM

@TD-er
Copy link
Contributor

TD-er commented May 25, 2023

Yep, very complex fix :)
LGTM.

I will also have a quick look at the other static values to see if we might have missed others too.

@TD-er
Copy link
Contributor

TD-er commented May 25, 2023

While we're at it, maybe also initialize the rx_data_buf to 0?

static uint8_t rx_data_buf[64]{};

I did a quick search for other calls of vSemaphoreDelete and there are more places where the pointer isn't set to NULL afterwards. This isn't an issue In destructors where the pointer used is a member.
But in esp32-hal-tinyusb.c it is a good candidate for future bugs when that function is later extended. (now it is a local variable inside that function and the function is left after this delete call)

Initialize rx_data_buf[] with zeros.
@SuGlider
Copy link
Collaborator Author

While we're at it, maybe also initialize the rx_data_buf to 0?

Ok, done.

I did a quick search for other calls of vSemaphoreDelete and there are more places where the pointer isn't set to NULL afterwards. This isn't an issue In destructors where the pointer used is a member. But in esp32-hal-tinyusb.c it is a good candidate for future bugs when that function is later extended. (now it is a local variable inside that function and the function is left after this delete call)

esp32-hal-tinyusb.c uses the semaphore internally only.
It will never be extended because it is just used to check the installation of the JTAG driver.

I found vSemaphoreDelete with "no NULL" in HardwareSerial::~HardwareSerial() and TwoWire::~TwoWire(), but those are the destructors and all object memory will be gone anyway. The construtor will set it to NULL.

All other places are fine.

@me-no-dev me-no-dev merged commit 083db16 into espressif:master May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

HWCDC::end() does not set tx_lock to NULL, causing crashes
4 participants