8343344: Windows attach logic failed to handle a failed open on a pipe

Reviewed-by: kevinw, cjplummer

Author: Alex Menkov

src/hotspot/os/windows/attachListener_windows.cpp

@@ -84,7 +84,8 @@ class PipeChannel : public AttachOperation::RequestReader, public AttachOperatio
                           0,              // default attributes
                           nullptr);       // no template file
     if (_hPipe == INVALID_HANDLE_VALUE) {
-      log_error(attach)("could not open (%d) pipe %s", GetLastError(), pipe);
+      log_error(attach)("could not open %s (%d) pipe %s",
+                        (write_only ? "write-only" : "read-write"), GetLastError(), pipe);
       return false;
     }
     return true;
@@ -106,7 +107,11 @@ class PipeChannel : public AttachOperation::RequestReader, public AttachOperatio
                              (DWORD)size,
                              &nread,
                              nullptr);   // not overlapped
-    return fSuccess ? (int)nread : -1;
+    if (!fSuccess) {
+      log_error(attach)("pipe read error (%d)", GetLastError());
+      return -1;
+    }
+    return (int)nread;
   }
 
   // ReplyWriter
@@ -118,7 +123,11 @@ class PipeChannel : public AttachOperation::RequestReader, public AttachOperatio
                               (DWORD)size,
                               &written,
                               nullptr);  // not overlapped
-    return fSuccess ? (int)written : -1;
+    if (!fSuccess) {
+        log_error(attach)("pipe write error (%d)", GetLastError());
+        return -1;
+    }
+    return (int)written;
   }
 
   void flush() override {
@@ -138,12 +147,12 @@ class Win32AttachOperation: public AttachOperation {
 
 public:
   // for v1 pipe must be write-only
-  void open_pipe(const char* pipe_name, bool write_only) {
-    _pipe.open(pipe_name, write_only);
+  bool open_pipe(const char* pipe_name, bool write_only) {
+    return _pipe.open(pipe_name, write_only);
   }
 
   bool read_request() {
-      return AttachOperation::read_request(&_pipe);
+    return AttachOperation::read_request(&_pipe);
   }
 
 public:
@@ -390,13 +399,17 @@ Win32AttachOperation* Win32AttachListener::dequeue() {
         for (int i = 0; i < AttachOperation::arg_count_max; i++) {
           op->append_arg(request->arg(i));
         }
-        op->open_pipe(request->pipe(), true/*write-only*/);
+        if (!op->open_pipe(request->pipe(), true/*write-only*/)) {
+          // error is already logged
+          delete op;
+          op = nullptr;
+        }
         break;
       case ATTACH_API_V2:
         op = new Win32AttachOperation();
-        op->open_pipe(request->pipe(), false/*write-only*/);
-        if (!op->read_request()) {
-          log_error(attach)("AttachListener::dequeue, reading request ERROR");
+        if (!op->open_pipe(request->pipe(), false/*write-only*/)
+            || !op->read_request()) {
+          // error is already logged
           delete op;
           op = nullptr;
         }

src/hotspot/share/services/attachListener.cpp

@@ -628,15 +628,16 @@ bool AttachOperation::read_request(RequestReader* reader) {
     buffer_size = (name_length_max + 1) + arg_count_max * (arg_length_max + 1);
     min_str_count = 1 /*name*/ + arg_count_max;
     break;
-  case ATTACH_API_V2: // <ver>0<size>0<cmd>0<arg>0<arg>0<arg>0
+  case ATTACH_API_V2: // <ver>0<size>0<cmd>0(<arg>0)* (any number of arguments)
     if (AttachListener::get_supported_version() < 2) {
-        log_error(attach)("Failed to read request: v2 is unsupported ot disabled");
+        log_error(attach)("Failed to read request: v2 is unsupported or disabled");
         return false;
     }
 
     // read size of the data
     buffer_size = reader->read_uint();
     if (buffer_size < 0) {
+      log_error(attach)("Failed to read request: negative request size (%d)", buffer_size);
       return false;
     }
     log_debug(attach)("v2 request, data size = %d", buffer_size);
@@ -646,7 +647,7 @@ bool AttachOperation::read_request(RequestReader* reader) {
       log_error(attach)("Failed to read request: too big");
       return false;
     }
-    // Must contain exact 'buffer_size' bytes.
+    // Must contain exactly 'buffer_size' bytes.
     min_read_size = buffer_size;
     break;
   default: