Add an Error Prone check to discourage APIs that convert a hostname t…

…o a single IP address PiperOrigin-RevId: 552955605
google · Aug 1, 2023 · a2d6ad7 · a2d6ad7
1 parent 2ef7ec2
commit a2d6ad7
Show file tree

Hide file tree

Showing 4 changed files with 320 additions and 0 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java b/core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java
@@ -0,0 +1,110 @@
+/*
+ * Copyright 2023 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import static com.google.common.collect.Iterables.getOnlyElement;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;
+import static com.google.errorprone.fixes.SuggestedFixes.renameMethodInvocation;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.method.MethodMatchers.constructor;
+import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
+import static com.google.errorprone.util.ASTHelpers.constValue;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.Matchers;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.NewClassTree;
+import java.util.Objects;
+import java.util.function.Supplier;
+
+/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
+@BugPattern(
+    summary =
+        "Prefer InetAddress.getAllName to APIs that convert a hostname to a single IP address",
+    severity = WARNING)
+public final class AddressSelection extends BugChecker
+    implements NewClassTreeMatcher, MethodInvocationTreeMatcher {
+
+  private static final Matcher<ExpressionTree> CONSTRUCTORS =
+      Matchers.anyOf(
+          constructor().forClass("java.net.Socket").withParameters("java.lang.String", "int"),
+          constructor()
+              .forClass("java.net.InetSocketAddress")
+              .withParameters("java.lang.String", "int"));
+  private static final Matcher<ExpressionTree> METHODS =
+      staticMethod()
+          .onClass("java.net.InetAddress")
+          .named("getByName")
+          .withParameters("java.lang.String");
+
+  @Override
+  public Description matchNewClass(NewClassTree tree, VisitorState state) {
+    if (!CONSTRUCTORS.matches(tree, state)) {
+      return NO_MATCH;
+    }
+    ExpressionTree argument = tree.getArguments().get(0);
+    return handleMatch(
+        argument,
+        argument,
+        () -> {
+          SuggestedFix.Builder fix = SuggestedFix.builder();
+          fix.replace(
+              argument, qualifyType(state, fix, "java.net.InetAddress") + ".getLoopbackAddress()");
+          return fix.build();
+        });
+  }
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!METHODS.matches(tree, state)) {
+      return NO_MATCH;
+    }
+    ExpressionTree argument = getOnlyElement(tree.getArguments());
+    return handleMatch(
+        argument,
+        tree,
+        () ->
+            SuggestedFix.builder()
+                .merge(renameMethodInvocation(tree, "getLoopbackAddress", state))
+                .delete(argument)
+                .build());
+  }
+
+  private static final ImmutableSet<String> LOOPBACK = ImmutableSet.of("127.0.0.1", "::1");
+
+  private Description handleMatch(
+      ExpressionTree argument, ExpressionTree replacement, Supplier<SuggestedFix> fix) {
+    String value = constValue(argument, String.class);
+    if (Objects.equals(value, "localhost")) {
+      return NO_MATCH;
+    }
+    Description.Builder description = buildDescription(replacement);
+    if (LOOPBACK.contains(value)) {
+      description.addFix(fix.get());
+    }
+    return description.build();
+  }
+}
diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
@@ -22,6 +22,7 @@
 import com.google.common.collect.Streams;
 import com.google.errorprone.BugCheckerInfo;
 import com.google.errorprone.bugpatterns.ASTHelpersSuggestions;
+import com.google.errorprone.bugpatterns.AddressSelection;
 import com.google.errorprone.bugpatterns.AlreadyChecked;
 import com.google.errorprone.bugpatterns.AlwaysThrows;
 import com.google.errorprone.bugpatterns.AmbiguousMethodReference;
@@ -817,6 +818,7 @@ public static ScannerSupplier warningChecks() {
       getSuppliers(
           // keep-sorted start
           ASTHelpersSuggestions.class,
+          AddressSelection.class,
           AlmostJavadoc.class,
           AlreadyChecked.class,
           AmbiguousMethodReference.class,

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java
@@ -0,0 +1,126 @@
+/*
+ * Copyright 2023 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class AddressSelectionTest {
+
+  private final CompilationTestHelper compilationHelper =
+      CompilationTestHelper.newInstance(AddressSelection.class, getClass());
+
+  private final BugCheckerRefactoringTestHelper refactoringTestHelper =
+      BugCheckerRefactoringTestHelper.newInstance(AddressSelection.class, getClass());
+
+  @Test
+  public void positive() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import java.net.InetAddress;",
+            "import java.net.InetSocketAddress;",
+            "import java.net.Socket;",
+            "class Test {",
+            "  void f() throws Exception{",
+            "    // BUG: Diagnostic contains:",
+            "    InetAddress.getByName(\"example.com\");",
+            "    // BUG: Diagnostic contains:",
+            "    new Socket(\"example.com\", 80);",
+            "    // BUG: Diagnostic contains:",
+            "    new InetSocketAddress(\"example.com\", 80);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negative() throws Exception {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import java.net.InetAddress;",
+            "import java.net.InetSocketAddress;",
+            "import java.net.Socket;",
+            "class Test {",
+            "  void f() throws Exception{",
+            "    new Socket(InetAddress.getLoopbackAddress(), 80);",
+            "    InetAddress.getAllByName(\"example.com\");",
+            "    new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negativeLocalhost() throws Exception {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import java.net.InetAddress;",
+            "import java.net.InetSocketAddress;",
+            "import java.net.Socket;",
+            "class Test {",
+            "  void f() throws Exception{",
+            "    new Socket(\"localhost\", 80);",
+            "    InetAddress.getByName(\"localhost\");",
+            "    new InetSocketAddress(\"localhost\", 80);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void refactor() throws Exception {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java",
+            "import java.net.InetAddress;",
+            "import java.net.InetSocketAddress;",
+            "import java.net.Socket;",
+            "class Test {",
+            "  void f() throws Exception{",
+            "    new Socket(\"127.0.0.1\", 80);",
+            "    InetAddress.getByName(\"127.0.0.1\");",
+            "    new InetSocketAddress(\"127.0.0.1\", 80);",
+            "    new Socket(\"::1\", 80);",
+            "    InetAddress.getByName(\"::1\");",
+            "    new InetSocketAddress(\"::1\", 80);",
+            "  }",
+            "}")
+        .addOutputLines(
+            "Test.java",
+            "import java.net.InetAddress;",
+            "import java.net.InetSocketAddress;",
+            "import java.net.Socket;",
+            "class Test {",
+            "  void f() throws Exception{",
+            "    new Socket(InetAddress.getLoopbackAddress(), 80);",
+            "    InetAddress.getLoopbackAddress();",
+            "    new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);",
+            "    new Socket(InetAddress.getLoopbackAddress(), 80);",
+            "    InetAddress.getLoopbackAddress();",
+            "    new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);",
+            "  }",
+            "}")
+        .doTest();
+  }
+}
diff --git a/docs/bugpattern/AddressSelection.md b/docs/bugpattern/AddressSelection.md
@@ -0,0 +1,82 @@
+Avoid APIs that convert a hostname to a single IP address:
+
+*   [`java.net.Socket(String,int)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/Socket.html#%3Cinit%3E\(java.lang.String,int,boolean\))
+*   [`java.net.InetSocketAddress(String,int)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetSocketAddress.html#%3Cinit%3E\(java.lang.String,int\))
+*   [`java.net.InetAddress.html#getByName(String)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#getByName\(java.lang.String\))
+
+Depending on the value of the
+[`-Djava.net.preferIPv6Addresses=true`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/doc-files/net-properties.html)
+system property, those APIs will return an IPv4 or IPv6 address. If a client
+only has IPv4 connectivity, it will fail to connect with
+`-Djava.net.preferIPv6Addresses=true`. If a client only has IPv6 connectivity,
+it will fail to connect with `-Djava.net.preferIPv6Addresses=false`.
+
+The preferred alternative is for clients to consider all addresses returned by
+[`java.net.InetAddress.html#getAllByName(String)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#getAllByName\(java.lang.String\)),
+and try to connect to each one until a successful connection is made.
+
+TIP: To resolve a loopback address, prefer `InetAddress.getLoopbackAddress()`
+over hard-coding an IPv4 or IPv6 loopback address with
+`InetAddress.getByName("127.0.0.1")` or `InetAddress.getByName("::1")`.
+
+This is, prefer this:
+
+```java
+  Socket doConnect(String hostname, int port) throws IOException {
+    IOException exception = null;
+    for (InetAddress address : InetAddress.getAllByName(hostname)) {
+      try {
+        return new Socket(address, port);
+      } catch (IOException e) {
+        if (exception == null) {
+          exception = e;
+        } else {
+          exception.addSuppressed(e);
+        }
+      }
+    }
+    throw exception;
+  }
+```
+
+```java
+  Socket doConnect(String hostname, int port) throws IOException {
+    IOException exception = null;
+    for (InetAddress address : InetAddress.getAllByName(hostname)) {
+      try {
+        Socket s = new Socket();
+        s.connect(new InetSocketAddress(address, port));
+        return s;
+      } catch (IOException e) {
+        if (exception == null) {
+          exception = e;
+        } else {
+          exception.addSuppressed(e);
+        }
+      }
+    }
+    throw exception;
+  }
+```
+
+instead of this:
+
+```java
+  Socket doConnect(String hostname, int port) throws IOException {
+    return new Socket(hostname, port);
+  }
+```
+
+```java
+  void doConnect(String hostname, int port) throws IOException {
+    Socket s = new Socket();
+    s.connect(new InetSocketAddress(hostname, port));
+  }
+```
+
+```java
+  void doConnect(String hostname, int port) throws IOException {
+    Socket s = new Socket();
+    s.connect(new InetSocketAddress(InetAddress.getByName(hostname), port));
+  }
+```