Make sure that TLSSocketWrapper::close() is called before the transport is destroyed.

[SeppoTakalo]

Description

Transport is a member of TLSSocket which is derived from TLSSocketWrapper.
Make sure that TLSSocketWrapper::close() is called before the transport is
destroyed.

If you deleted the TLSSocket class without closing it first, it lead to mbed_error() and crashed the device.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

[SeppoTakalo]

@kjbracey-arm please review

@michalpasztamobica FYI

[SeppoTakalo]

@cmonr This has already been reviewed by Kevin. So please set the status to "Needs: CI" or start CI.

Thanks.

[cmonr]

/morph build

[mbed-ci]

Build : SUCCESS

Build number : 3538
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8613/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

[mbed-ci]

Exporter Build : SUCCESS

Build number : 3149
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8613/

[mbed-ci]

Test : FAILURE

Build number : 3322
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8613/3322

[NirSonnenschein]

test failure seems unrelated, re-trying
/morph build

[mbed-ci]

Build : SUCCESS

Build number : 3539
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8613/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

[mbed-ci]

Exporter Build : SUCCESS

Build number : 3150
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8613/

[mbed-ci]

Test : FAILURE

Build number : 3323
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8613/3323

[cmonr]

/morph test

[mbed-ci]

Test : SUCCESS

Build number : 3329
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8613/3329

[michalpasztamobica]

This change is fully legit, but I would also move the _transport = NULL; assignment from the close function to the TLSSocketWrapper destructor. Otherwise we cannot reopen a closed socket...

[0xc0170]

This change is fully legit, but I would also move the _transport = NULL; assignment from the close function to the TLSSocketWrapper destructor. Otherwise we cannot reopen a closed socket...

As this is now ready, this can be done separately

[adbridge]

This functionality is only coming in for 5.11

[boraozgen]

This change is fully legit, but I would also move the _transport = NULL; assignment from the close function to the TLSSocketWrapper destructor. Otherwise we cannot reopen a closed socket...

Is there a reason why this is still not fixed?

[kjbracey]

Looking at the code, I believe the _transport = nullptr should be inside the if (_close_transport) test in close.

That would then let you close the SSL session and make a new one on the same transport. That's not a normal use case though. You'd normally have one SSL session per TCP connection - this wrapper doesn't support closing and re-opening the underlying transport. That would be a bigger job.

Could be I'm misunderstanding something...

[boraozgen]

Technically, TLSSocket should be able to close and reopen the underlying transport if it opens the transport in the first place (as it does now).

Furthermore, why would be the pointer reset on close if it is set in the constructor? It should be destroyed together with the instance, whereas close should only close the transport.

I managed to get reconnection working without destroying the instance, I can open a PR regarding this.

[kjbracey]

The TLSSocketWrapper (I assume you meant that) doesn't open the underlying transport socket, it (optionally) connects it. It has to be already open. Unless I've missed something.

After closing a socket, it's potentially no longer valid, eg a socket that was created by a TCP listen. The close call of a Socket * can delete it. Hence the handle is cleared to acknowledge that it's no longer safe to reference.

I'm not clear whether you have an issue with the complete TLSSocket assembly, or you need an extension to the generic TLSSocketWrapper.

It should be possible to open and close a TLSSocket repeatedly. But it would be the user calling open on the TLSSocket, which then passes through to the TCPSocket base class. It's not the TLSSocketWrapper calling open on the transport.

It seems to me like TLSSocket::open could make a call to TLSSocketWrapper to re-set its transport pointer after calling TCPSocket::open. Perhaps TLSSocketWrapper needs an open(Socket *, ...) call for that purpose.

I think we were reluctant to re-do the "constructed-but-not-opened" state for the wrapper, so wanted those things set only on construction. But you end up in that state anyway after close...

Alternatively we could just remove that nulling and leave it up to users of TLSSocketWrapper not to mess up and keep using the wrapper after close, until they've re-opened. But then all those if (!transport) { return NSAPI_ERROR_NO_SOCKET; } won't kick in to catch them. Or maybe behaviour could be chosen with another flag like those "connect" and "close" flags options.

