Make UriParser to support [$HOSTNAME] notation.

[ovyalov]

Summary

UriParser:Parse expects that host name doesn't contain special URL symbols - ":", ";", "$",...
However, in case of adb protocol (adb://$device_serial_no:$port) it poses a problem - device serial number may contain a colon ("192.168.0.1:5555").
In order to handle this situation UriParser now supports optional [$HOSTNAME] notation - i.e. connect://localhost:5555 and connect://[localhost]:5555 should be parsed identically.
This change allows us to pass special URL symbols (like colon) as part of a hostname.

[tberghammer]

The implementation looks good, but I don't like the approach you try to handle the problem.

I think you do far too much work to use UriParser in a case where it isn't necessary and isn't make things easier. I think a better (and definitely simpler) solution would be to change only ConnectionFileDescriptorPosix.cpp to parse the adb addresses with a locally specified regexp or with looking for the last colon in the address and split the address to host and port based on that.

[ovyalov]

In D12025#224355, @tberghammer wrote:

The implementation looks good, but I don't like the approach you try to handle the problem.

I think you do far too much work to use UriParser in a case where it isn't necessary and isn't make things easier. I think a better (and definitely simpler) solution would be to change only ConnectionFileDescriptorPosix.cpp to parse the adb addresses with a locally specified regexp or with looking for the last colon in the address and split the address to host and port based on that.

The problem that connection url goes all way down PlatformAndroid::ConnectRemote-> PlatformAndroidRemoteGDBServer::ConnectRemote->PlatformRemoteGDBServer::ConnectRemote whiles it reaches ConnectionFileDescriptorPosix::Connect. Within each ConnectRemote we call UriParser::Parse - either to verify url correctness or fetch device_id from adb url. In this case we may need to have special handling for adb protocol inside of UriParser::Parse to pass it through - I'm not very happy to bring protocol-specific knowledge to this class.

[tberghammer]

I see your point. I am still not too happy with this approach but I don't have any better idea to solve it. I am fine with committing it in in this format and we can improve it later if we find a better approach.

[ovyalov]

Rewrote the CL to make UriParser to support optional [$HOSTNAME] notation for LLDB connect URL - i.e. connect://localhost:5555 and connect://[localhost]:5555 should be parsed identically.
This change allows us to pass special URL symbols (like colon) as part of a hostname.

[tberghammer]

source/Utility/UriParser.cpp

12–13

I think you wanted to include <string>, not <string.h>

88–97

I would prefer no to remove the '[' and the ']' characters from the hostname. At the moment you don't use the hostname for the adb protocol so it isn't matter but if we consider IPv6 then having '[' and ']' around the address sounds like a good idea to me (later stages can use this information).

[ovyalov]

source/Utility/UriParser.cpp

12–13

I need string.h for strlen - it's required even with <string> included .

88–97

Let me leave [] removal as-is since we treat hostname as device_id in case of adb and then use it for port forwarding setup - https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Platform/Android/PlatformAndroid.cpp#L199

[labath]

source/Utility/UriParser.cpp

12–13

The C++ way would be to #include <cstring>

</driveby>

[tberghammer]

source/Utility/UriParser.cpp

12–13

Sorry, I missed that you use strlen

88–97

I would prefer to remove them in PlatformAndroid when we convert it to devices id, but if you want to leave it this way I am fine with that also.

[ovyalov]

Replaced #include <string.h> with <cstring>

[ovyalov]

Files:

/lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
/lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
/lldb/trunk/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
/lldb/trunk/source/Utility/UriParser.cpp
/lldb/trunk/source/Utility/UriParser.h
/lldb/trunk/unittests/Utility/UriParserTest.cpp

Users:

ovyalov (Author)

http://reviews.llvm.org/rL245639

source/Host/posix/ConnectionFileDescriptorPosix.cpp

 #include <stdlib.h>
 #include <sys/types.h>
 
 #ifndef LLDB_DISABLE_POSIX
 #include <termios.h>
 #endif
 
 // C++ Includes
+#include <sstream>
+
 // Other libraries and framework includes
 #include "llvm/Support/ErrorHandling.h"
 #if defined(__APPLE__)
 #include "llvm/ADT/SmallVector.h"
 #endif
 // Project includes
 #include "lldb/Core/Communication.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/StreamString.h"
 #include "lldb/Core/Timer.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Interpreter/Args.h"
 
+#include "Utility/UriParser.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
 ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit)
     : Connection()
     , m_pipe()
     , m_mutex(Mutex::eMutexTypeRecursive)
     , m_shutting_down(false)
