Gitiles
Code Review Sign In
review.blissroms.org / platform_packages_modules_DnsResolver / e2af226be9090f0105e42f12a93f07feb08a9d3a
commite2af226be9090f0105e42f12a93f07feb08a9d3a[log] [tgz]
authorBernie Innocenti <codewiz@google.com>Wed Oct 28 16:53:19 2020 +0900
committerBernie Innocenti <codewiz@google.com>Thu Oct 29 13:04:27 2020 +0900
tree506d915f3479140ca083fe080493c55d3329fdf3
parent0c7b1ca271d4a52d5de8e2b1433f04e96b3d1079 [diff]
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
2 files changed
tree: 506d915f3479140ca083fe080493c55d3329fdf3
  1. aidl_api/
  2. apex/
  3. binder/
  4. include/
  5. tests/
  6. .clang-format ⇨ ../../../build/soong/scripts/system-clang-format
  7. .editorconfig
  8. Android.bp
  9. Dns64Configuration.cpp
  10. Dns64Configuration.h
  11. DnsProxyListener.cpp
  12. DnsProxyListener.h
  13. DnsQueryLog.cpp
  14. DnsQueryLog.h
  15. DnsQueryLogTest.cpp
  16. DnsResolver.cpp
  17. DnsResolver.h
  18. DnsResolverService.cpp
  19. DnsResolverService.h
  20. DnsStats.cpp
  21. DnsStats.h
  22. DnsStatsTest.cpp
  23. DnsTlsDispatcher.cpp
  24. DnsTlsDispatcher.h
  25. DnsTlsQueryMap.cpp
  26. DnsTlsQueryMap.h
  27. DnsTlsServer.cpp
  28. DnsTlsServer.h
  29. DnsTlsSessionCache.cpp
  30. DnsTlsSessionCache.h
  31. DnsTlsSocket.cpp
  32. DnsTlsSocket.h
  33. DnsTlsSocketFactory.h
  34. DnsTlsTransport.cpp
  35. DnsTlsTransport.h
  36. Experiments.cpp
  37. Experiments.h
  38. ExperimentsTest.cpp
  39. getaddrinfo.cpp
  40. getaddrinfo.h
  41. gethnamaddr.cpp
  42. gethnamaddr.h
  43. hostent.h
  44. IDnsTlsSocket.h
  45. IDnsTlsSocketFactory.h
  46. IDnsTlsSocketObserver.h
  47. libnetd_resolv.map.txt
  48. LockedQueue.h
  49. NOTICE
  50. OWNERS
  51. params.h
  52. PREUPLOAD.cfg
  53. PrivateDnsConfiguration.cpp
  54. PrivateDnsConfiguration.h
  55. README-DoT.md
  56. README.md
  57. res_cache.cpp
  58. res_comp.cpp
  59. res_comp.h
  60. res_debug.cpp
  61. res_debug.h
  62. res_init.cpp
  63. res_init.h
  64. res_mkquery.cpp
  65. res_query.cpp
  66. res_send.cpp
  67. res_send.h
  68. res_stats.cpp
  69. resolv_cache.h
  70. resolv_cache_unit_test.cpp
  71. resolv_callback_unit_test.cpp
  72. resolv_private.h
  73. resolv_test_config_template.xml
  74. resolv_tls_unit_test.cpp
  75. resolv_unit_test.cpp
  76. ResolverController.cpp
  77. ResolverController.h
  78. ResolverEventReporter.cpp
  79. ResolverEventReporter.h
  80. ResolverStats.h
  81. sethostent.cpp
  82. stats.h
  83. stats.proto
  84. TEST_MAPPING
  85. util.cpp
  86. util.h
README.md

Logging

This code uses LOG(X) for logging. Log levels are VERBOSE,DEBUG,INFO,WARNING and ERROR. The default setting is WARNING and logs relate to WARNING and ERROR will be shown. If you want to enable the DEBUG level logs, using following command. adb shell service call dnsresolver 10 i32 1 VERBOSE 0 DEBUG 1 INFO 2 WARNING 3 ERROR 4 Verbose resolver logs could contain PII -- do NOT enable in production builds.