inspector: fix request path nullptr dereference

Fix a nullptr dereference when an invalid path is requested. Regression introduced in commit 69fc85d ("inspector: generate UUID for debug targets"), caught by Coverity. PR-URL: #9184 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · bnoordhuis · Oct 24, 2016 · Oct 19, 2016 · Oct 19, 2016 · Oct 24, 2016
commit 3d9b379cd03b966a22a9bcbbbc49caa8e7acc248
diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -681,17 +681,20 @@ bool AgentImpl::RespondToGet(InspectorSocket* socket, const std::string& path) {
 
   if (match_path_segment(command, "list") || command[0] == '\0') {
     SendTargentsListResponse(socket);
+    return true;
   } else if (match_path_segment(command, "protocol")) {
     SendProtocolJson(socket);
+    return true;
   } else if (match_path_segment(command, "version")) {
     SendVersionResponse(socket);
-  } else {
-    const char* pid = match_path_segment(command, "activate");
+    return true;
+  } else if (const char* pid = match_path_segment(command, "activate")) {
     if (pid != id_)
       return false;
     SendHttpResponse(socket, "Target activated");
+    return true;
   }
-  return true;
+  return false;
 }
 
 // static

diff --git a/test/inspector/inspector-helper.js b/test/inspector/inspector-helper.js
@@ -86,7 +86,17 @@ function checkHttpResponse(port, path, callback) {
     res.setEncoding('utf8');
     res
       .on('data', (data) => response += data.toString())
-      .on('end', () => callback(JSON.parse(response)));
+      .on('end', () => {
+        let err = null;
+        let json = undefined;
+        try {
+          json = JSON.parse(response);
+        } catch (e) {
+          err = e;
+          err.response = response;
+        }
+        callback(err, json);
+      });
   });
 }
 
@@ -284,8 +294,8 @@ TestSession.prototype.disconnect = function(childDone) {
 
 TestSession.prototype.testHttpResponse = function(path, check) {
   return this.enqueue((callback) =>
-      checkHttpResponse(this.harness_.port, path, (response) => {
-        check.call(this, response);
+      checkHttpResponse(this.harness_.port, path, (err, response) => {
+        check.call(this, err, response);
         callback();
       }));
 };
@@ -352,8 +362,8 @@ Harness.prototype.enqueue_ = function(task) {
 
 Harness.prototype.testHttpResponse = function(path, check) {
   return this.enqueue_((doneCallback) => {
-    checkHttpResponse(this.port, path, (response) => {
-      check.call(this, response);
+    checkHttpResponse(this.port, path, (err, response) => {
+      check.call(this, err, response);
       doneCallback();
     });
   });
@@ -393,7 +403,8 @@ Harness.prototype.wsHandshake = function(devtoolsUrl, tests, readyCallback) {
 
 Harness.prototype.runFrontendSession = function(tests) {
   return this.enqueue_((callback) => {
-    checkHttpResponse(this.port, '/json/list', (response) => {
+    checkHttpResponse(this.port, '/json/list', (err, response) => {
+      assert.ifError(err);
       this.wsHandshake(response[0]['webSocketDebuggerUrl'], tests, callback);
     });
   });

diff --git a/test/inspector/test-inspector.js b/test/inspector/test-inspector.js
@@ -5,14 +5,26 @@ const helper = require('./inspector-helper.js');
 
 let scopeId;
 
-function checkListResponse(response) {
+function checkListResponse(err, response) {
+  assert.ifError(err);
   assert.strictEqual(1, response.length);
   assert.ok(response[0]['devtoolsFrontendUrl']);
   assert.ok(
     response[0]['webSocketDebuggerUrl']
       .match(/ws:\/\/localhost:\d+\/[0-9A-Fa-f]{8}-/));
 }
 
+function checkVersion(err, response) {
+  assert.ifError(err);
+  assert.ok(response);
+}
+
+function checkBadPath(err, response) {
+  assert(err instanceof SyntaxError);
+  assert(/Unexpected token/.test(err.message));
+  assert(/WebSockets request was expected/.test(err.response));
+}
+
 function expectMainScriptSource(result) {
   const expected = helper.mainScriptSource();
   const source = result['scriptSource'];
@@ -153,7 +165,8 @@ function testInspectScope(session) {
 }
 
 function testNoUrlsWhenConnected(session) {
-  session.testHttpResponse('/json/list', (response) => {
+  session.testHttpResponse('/json/list', (err, response) => {
+    assert.ifError(err);
     assert.strictEqual(1, response.length);
     assert.ok(!response[0].hasOwnProperty('devtoolsFrontendUrl'));
     assert.ok(!response[0].hasOwnProperty('webSocketDebuggerUrl'));
@@ -171,7 +184,10 @@ function runTests(harness) {
   harness
     .testHttpResponse('/json', checkListResponse)
     .testHttpResponse('/json/list', checkListResponse)
-    .testHttpResponse('/json/version', assert.ok)
+    .testHttpResponse('/json/version', checkVersion)
+    .testHttpResponse('/json/activate', checkBadPath)
+    .testHttpResponse('/json/activate/boom', checkBadPath)
+    .testHttpResponse('/json/badpath', checkBadPath)
     .runFrontendSession([
       testNoUrlsWhenConnected,
       testBreakpointOnStart,