if (s && s[0])
         else if (strstr(s, "unix-accept://") == s)
         {
             // unix://SOCKNAME
             return NamedSocketAccept(s + strlen("unix-accept://"), error_ptr);
         }
         else if (strstr(s, "adb://") == s)
         {
             int port = -1;
-            sscanf(s, "adb://%*[^:]:%d", &port);
-            char host_and_port[sizeof("localhost:65535")];
-            snprintf(host_and_port, sizeof(host_and_port), "localhost:%d", port);
-            return ConnectTCP(host_and_port, error_ptr);
+            std::string scheme, host, path;
+            if (!UriParser::Parse(s, scheme, host, port, path))
+            {
+                if (error_ptr)
+                    error_ptr->SetErrorStringWithFormat("Failed to parse URL '%s'", s);
+                return eConnectionStatusError;
+            }
+            std::ostringstream host_and_port;
+            host_and_port << "localhost:" << port;
+            return ConnectTCP(host_and_port.str().c_str(), error_ptr);
         }
         else if (strstr(s, "connect://") == s)
         {
             return ConnectTCP(s + strlen("connect://"), error_ptr);
         }
         else if (strstr(s, "tcp-connect://") == s)
         {
             return ConnectTCP(s + strlen("tcp-connect://"), error_ptr);

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h

protected:
     LaunchGDBserverAndGetPort (lldb::pid_t &pid) override;
 
     bool
     KillSpawnedProcess (lldb::pid_t pid) override;
 
     void
     DeleteForwardPort (lldb::pid_t pid);
 
+    std::string
+    MakeServerUrl(const char* scheme,
+                  const char* hostname,
+                  uint16_t port) override;
+
 private:
     DISALLOW_COPY_AND_ASSIGN (PlatformAndroidRemoteGDBServer);
 
 };
 
 } // namespace platform_android
 } // namespace lldb_private
 
 #endif  // liblldb_PlatformAndroidRemoteGDBServer_h_

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

 #include "lldb/Core/Error.h"
 #include "lldb/Core/Log.h"
 
 // Project includes
 #include "AdbClient.h"
 #include "PlatformAndroidRemoteGDBServer.h"
 #include "Utility/UriParser.h"
 
+#include <sstream>
+
 using namespace lldb;
 using namespace lldb_private;
 using namespace platform_android;
 
 static const lldb::pid_t g_remote_platform_pid = 0; // Alias for the process id of lldb-platform
 
 static Error
 ForwardPortWithAdb (uint16_t port, std::string& device_id)
PlatformAndroidRemoteGDBServer::DeleteForwardPort (lldb::pid_t pid)
     const auto error = DeleteForwardPortWithAdb(port, m_device_id);
     if (error.Fail()) {
         if (log)
             log->Printf("Failed to delete port forwarding (pid=%" PRIu64 ", port=%d, device=%s): %s",
                          pid, port, m_device_id.c_str(), error.AsCString());
     }
     m_port_forwards.erase(it);
 }
+
+std::string
+PlatformAndroidRemoteGDBServer::MakeServerUrl(const char* scheme,
+                                              const char* hostname,
+                                              uint16_t port)
+{
+    std::ostringstream hostname_str;
+    if (!strcmp(scheme, "adb"))
+        hostname_str << "[" << hostname << "]";
+    else
+        hostname_str << hostname;
+
+    return PlatformRemoteGDBServer::MakeServerUrl(scheme,
+                                                  hostname_str.str().c_str(),
+                                                  port);
+}
+

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h

protected:
     // failure. Subclasses should override this method if they want to do extra actions before or
     // after launching the lldb-gdbserver.
     virtual uint16_t
     LaunchGDBserverAndGetPort (lldb::pid_t &pid);
 
     virtual bool
     KillSpawnedProcess (lldb::pid_t pid);
 
+    virtual std::string
+    MakeServerUrl(const char* scheme,
+                  const char* hostname,
+                  uint16_t port);
+
 private:
