tree 506d915f3479140ca083fe080493c55d3329fdf3
parent 0c7b1ca271d4a52d5de8e2b1433f04e96b3d1079
author Bernie Innocenti <codewiz@google.com> 1603871599 +0900
committer Bernie Innocenti <codewiz@google.com> 1603944267 +0900

Fix refcounting on thread creation failure path for android_res_nsend()

SocketClient instances are manually reference-counted. DnsProxyListener
passes them down to handler threads, and expects them to decrement the
reference coint when done.

On thread creation failure, tryThreadOrError() deletes the handler and
decrements the reference:

void tryThreadOrError(SocketClient* cli, T* handler) {
    const int rval = netdutils::threadLaunch(handler);
    if (rval == 0) {
        // SocketClient decRef() happens in the handler's run() method.
        return;
    }
    ...
    delete handler;  // Calls decRef() in ~ResNSendHandler!!!
    cli->decRef();
}

However, the assumption that decRef() is done in the handler's run()
method was not true in all cases: ResNSendHandler, added in Android Q,
actually decrements the client's reference count in its destructor.

Since tryThreadOrError() decrements the counter twice on the error path,
the SocketClient self-deletes too early, while it is still referenced by
the SocketListener.

Triggering this condition requires an unresponsive network and multiple
apps sending queries, until netd runs out of threads (each app is
limited to 256 concurrent queries by queryLimiter). At least one app
should be using android_res_nsend().

The fix consists in moving ownership of SocketClient to a base class of
all handlers, with strong encapsulation of the manual reference
counting, RAII-style. This simplifies DnsProxyListener while making
further refcounting bugs unlikely to occur in the future.

DnsProxyListener remains dangerously open to memory leaks in error paths
due to manual memory management. We should take care of this in a
followup change.

Bug: 169105756
Change-Id: I9241a094653651e1bdda79eb753e7a53e5e51d8f