fix(websocket): fix setup logic in the external transport (IDFGH-15250)

[huming2207]

Description

Hi all,

I made a dumb mistake when I was submitting the PR: #573

I somehow forgot two branch logic when setting up the websocket client transport. These two logic exist in our company's codebase and we have used it for a year already, but somehow was missing in the PR to this upstream codebase. Without these two conditions, the websocket client will create a esp_transport handle anyhow, no matter the ext_transport was provided or not. This will break the proxy support of course.

Anyway, here's the fix. Sorry again for the trouble I made.

Regards,
Jackson

Related

PR related:

• feat(websocket): allow using external TCP transport handle (IDFGH-12825) #573
• fix(websocket): don't fetch transport from the list if external transport is set #616

Testing

These two logic exist in our company's codebase and we have used it for a year already in our products.

Checklist

Before submitting a Pull Request, please ensure the following:

• 🚨 This PR does not introduce breaking changes.
• All CI checks (GH Actions) pass.
• Documentation is updated as needed.
• Tests are updated or added as necessary.
• Code is well-commented, especially in complex areas.
• Git history is clean — commits are squashed to the minimum necessary.

[huming2207]

Sorry, we internally reviewed and we think we have more issues to fix. I will fix it first and then let you know we are ready.

[huming2207]

Okay now I'm ready for review.

This also fixes some potential memory corruptions. Previously if I use an externally created proxy handle that is not a transport_ssl or transport_tcp handle, and if some options like keep alive was set, the esp_transport_ssl_set_*() will be called and eventually corrupts the proxy handle context. Because the type is different.

[huming2207]

Hello @glmfe , may I ask is there any feedback on this PR?

[huming2207]

Any updates...? @glmfe

[huming2207]

Hello......??

[euripedesrocha]

Hi @huming2207, thanks for the contribution, and sorry for the delay.

I need to double-check the code because we shouldn't be reaching that logic.

Second and most important, when adding the external transport we should be adding the full transport, not only the parent. To be honest, I'm undecided on what is more confusing to users.

At this point, I believe that the simple approach is that the external transport must be the full transport implementation, not only the parent for transport_ws.

[huming2207]

Hi @euripedesrocha

Thanks for the followup.

I need to double-check the code because we shouldn't be reaching that logic.

Nah I think the original implementation has already caused us heap corruptions.

At this point, I believe that the simple approach is that the external transport must be the full transport implementation, not only the parent for transport_ws.

Yea, and the original implementation will simply doesn't work and cause memory corruptions. I think we need something that actually work at least... 😅

Jackson

[euripedesrocha]

@huming2207 I would like to understand your problem here.

The original version of this feature seems to take the correct approach.

When adding an external transport, I expect that the user needs to set all parameters of the transport and the library should work with what was added. For this library it means that you need to initialize the parent and pass it to websocket_ts or even implement the base transport completely. I agree that the first case of adding the parent is the most common.

Can you share a small reproducer that causes the memory corruption?

[huming2207]

Sorry @euripedesrocha I was a bit busy earlier as well 😅

When adding an external transport, I expect that the user needs to set all parameters of the transport and the library should work with what was added. For this library it means that you need to initialize the parent and pass it to websocket_ts or even implement the base transport completely. I agree that the first case of adding the parent is the most common.

Can you share a small reproducer that causes the memory corruption?

Ah yes I think you are right. I misread the code and the esp_transport_tcp_set_***() won't apply to the external transport because I already have a guard here anyway:

esp-protocols/components/esp_websocket_client/esp_websocket_client.c

Line 1159 in f43dd50

if (!client->transport) {

But yes for fb22749 , This one I think is necessary. Otherwise it won't actually use my external transport at all.

Sorry for the trouble. I will remove this commit: 76b475e

EDIT: I will recheck a bit later. I think I misread something badly.

[huming2207]

Done. Could you please check again? Thanks! @euripedesrocha

No sorry I think I misread my code again. Let me recheck a bit later. I think both commits are actually necessary. I'm now busy fixing some other bugs and I will re-address this issue maybe tomorrow or early next week.

[huming2207]

Sorry I will come back on this a bit later. I think I might did something wrong in the beginning. I will come back to this maybe next week after I fix some other urgent issues.

[huming2207]

Hi @euripedesrocha

I think I know why now. I think our company's esp-websocket-client codebase probably somehow messed up and missing the patch from this PR: #616

Thus you were indeed correct, there's probably no issue from the beginning. Sorry for the trouble...😅

I will close this PR for now and I will test again tomorrow. If we find something new I will open a new discussion and provide more details.

Regards,
Jackson

[euripedesrocha]

@huming2207 thanks for taking a look! If you can provide a small sample on how you are using the feature, is also useful for me to understand how to address expectations and document better.