+    std::string
+    MakeGdbServerUrl(const std::string &platform_scheme,
+                     const std::string &platform_hostname,
+                     uint16_t port);
+
     DISALLOW_COPY_AND_ASSIGN (PlatformRemoteGDBServer);
 
 };
 
 } // namespace platform_gdb_server
 } // namespace lldb_private
 
 #endif  // liblldb_PlatformRemoteGDBServer_h_

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp

 #include "Plugins/Process/Utility/GDBRemoteSignals.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::platform_gdb_server;
 
 static bool g_initialized = false;
 
-static std::string MakeGdbServerUrl(
-        const std::string &platform_scheme,
-        const std::string &platform_hostname,
-        uint16_t port)
-{
-    const char *override_scheme = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_SCHEME");
-    const char *override_hostname = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_HOSTNAME");
-    const char *port_offset_c_str = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_PORT_OFFSET");
-    int port_offset = port_offset_c_str ? ::atoi(port_offset_c_str) : 0;
-    StreamString result;
-    result.Printf("%s://%s:%u",
-            override_scheme ? override_scheme : platform_scheme.c_str(),
-            override_hostname ? override_hostname : platform_hostname.c_str(),
-            port + port_offset);
-    return result.GetString();
-}
-
 void
 PlatformRemoteGDBServer::Initialize ()
 {
     Platform::Initialize ();
 
     if (g_initialized == false)
     {
         g_initialized = true;
bool done = array_sp->ForEach(
             return true;
         });
 
     if (done)
         m_remote_signals_sp = std::move(remote_signals_sp);
 
     return m_remote_signals_sp;
 }
+
+std::string
+PlatformRemoteGDBServer::MakeGdbServerUrl(const std::string &platform_scheme,
+                                          const std::string &platform_hostname,
+                                          uint16_t port)
+{
+    const char *override_scheme = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_SCHEME");
+    const char *override_hostname = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_HOSTNAME");
+    const char *port_offset_c_str = getenv("LLDB_PLATFORM_REMOTE_GDB_SERVER_PORT_OFFSET");
+    int port_offset = port_offset_c_str ? ::atoi(port_offset_c_str) : 0;
+
+    return MakeServerUrl(override_scheme ? override_scheme : platform_scheme.c_str(),
+                         override_hostname ? override_hostname : platform_hostname.c_str(),
+                         port + port_offset);
+}
+
+std::string
+PlatformRemoteGDBServer::MakeServerUrl(const char* scheme,
+                                       const char* hostname,
+                                       uint16_t port)
+{
+    StreamString result;
+    result.Printf("%s://%s:%u", scheme, hostname, port);
+    return result.GetString();
+}
+

source/Utility/UriParser.h

public:
     // Parses
     // RETURN VALUE
     //   if url is valid, function returns true and 
     //   scheme/hostname/port/path are set to the parsed values
     //   port it set to -1 if it is not included in the URL
     //
     //   if the url is invalid, function returns false and
     //   output parameters remain unchanged
-    static bool Parse(const char* uri,
+    static bool
+    Parse(const std::string& uri,
           std::string& scheme,
           std::string& hostname,
           int& port,
-        std::string& path
-        );
+          std::string& path);
 };
 
 #endif  // utility_UriParser_h_

source/Utility/UriParser.cpp

 //===-- UriParser.cpp -------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Utility/UriParser.h"
 
 // C Includes
-#include <stdlib.h>
-#include <stdio.h>
 
 // C++ Includes
+#include <cstring>
+
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Host/StringConvert.h"
 
 using namespace lldb_private;
 
 //----------------------------------------------------------------------
 // UriParser::Parse
 //----------------------------------------------------------------------
 bool
