setServer(domain, ...) dangerous/dangling string pointer?

[binary1230]

I originally reported this in #751 but, I'm not sure it's the same issue and, it's potentially serious enough to move into its own issue. it might also be related to #794 and other heap-corruption crashes, not sure.

In here:

PubSubClient& PubSubClient::setServer(const char * domain, uint16_t port) {
    this->domain = domain;
    this->port = port;
    return *this;
}

the problem is with 'domain':

this->domain = domain;

it's a member variable inside class PubSubClient:

const char* domain;

The problem here is, it's just directly storing the char* pointer that was passed in. if the caller's string goes out of scope, then PubSubClient is holding onto a dangling pointer.

I noticed this issue in my code when the string I was using was re-allocated and 'domain' pointed to the string my client ID was using. (I happened to catch this because I was experiencing repeated connection failures and was using TinyGSM with verbose logging enabled. I noticed it was opening a TCP connection to the string I was using as a client ID, which in my case was a mac address. it amazingly wasn't crashing)

The solution is, PubSubClient needs to not store the string pointer directly, but have its own string buffer (or use a string class) and copy the string from the pointer passed in.

The simplest solution is probably to not store 'domain' as char*, but as a 'string' or 'String', which will automatically copy and do the right thing here.

If I'm not too tired right now and reading this right, this is a pretty serious bug.

[hmueller01]

@binary1230 Can you please take a look into hmueller01/pubsubclient3 if this solves your issue?

[binary1230]

Hey I saw your PR and a bunch of other stuff there, I was definitely planning on giving that a spin and switching to another library probably yours. Just haven't had the time to check on it yet.

[binary1230]

At a quick glance it sure looks like it'll solve the issue. Thanks for doing that, I'll give it a shot next time I'm in my code base.

I worked around the issue I was seeing by just allocating my own static buffer for that variable so that I could safely use the original library, which is obviously a real bad thing to have to do :)

[hmueller01]

I never used setServer() as I use a BearSSL connection (WiFiClientSecure) ad do the connect there. So I never saw this bug ...

While doing the refactoring I found a lot of other potential problems. Should be much better now. And if not and someone finds something I'll fix it.