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?