-UriParser::Parse(const char* uri,
+UriParser::Parse(const std::string& uri,
                  std::string& scheme,
                  std::string& hostname,
                  int& port,
-    std::string& path
-    )
+                 std::string& path)
 {
-    char scheme_buf[100] = {0};
-    char hostname_buf[256] = {0};
-    char port_buf[11] = {0}; // 10==strlen(2^32)
-    char path_buf[2049] = {'/', 0};
-  
-    bool ok = false;
-         if (4==sscanf(uri, "%99[^:/]://%255[^/:]:%10[^/]/%2047s", scheme_buf, hostname_buf, port_buf, path_buf+1)) { ok = true; }
-    else if (3==sscanf(uri, "%99[^:/]://%255[^/:]:%10[^/]", scheme_buf, hostname_buf, port_buf)) { ok = true; }
-    else if (3==sscanf(uri, "%99[^:/]://%255[^/]/%2047s", scheme_buf, hostname_buf, path_buf+1)) { ok = true; }
-    else if (2==sscanf(uri, "%99[^:/]://%255[^/]", scheme_buf, hostname_buf)) { ok = true; }
+    std::string tmp_scheme, tmp_hostname, tmp_port, tmp_path;
 
-    bool success = false;
-    int port_tmp = -1;
-    if (port_buf[0])
-    {
-        port_tmp = StringConvert::ToUInt32(port_buf, UINT32_MAX, 10, &success);
-        if (!success || port_tmp > 65535)
-        {
-            // there are invalid characters in port_buf
+    static const char* kSchemeSep = "://";
+    auto pos = uri.find(kSchemeSep);
+    if (pos == std::string::npos)
+        return false;
+
+    // Extract path.
+    tmp_scheme = uri.substr(0, pos);
+    auto host_pos = pos + strlen(kSchemeSep);
+    auto path_pos = uri.find_first_of("/", host_pos);
+    if (path_pos != std::string::npos)
+        tmp_path = uri.substr(path_pos);
+    else
+        tmp_path = "/";
+
+    auto host_port = uri.substr(
+        host_pos, ((path_pos != std::string::npos) ? path_pos  : uri.size()) - host_pos);
+
+    // Extract hostname
+    if (host_port[0] == '[')
+        // hostname is enclosed with square brackets.
+        pos = host_port.find(']');
+        if (pos == std::string::npos)
             return false;
-        }
+
+        tmp_hostname = host_port.substr(1, pos - 1);
+        host_port.erase(0, pos + 1);
+    else
+    {
+        pos = host_port.find(':');
+        tmp_hostname = host_port.substr(0, (pos != std::string::npos) ? pos : host_port.size());
+        host_port.erase(0, (pos != std::string::npos) ? pos : host_port.size());
     }
 
-    if (ok)
+    // Extract port
+    tmp_port = host_port;
+    if (!tmp_port.empty())
+    {
+        if (tmp_port[0] != ':')
+            return false;
+        tmp_port = tmp_port.substr(1);
+        bool success = false;
+        auto port_tmp = StringConvert::ToUInt32(tmp_port.c_str(), UINT32_MAX, 10, &success);
+        if (!success || port_tmp > 65535)
         {
-        scheme.assign(scheme_buf);
-        hostname.assign(hostname_buf);
+            // there are invalid characters in port_buf
+            return false;
+        }
         port = port_tmp;
-        path.assign(path_buf);
     }
-    return ok;
+    else
+        port = -1;
+
+    scheme = tmp_scheme;
+    hostname = tmp_hostname;
+    path = tmp_path;
+    return true;
 }
 

unittests/Utility/UriParserTest.cpp

 }
 
 TEST_F (UriParserTest, MinimalPortPath)
 {
     const UriTestCase testCase("x://y:1/", "x", "y", 1, "/");
     VALIDATE
 }
 
+TEST_F (UriParserTest, LongPath)
+{
+    const UriTestCase testCase("x://y/abc/def/xyz", "x", "y", -1, "/abc/def/xyz");
+    VALIDATE
+}
+
 TEST_F (UriParserTest, TypicalPortPath)
 {
     const UriTestCase testCase("connect://192.168.100.132:5432/", "connect", "192.168.100.132", 5432, "/");
     VALIDATE
 }
 
+TEST_F (UriParserTest, BracketedHostnamePort)
+{
+    const UriTestCase testCase("connect://[192.168.100.132]:5432/", "connect", "192.168.100.132", 5432, "/");
+    VALIDATE
+}
+
+TEST_F (UriParserTest, BracketedHostname)
+{
+    const UriTestCase testCase("connect://[192.168.100.132]", "connect", "192.168.100.132", -1, "/");
+    VALIDATE
+}
+
+TEST_F (UriParserTest, BracketedHostnameWithColon)
+{
+    const UriTestCase testCase("connect://[192.168.100.132:5555]:1234", "connect", "192.168.100.132:5555", 1234, "/");
+    VALIDATE
+}
+
 TEST_F (UriParserTest, SchemeHostSeparator)
 {
     const UriTestCase testCase("x:/y");
     VALIDATE
 }
 
 TEST_F (UriParserTest, SchemeHostSeparator2)
 {