Anyway, it does need some thought.

[boraozgen]

I am using the TLSSocket class, so I might have somewhat ignored the TLSSocketWrapper's direct usage.

It seems to me like TLSSocket::open could make a call to TLSSocketWrapper to re-set its transport pointer after calling TCPSocket::open.

This is a good idea, I tried it out and seems to be working. Is there an argument against doing this?

It is not the only change btw, for instance mbedtls_ssl_session_reset must be called in connect. I got it working though.

[kjbracey]

Is there an argument against doing this?

I think it's probably the best overall solution, we just maybe need to think about the lifetime model of TLSSocketWrapper a bit. I'd suggest making a PR.

In general I've not been hugely keen on reuse of sockets because it's (a) non standard, (b) it rather mandates having a chunk of dedicated clean up code and getting that right, increasing code size.

It would be "cleaner" to destroy and reconstruct the socket, as portable BSD TCP socket code would.

The counterproblem with that is that we also aren't keep on dynamic allocation. new and delete isn't ideal for memory fragmentation. Achieving destruction and reconstruction of static objects is possible, but ugly.

C++17's std::variant has struck me as a potential answer to this, and various other use cases. Use that to be your socket storage, and you can rebuild as needed:

 // union of monostate dummy and TLSSocket (starting in monostate)
 std::variant<std::monostate, TLSSocket> socket_store;

 while (1) {
     // construct the socket in the storage (destroying the old one if second time round)
     TLSSocket &socket = socket_store.emplace<TLSSocket>(stack);
     socket.connect();
     // do stuff on socket
  }

Don't have that available in Mbed OS yet though. And arguably it's not that much clearer than placement new and delete, although partly due to novelty. And no doubt would end up bigger than placement new/delete - probably more bloat than any dedicated close-reopen support with direct calls.

[boraozgen]

Hi @kjbracey-arm,
Thank you for your explanation. I did not know that BSD sockets are not meant to be reused. It makes more sense now. However I think it should still be possible to allocate the socket in static memory. I tried to create a TLSSocket statically and (copy) assign it to a global variable like this:

socket = TLSSocket();

but I get the error:

use of deleted function 'TLSSocket& TLSSocket::operator=(const TLSSocket&)'

Do you know why this is not allowed? Shouldn't this be theoretically possible?

[kjbracey]

Hmm, that could in principle be permitted, by adding a move assignment operator. Copying is not permitted (you can't fork a socket into two sockets), but in this case the thing you're copying from is a temporary, so it could be implemented as a move.

However, the code required to support that is non-trivial, and in effect recreates the same problem - having to write code to manually clean up the state for re-use.

You can achieve what you want to achieve in a way that will work with any object by using placement delete and new manually:

 socket.~TLSSocket();
 new (&socket) TLSSocket();

That just uses the class's destructor and constructor, which must always exist.

(And that's what the std::variant I referred to above would be doing internally. The difference is that it safely tracks the storage state, whereas when doing it manually you've got to track whether the object is currently constructed or not, and not do something illegal like double destruction or double construction.)

[boraozgen]

Yes, that was exactly what I wanted. I didn't know about the pointer argument of the new operator either. Thanks for the tip!

I guess this will be the way to go for me until we have std::variant support.

[kjbracey]

Another general tip is check out SingletonPtr - that provides a wrapper to some static storage that can make lifetime management a bit nicer. The stored object gets created on first reference, rather than pre-main, and it ensures the destructor is not pulled into the ROM.

(We never call static object destructors, but some toolchains will insist on them being referenced. SingletonPtr avoids this).

We could maybe modify that to add a reconstruct method?

So

SingletonPtr<TLSSocket> socket;

socket.reconstruct();

I've just remembered I added a destroy method recently, so I was wondering if using destroy would do that already, forcing it to be reinitted on next reference, but the current destroy doesn't deal with the pointer properly - it was intended for embedding a SingletonPtr in an object that was being destroyed.