You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Android’s DefaultProxySelector.select() may throw IllegalArgumentException when the system proxy configuration contains an invalid port. This unchecked exception currently escapes the DNS resolver task and can terminate the process.
This change catches the exception at the ProxySelector.select() call and wraps it in an IOException. The original cause is preserved, and the exception message identifies the ProxySelector implementation.
DnsNameResolver already handles an IOException from proxy detection as an UNAVAILABLE name-resolution result. This allows the resolver to fail cleanly instead of letting the unchecked exception escape.
This intentionally avoids silently falling back to a direct connection. It follows the fail-cleanly approach used for invalid ProxySelector results in #12793 and avoids bypassing a configured proxy.
Testing
Added a regression test verifying that the exception is wrapped, the original cause is preserved, and the ProxySelector implementation is identified.
Ran .\gradlew.bat :grpc-core:test --tests "io.grpc.internal.ProxyDetectorImplTest"
All 11 tests passed on Windows with Temurin JDK 17.
Thanks for picking this up. Approach is right, a few things need changing.
The test doesn't cover the actual bug. ProxyDetectorImpl throwing IOException was already
handled before this PR the crash is the unchecked exception escaping DnsNameResolver$Resolve.run()
onto the executor thread. As written the test passes on a build that still crashes. There's a
pattern to copy in DnsNameResolverTest around L519 (thenThrow(new IOException())); the same with
an IllegalArgumentException, asserting the listener gets a result, fails on master today.
Related: select() isn't the only unguarded call on that path. A few lines down requestPasswordAuthentication() lands in java.net.Authenticator, which apps install via setDefault() same thread, same crash. So this also wants a catch around detectProxy() in Resolve.run() converting unchecked to IOException, and I'd widen the catch here to RuntimeException rather than just IAE.
Also worth knowing: the message you're building never reaches the user, because DnsNameResolver
overrides the description with "Unable to resolve host " + host. I've argued for falling back to
no-proxy on the issue instead probably worth waiting for that to settle before reworking, no point
doing it twice.
The reason will be displayed to describe this comment to others. Learn more.
Nit: assertSame for the cause.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13052.
Android’s
DefaultProxySelector.select()may throwIllegalArgumentExceptionwhen the system proxy configuration contains an invalid port. This unchecked exception currently escapes the DNS resolver task and can terminate the process.This change catches the exception at the
ProxySelector.select()call and wraps it in anIOException. The original cause is preserved, and the exception message identifies theProxySelectorimplementation.DnsNameResolveralready handles anIOExceptionfrom proxy detection as anUNAVAILABLEname-resolution result. This allows the resolver to fail cleanly instead of letting the unchecked exception escape.This intentionally avoids silently falling back to a direct connection. It follows the fail-cleanly approach used for invalid
ProxySelectorresults in #12793 and avoids bypassing a configured proxy.Testing
ProxySelectorimplementation is identified..\gradlew.bat :grpc-core:test --tests "io.grpc.internal.ProxyDetectorImplTest"