Fix WiFiClientSecure remoteIP(), remotePort(), localIP(), localPort() functions

[JiriBilek]

Fixes the https://github.com/esp8266/Arduino/issues/8692

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

[mcspr]

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]

I find the multiple inheritance of WiFiClient (in WiFiClientSecure and WiFiClientSecureCtx) confusing. Having read the comment https://github.com/esp8266/Arduino/blob/d3eddeb5014d04aa9e61b97474ef98d0214eab1a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h#L237 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]

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]

Hi, thanks for finding the https://github.com/esp8266/Arduino/pull/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]

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]

(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]

... and also ref. https://github.com/esp8266/Arduino/pull/7612#issuecomment-700942104 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]

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]

Likely, not closing anything yet.