ZOOKEEPER-4020: Fix memory leak from ssl cert in c client

[Gowrima]

Fixing memory leak by freeing memory allocated by strdup in cert_t.

[kezhuw]

This does not look right. I think it is zookeeper_close's job to free cert->ca.

[Gowrima]

@kezhuw Please review.

[kezhuw]

zhandle_t *zookeeper_init_ssl(const char *host, const char *cert, watcher_fn watcher,
        int recv_timeout, const clientid_t *clientid, void *context, int flags)
{
    zcert_t zcert;
    zcert.certstr = strdup(cert);
    zcert.ca = strtok(strdup(cert), ",");
    zcert.cert = strtok(NULL, ",");
    zcert.key = strtok(NULL, ",");
    zcert.passwd = strtok(NULL, ",");       
    return zookeeper_init_internal(host, watcher, recv_timeout, clientid, context, flags, NULL, &zcert, NULL);
}

After checking above code, I suggest we fix like following.

-    zcert.ca = strtok(strdup(cert), ",");
+    zcert.ca = strtok(zcert.certstr, ",");

Given the "Possible implementation" in https://en.cppreference.com/w/cpp/string/byte/strtok and also after evaluting strtok. I think there is no guarantee that zcert.ca will always point to allocated memory after strtok.

cc @symat @ztzg

[Gowrima]

@kezhuw
Isn't zcert.certstr supposed to hold the entire certificate and zcert.ca, zcert.key, zcert.password hold the appropriate tokens? With the suggested change, zcert.certstr will no longer hold the whole certificate, but only the first token (both zcert.certstr and zcert.ca will point to the first token - duplicates).

zcert.ca = strtok(strdup(cert), ","); -- this will point to the first token (i.e., 'ca') in the allocated memory.
zcert.certstr = strdup(cert) - this will hold the entire certificate as a string.

[kezhuw]

Isn't zcert.certstr supposed to hold the entire certificate and zcert.ca, zcert.key, zcert.password hold the appropriate tokens?

zcert.certstr = strdup(cert) - this will hold the entire certificate as a string.

zookeeper/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc

Lines 144 to 158 in 9d52264

	zhandle_t* connectReadOnlySSL(const char *address, const char *certs, watchctx_t *watch) {
	zhandle_t* zh = zookeeper_init_ssl(address, certs, watcher, 10000, NULL, watch, ZOO_READONLY);
	watch->zh = zh;
	CPPUNIT_ASSERT(zh != 0);
	sleep(1);
	return zh;
	}
	void testReadOnlyWithSSL() {
	startReadOnly();
	watchctx_t watch;
	zhandle_t* zh = connectReadOnlySSL("localhost:22281",
	"/tmp/certs/server.crt,/tmp/certs/client.crt,/tmp/certs/clientkey.pem,password",
	&watch);

certstr is only used in free(zh->fd->cert->certstr). I think it serves as a way to pass multiple ssl related parameters.
After zookeeper_init_ssl, it is done and should never be used until free. If zcert_t is a strong encapsulated struct, than certstr should not be exposed.

zcert_t zcert_new(const char *certstr) {
    zcert_t zcert;
    zcert.certstr = strdup(cert);
    zcert.ca = strtok(zcert.certstr, ",");
    zcert.cert = strtok(NULL, ",");
    zcert.key = strtok(NULL, ",");
    zcert.passwd = strtok(NULL, ",");  
    return zcert;
}

void zcert_destroy(zert_t *zcert) {
    free(zcert->certstr);
}

const char* zcert_get_ca(zcert_t *zcert) {
    return zcert->ca;
}

// api to retrieve `cert`, `key` and `passwd` but not `certstr`.

[Gowrima]

void testReadOnlyWithSSL() {
startReadOnly();

    watchctx_t watch;
    zhandle_t* zh = connectReadOnlySSL("localhost:22281",
                                       "/tmp/certs/server.crt,/tmp/certs/client.crt,/tmp/certs/clientkey.pem,password",
                                       &watch);

certstr = "/tmp/certs/server.crt,/tmp/certs/client.crt,/tmp/certs/clientkey.pem,password"

server.crt is the server's certificate stored in zcert.ca, client.crt is the client's certificate stored in zcert.cert, followed by zcert.key and zcert.password storing the client's private key and password to protect it.

With the suggested change, both zcert.certstr and zcert.ca will point to 'server.crt', as follows,

certstr = /tmp/certs/server.crt
ca = /tmp/certs/server.crt
cert = /tmp/certs/client.crt
key = /tmp/certs/clientkey.pem
password = password

We should either completely remove certstr from struct zcert_t or keep it to store the SSL parameters. I strongly recommend keeping the existing implementation to store the original SSL parameters.

[kezhuw]

With the suggested change, both zcert.certstr and zcert.ca will point to 'server.crt', as follows,

Does it matter if we never use certstr as const char* ?

The problem of current aproach is that it will corrupt program only after zookeeper_close with cert str ",,,/tmp/certs/server.crt,/tmp/certs/client.crt,/tmp/certs/clientkey.pem,password"(be aware of the leading ",") as ca will point to address not from malloc. My best wish is crash.

I think there are several ways to fix this:

1. zcert.ca = strtok(zcert.certstr, ",");
2. Enforce strong checking against input certstr and use only ca.
3. Enforce strong checking against input certstr and use both certstr and ca.

I prefer to the first as it demands no api semantic changes.

I strongly recommend keeping the existing implementation to store the original SSL parameters.

If you are going to this approach, please ensure ca pointing to address from malloc.

[kezhuw]

Given the "Possible implementation" in https://en.cppreference.com/w/cpp/string/byte/strtok and also after evaluting strtok. I think there is no guarantee that zcert.ca will always point to allocated memory after strtok.

Quoted from #2209 (comment)

[Gowrima]

This is the original implementation,

zcert_t zcert;
zcert.certstr = strdup(cert);
zcert.ca = strtok(strdup(cert), ",");

zcert.ca is pointing to malloc'd memory created by strdup() (https://man7.org/linux/man-pages/man3/strdup.3.html). strtok() returns a pointer to the first token in the heap allocated string (it points to the address returned by strdup() allocated in heap memory).

Since zcert.ca is allocated by malloc, it has to be explicitly freed, it leads to memory leak otherwise.

[Gowrima]

@kezhuw Awaiting your approval to merge. Please let me know if the changes look good.

[kezhuw]

The problem of current aproach is that it will corrupt program only after zookeeper_close with cert str ",,,/tmp/certs/server.crt,/tmp/certs/client.crt,/tmp/certs/clientkey.pem,password"(be aware of the leading ",") as ca will point to address not from malloc. My best wish is crash. -- #2209 (comment)

Could you test it with cert str ",,,/tmp/certs/server.crt,/tmp/certs/client.crt,/tmp/certs/clientkey.pem,password" ? I tested with an online repl, it crashed. Though I think crash is ok sometimes, but I don't think this is the case:

1. The crash is far from the cause.
2. There is no guarantee that free will crash in this case.

[kezhuw]

There is no guarantee that free will crash in this case. #2209 (comment)

Here is the reference.

The behavior is undefined if the value of ptr does not equal a value returned earlier by malloc(), calloc(), realloc(), or aligned_alloc()(since C11). https://en.cppreference.com/w/c/memory/free