8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library

Backport-of: fc67c2b4f17216d4adcc0825d0f378ae4f150025

Author: cost0much

src/hotspot/share/prims/jvmtiAgentList.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -124,11 +124,11 @@ void JvmtiAgentList::add(JvmtiAgent* agent) {
   } while (Atomic::cmpxchg(&_list, next, agent) != next);
 }
 
-void JvmtiAgentList::add(const char* name, char* options, bool absolute_path) {
+void JvmtiAgentList::add(const char* name, const char* options, bool absolute_path) {
   add(new JvmtiAgent(name, options, absolute_path));
 }
 
-void JvmtiAgentList::add_xrun(const char* name, char* options, bool absolute_path) {
+void JvmtiAgentList::add_xrun(const char* name, const char* options, bool absolute_path) {
   JvmtiAgent* agent = new JvmtiAgent(name, options, absolute_path);
   agent->set_xrun();
   add(agent);
@@ -198,18 +198,14 @@ void JvmtiAgentList::load_xrun_agents() {
 }
 
 // Invokes Agent_OnAttach for agents loaded dynamically during runtime.
-jint JvmtiAgentList::load_agent(const char* agent_name, const char* absParam,
-                           const char* options, outputStream* st) {
-  // The abs parameter should be "true" or "false"
-  const bool is_absolute_path = (absParam != nullptr) && (strcmp(absParam, "true") == 0);
+void JvmtiAgentList::load_agent(const char* agent_name, bool is_absolute_path,
+                                const char* options, outputStream* st) {
   JvmtiAgent* const agent = new JvmtiAgent(agent_name, options, is_absolute_path, /* dynamic agent */ true);
   if (agent->load(st)) {
     add(agent);
   } else {
     delete agent;
   }
-  // Agent_OnAttach executed so completion status is JNI_OK
-  return JNI_OK;
 }
 
 // Send any Agent_OnUnload notifications

src/hotspot/share/prims/jvmtiAgentList.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -64,14 +64,15 @@ class JvmtiAgentList : AllStatic {
   static void initialize();
   static void convert_xrun_agents();
 
- public:
   static void add(JvmtiAgent* agent) NOT_JVMTI_RETURN;
-  static void add(const char* name, char* options, bool absolute_path) NOT_JVMTI_RETURN;
-  static void add_xrun(const char* name, char* options, bool absolute_path) NOT_JVMTI_RETURN;
+
+ public:
+  static void add(const char* name, const char* options, bool absolute_path) NOT_JVMTI_RETURN;
+  static void add_xrun(const char* name, const char* options, bool absolute_path) NOT_JVMTI_RETURN;
 
   static void load_agents() NOT_JVMTI_RETURN;
-  static jint load_agent(const char* agent, const char* absParam,
-                         const char* options, outputStream* st) NOT_JVMTI_RETURN_(0);
+  static void load_agent(const char* agent, bool is_absolute_path,
+                         const char* options, outputStream* st) NOT_JVMTI_RETURN;
   static void load_xrun_agents() NOT_JVMTI_RETURN;
   static void unload_agents() NOT_JVMTI_RETURN;
 

src/hotspot/share/services/attachListener.cpp

@@ -135,7 +135,12 @@ static jint load_agent(AttachOperation* op, outputStream* out) {
     }
   }
 
-  return JvmtiAgentList::load_agent(agent, absParam, options, out);
+  // The abs parameter should be "true" or "false".
+  const bool is_absolute_path = (absParam != nullptr) && (strcmp(absParam, "true") == 0);
+  JvmtiAgentList::load_agent(agent, is_absolute_path, options, out);
+
+  // Agent_OnAttach result or error message is written to 'out'.
+  return JNI_OK;
 }
 
 // Implementation of "properties" command.

src/hotspot/share/services/diagnosticCommand.cpp

@@ -304,7 +304,7 @@ void JVMTIAgentLoadDCmd::execute(DCmdSource source, TRAPS) {
 
   if (is_java_agent) {
     if (_option.value() == nullptr) {
-      JvmtiAgentList::load_agent("instrument", "false", _libpath.value(), output());
+      JvmtiAgentList::load_agent("instrument", false, _libpath.value(), output());
     } else {
       size_t opt_len = strlen(_libpath.value()) + strlen(_option.value()) + 2;
       if (opt_len > 4096) {
@@ -321,12 +321,12 @@ void JVMTIAgentLoadDCmd::execute(DCmdSource source, TRAPS) {
       }
 
       jio_snprintf(opt, opt_len, "%s=%s", _libpath.value(), _option.value());
-      JvmtiAgentList::load_agent("instrument", "false", opt, output());
+      JvmtiAgentList::load_agent("instrument", false, opt, output());
 
       os::free(opt);
     }
   } else {
-    JvmtiAgentList::load_agent(_libpath.value(), "true", _option.value(), output());
+    JvmtiAgentList::load_agent(_libpath.value(), true, _option.value(), output());
   }
 }
 

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2005, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -96,22 +96,30 @@ private void loadAgentLibrary(String agentLibrary, boolean isAbsolute, String op
         }
 
         String msgPrefix = "return code: ";
-        InputStream in = execute("load",
-                                 agentLibrary,
-                                 isAbsolute ? "true" : "false",
-                                 options);
-        try (BufferedReader reader = new BufferedReader(new InputStreamReader(in))) {
-            String result = reader.readLine();
-            if (result == null) {
+        String errorMsg = "Failed to load agent library";
+        try {
+            InputStream in = execute("load",
+                                     agentLibrary,
+                                     isAbsolute ? "true" : "false",
+                                     options);
+            String result = readErrorMessage(in);
+            if (result.isEmpty()) {
                 throw new AgentLoadException("Target VM did not respond");
             } else if (result.startsWith(msgPrefix)) {
                 int retCode = Integer.parseInt(result.substring(msgPrefix.length()));
                 if (retCode != 0) {
                     throw new AgentInitializationException("Agent_OnAttach failed", retCode);
                 }
             } else {
-                throw new AgentLoadException(result);
+                if (!result.isEmpty()) {
+                    errorMsg += ": " + result;
+                }
+                throw new AgentLoadException(errorMsg);
             }
+        } catch (AttachOperationFailedException ex) {
+            // execute() throws AttachOperationFailedException if attach agent reported error.
+            // Convert it to AgentLoadException.
+            throw new AgentLoadException(errorMsg + ": " + ex.getMessage());
         }
     }
 
@@ -364,6 +372,9 @@ String readErrorMessage(InputStream in) throws IOException {
         StringBuilder message = new StringBuilder();
         BufferedReader br = new BufferedReader(new InputStreamReader(in));
         while ((s = br.readLine()) != null) {
+            if (message.length() > 0) {
+                message.append(' ');
+            }
             message.append(s);
         }
         return message.toString();
@@ -399,20 +410,10 @@ void processCompletionStatus(IOException ioe, String cmd, InputStream sis) throw
                 throw new IOException("Protocol mismatch with target VM");
             }
 
-            // Special-case the "load" command so that the right exception is
-            // thrown.
-            if (cmd.equals("load")) {
-                String msg = "Failed to load agent library";
-                if (!message.isEmpty()) {
-                    msg += ": " + message;
-                }
-                throw new AgentLoadException(msg);
-            } else {
-                if (message.isEmpty()) {
-                    message = "Command failed in target VM";
-                }
-                throw new AttachOperationFailedException(message);
+            if (message.isEmpty()) {
+                message = "Command failed in target VM";
             }
+            throw new AttachOperationFailedException(message);
         }
     }
 

test/jdk/com/sun/tools/attach/FailedLoadAgentTest.java

@@ -0,0 +1,90 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8325530
+ * @summary Test that failed VirtualMachine.loadAgentPath/loadAgentLibrary reports detailed reason
+ * @requires vm.jvmti
+ * @modules jdk.attach
+ * @library /test/lib
+ * @run driver FailedLoadAgentTest
+ */
+
+import java.nio.file.Path;
+import com.sun.tools.attach.AgentLoadException;
+import com.sun.tools.attach.VirtualMachine;
+
+import jdk.test.lib.Platform;
+import jdk.test.lib.Utils;
+import jdk.test.lib.apps.LingeredApp;
+
+public class FailedLoadAgentTest {
+    private static final String jvmtiAgentLib = "FailedLoadAgentTestNotExists";
+    private static final String jvmtiAgentPath = getLibPath(jvmtiAgentLib);
+
+    private static String getLibPath(String libName) {
+        String fullName = Platform.buildSharedLibraryName(libName);
+        return Path.of(Utils.TEST_NATIVE_PATH, fullName).toAbsolutePath().toString();
+    }
+
+    private interface TestAction {
+        void test() throws Exception;
+    }
+
+    private static void test(TestAction action) throws Exception {
+        try {
+            action.test();
+            throw new RuntimeException("AgentLoadException not thrown");
+        } catch (AgentLoadException ex) {
+            System.out.println("AgentLoadException thrown as expected:");
+            ex.printStackTrace(System.out);
+            String msg = ex.getMessage();
+            // Attach agent prints general "<agent> was not loaded." message on error.
+            // But additionally we expect detailed message with the reason.
+            String parts[] = msg.split("was not loaded.");
+            if (parts.length < 2 || parts[1].isEmpty()) {
+                throw new RuntimeException("AgentLoadException message is vague");
+            }
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        LingeredApp theApp = null;
+        try {
+            theApp = new LingeredApp();
+            LingeredApp.startApp(theApp, "-XX:+EnableDynamicAgentLoading");
+
+            VirtualMachine vm = VirtualMachine.attach(Long.toString(theApp.getPid()));
+
+            // absolute path
+            test(() -> vm.loadAgentPath(jvmtiAgentPath));
+            // relative path
+            test(() -> vm.loadAgentLibrary(jvmtiAgentLib));
+
+        } finally {
+            LingeredApp.stopApp(theApp);
+        }
+    }
+
+}