Replace file system forbidden symbols in the hostname which passed to the ModuleCache

[ovyalov]

Summary

ModuleCache hostname may contain symbols that are not allowed in filenames - for example, in Android device_id is used as a hostname and in some cases can have colons ("10.0.0.1:5555").
Replacing forbidden symbols with underscores in order to make hostname filesystem-friendly.

[ovyalov]

Added asterisk symbol

[labath]

According to https://msdn.microsoft.com/en-gb/library/windows/desktop/aa365247%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396, characters with codes 1..31 (basically < ' '), are illegal as well. We might as well convert those, just in case.

I've been sitting on a unit test for the module cache locally for a while now... I guess it's time to put that in, and then we can add a test for this as well.

[clayborg]

At some point we should probably make a host layer call like:

const char *const char *Host::GetIllegalFilenameCharacters();" so that each host can determine the correct thing for the current host. Then FileSpec can be updated to use this function handle this with a method like "void FileSpec::NormalizePathForHost()". This will probably need to be done elsewhere.

[ovyalov]

Added the check for symbol in [1;31] range.

[ovyalov]

In D20548#437567, @labath wrote:

According to https://msdn.microsoft.com/en-gb/library/windows/desktop/aa365247%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396, characters with codes 1..31 (basically < ' '), are illegal as well. We might as well convert those, just in case.

Agree - done.

I've been sitting on a unit test for the module cache locally for a while now... I guess it's time to put that in, and then we can add a test for this as well.

Sounds good.

[ovyalov]

In D20548#437862, @clayborg wrote:

At some point we should probably make a host layer call like:

const char *const char *Host::GetIllegalFilenameCharacters();" so that each host can determine the correct thing for the current host. Then FileSpec can be updated to use this function handle this with a method like "void FileSpec::NormalizePathForHost()". This will probably need to be done elsewhere.

I like this idea of generalizing illegal symbols handling on per-platform basis - since Pavel mentioned that 1..31 range of symbols is forbidden in Windows filenames we may have a method like bool Host::IsIllegalFilenameCharacter(char) to tell whether symbol is illegal on this platform.

[labath]

I like the idea of having a centralized place defining "invalid characters". However, I am not sure if that needs to be (at least in this case) system-dependent. Since presumably the code will need to work on all platforms anyway, having the escaping done in a divergent manner will just complicate testing. I don't see a reason to not define the illegal characters as a union of illegal characters across all platforms (which basically means, using the windows set).

[ovyalov]

Submitted as r270590

source/Utility/ModuleCache.cpp

 
 namespace {
 
 const char* kModulesSubdir = ".cache";
 const char* kLockDirName = ".lock";
 const char* kTempFileName = ".temp";
 const char* kTempSymFileName = ".symtemp";
 const char* kSymFileExtension = ".sym";
+const char* kFSIllegalChars = "\\/:*?\"<>|";
+
+std::string
+GetEscapedHostname(const char* hostname)
+{
+    std::string result(hostname);
+    size_t size = result.size();
+    for (size_t i = 0; i < size; ++i)
+    {
+        if ((result[i] >=1 && result[i] <= 31) ||
+            strchr(kFSIllegalChars, result[i]) != nullptr)
+            result[i] = '_';
+    }
+    return result;
+}
 
 class ModuleLock
 {
 private:
     File m_file;
     std::unique_ptr<lldb_private::LockFile> m_lock;
     FileSpec m_file_spec;
 
ModuleCache::GetAndPut (const FileSpec &root_dir_spec,
     auto error = MakeDirectory (module_spec_dir);
     if (error.Fail ())
         return error;
 
     ModuleLock lock (root_dir_spec,  module_spec.GetUUID (), error);
     if (error.Fail ())
         return Error("Failed to lock module %s: %s", module_spec.GetUUID ().GetAsString().c_str(), error.AsCString ());
 
+    const auto escaped_hostname(GetEscapedHostname(hostname));
     // Check local cache for a module.
-    error = Get (root_dir_spec, hostname, module_spec, cached_module_sp, did_create_ptr);
+    error = Get (root_dir_spec, escaped_hostname.c_str(), module_spec, cached_module_sp, did_create_ptr);
     if (error.Success ())
         return error;
 
     const auto tmp_download_file_spec = JoinPath (module_spec_dir, kTempFileName);
     error = module_downloader (module_spec, tmp_download_file_spec);
     llvm::FileRemover tmp_file_remover (tmp_download_file_spec.GetPath ().c_str ());
     if (error.Fail ())
         return Error("Failed to download module: %s", error.AsCString ());
 
     // Put downloaded file into local module cache.
-    error = Put (root_dir_spec, hostname, module_spec, tmp_download_file_spec, module_spec.GetFileSpec ());
+    error = Put (root_dir_spec, escaped_hostname.c_str(), module_spec, tmp_download_file_spec, module_spec.GetFileSpec ());
     if (error.Fail ())
         return Error ("Failed to put module into cache: %s", error.AsCString ());
 
     tmp_file_remover.releaseFile ();
-    error = Get (root_dir_spec, hostname, module_spec, cached_module_sp, did_create_ptr);
+    error = Get (root_dir_spec, escaped_hostname.c_str(), module_spec, cached_module_sp, did_create_ptr);
     if (error.Fail ())
         return error;
 
     // Fetching a symbol file for the module
     const auto tmp_download_sym_file_spec = JoinPath (module_spec_dir, kTempSymFileName);
     error = symfile_downloader (cached_module_sp, tmp_download_sym_file_spec);
     llvm::FileRemover tmp_symfile_remover (tmp_download_sym_file_spec.GetPath ().c_str ());
     if (error.Fail ())
         // Failed to download a symfile but fetching the module was successful. The module might
         // contain the neccessary symbols and the debugging is also possible without a symfile.
         return Error ();
 
-    error = Put (root_dir_spec, hostname, module_spec, tmp_download_sym_file_spec, GetSymbolFileSpec(module_spec.GetFileSpec ()));
+    error = Put (root_dir_spec, escaped_hostname.c_str(), module_spec, tmp_download_sym_file_spec, GetSymbolFileSpec(module_spec.GetFileSpec ()));
     if (error.Fail ())
         return Error ("Failed to put symbol file into cache: %s", error.AsCString ());
     
     tmp_symfile_remover.releaseFile();
 
     FileSpec symfile_spec = GetSymbolFileSpec (cached_module_sp->GetFileSpec ());
     cached_module_sp->SetSymbolFileFileSpec (symfile_spec);
     return Error ();
 }