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 WiFiClientSecure remoteIP(), remotePort(), localIP(), localPort() functions #8693

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

JiriBilek
Copy link
Contributor

Fixes the #8692

Fixes the incorrect behavior of WiFiClientSecure.remoteIP(), .remotePort(), .localIP(), .localPort().

Fixes the incorrect behavior of WiFiClientSecure.remoteIP(), .remotePort(), .localIP(), .localPort().
@mcspr
Copy link
Collaborator

mcspr commented Oct 15, 2022

Notice that keep alive also has this problem. Could we go about fixing the inheritance / class access instead of proxying these methods or adding even more virtual calls?

@JiriBilek
Copy link
Contributor Author

I find the multiple inheritance of WiFiClient (in WiFiClientSecure and WiFiClientSecureCtx) confusing. Having read the comment

// WiFiClient's "ClientContext* _client" is always nullptr in this class.

I thought the change was made on purpose and I didn't think of it more.

IMO changing the inheritance would result in lot of work quite close to the rule 'if it ain't broken don't fix it'. Although, I know, it is a bit broken.

@mcspr
Copy link
Collaborator

mcspr commented Oct 15, 2022

I find the multiple inheritance of WiFiClient (in WiFiClientSecure and WiFiClientSecureCtx) confusing
IMO changing the inheritance would result in lot of work quite close to the rule 'if it ain't broken don't fix it'. Although, I know, it is a bit broken.

Main issue is, bit by bit it becomes even more broken and not really fixed. Plus, anyone reading this now or in the future either thinks C++ is insane or this is the right way to do this kind of thing (whatever the thing is). So am leaning more to fixing the overall situation instead of patching it here and there. With the same idea as #7680, just doing it a bit different

More comments were added after the fact, when the clone() was introduced to fix object copies, but that was more of a sidenote thing to understand what's happening there. And lack of time was another factor of not really fixing anything

@JiriBilek
Copy link
Contributor Author

Hi,
thanks for finding the #7680, I thought this must have been done on purpose.
I am going to read the PR and related info to learn why exactly the change was done this way.

I agree with you that doing multiple fixes makes the code bad and hard to maintain.
I don't know of a case where this erratic behavior of WiFiClientSecure.remoteIP() would cause problems. I came to it by chance while testing my code. So this is not a priority issue, I suppose.

@JAndrassy
Copy link
Contributor

I see only two options how to solve remoteIP & co. if they should work for WiFiClientSecure executed over WiFiClient pointer.

  1. make remoteIP & co. in WiFiClient virtual as this PR does
  2. make remoteIP of WiFiClient work for WiFiClientSecure meaning WiFiClientSecureCtx implementing ClientContext, which would require virtual methods in ClientContext

@mcspr
Copy link
Collaborator

mcspr commented Oct 25, 2022

(edit: I am just rude :/ misread the above)

make remoteIP & co. in WiFiClient virtual as this PR does

only for the reasons described above - because the way it was written originally!

make remoteIP of WiFiClient work for WiFiClientSecure meaning WiFiClientSecureCtx implementing ClientContext, which would require virtual methods in ClientContext

nothing of sorts, just making it possible to copy wificlient and manage clientcontext with something. i.e. handle refs, unrefs, and removing another weird quirk of self-deleting itself on refcnt zero.

So far my understanding is no-one had time to do this, so I will PR my changes instead as soon as I figure out where to put keep alive consts and flush timeout. Meaning, we revert to the old WiFiClientSecure : WiFiClient inheritance without an extra class and just use the original methods to get remote & local address info.

@mcspr
Copy link
Collaborator

mcspr commented Oct 25, 2022

... and also ref. #7612 (comment)
Meaning, TLS state needs to be the same between copies. Not necessarily in the ClientContext, perhaps in a second helper object (and which is what I am adapting atm)

@JAndrassy
Copy link
Contributor

but in a perspective of the 3.1.0 release? maybe merge this PR for 3.1.0 and make the larger rewrite for 3.2.0?

@mcspr
Copy link
Collaborator

mcspr commented Oct 26, 2022 via email

@mcspr mcspr merged commit 5f94a60 into esp8266:master Oct 31, 2022
@mcspr mcspr added this to the 3.1 milestone Nov 2, 2022
@JiriBilek JiriBilek deleted the bugfix/ClientSecure branch November 18, 2022 16:41
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