[lldb/Host] Improve error messages on unowned read files

[mib]

Summary

When trying to read a core file that is not owned by the user running lldb
and that doesn't have read permission on the file, lldb shows a misleading
error message:

Unable to find process plug-in for core file

This is due to the fact that currently, lldb doesn't check the file
ownership. And when trying to to open and read a core file, the syscall
fails, which prevents a process to be created.

Since lldb already have a portable open syscall interface, lets take
advantage of that and delegate the error handling to the syscall
itself. This way, no matter if the file exists or if the user has proper
ownership, lldb will always try to open the file, and behave accordingly
to the error code returned.

rdar://42630030

Signed-off-by: Med Ismail Bennani <[e-mail]>

[mib]

lldb/test/API/commands/target/basic/TestTargetCommand.py

343

Currently, I try to open the root user home directory, since I sure it's not readable by other users on UNIX systems.

I went this route, because doing a os.chown requires privileges, and the test suites doesn't run in a privileged mode.

I'm open to suggestions on how I could test this on different platforms.

[shafik]

lldb/source/Host/common/FileSystem.cpp

504 ↗

(On Diff #259557)

Since file is a FileSpec can't we just use ReadableByCurrentUser(const FileSpec &file_spec) here?

[mib]

lldb/source/Host/common/FileSystem.cpp

504 ↗

(On Diff #259557)

Good catch!

[mib]

Address Shafik's comment.

[labath]

I'm not very fond of this reverse engineering of the checks that os performs when opening a file (this is not the only place doing it). Could we just make sure that the operation for opening a file returns a reasonable error message, and then do something like

Status /*or Error, or Expected...*/ error = File::Open(core);
if (error)
  SetErrorStringWithFormatv("Cannot open {0}: {1}.", core, error);

?

If done right, that should produce messages like:

Cannot open /my/core.file: File does not exist.
Cannot open /my/core.file: Permission denied.
Cannot open /my/core.file: Is a directory.
...

It may not as "user friendly" as "file is not readable by the current user", but it will at least be correct (it's not a problem to come up with situations where this file will be readable by the "current user", but we will not print this error message, and vice-versa), and it will also match what other tools are likely to print. Plus it will be composable, and involve less code at each call site.

lldb/source/Host/common/FileSystem.cpp

503 ↗

(On Diff #259608)

This is not the correct way to work with Twines. If you want to try to be efficient, you should call toStringRef (there are examples of how to do that throughout llvm). If you don't care about that, you can call str().

lldb/test/API/commands/target/basic/TestTargetCommand.py

343

if you set the mode of a file to 0000, you won't be able to open it even if you are still its owner.

[mib]

Removed the introduced FileSystem::ReadableByCurrentUser to rely only FileSystem::Open as Pavel suggested.
Updated the tests to reflect the changes introduced.

[JDevlieghere]

I also prefer this approach, seems less fragile from a testing perspective and reduces the different code paths.

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

102 ↗

(On Diff #260751)

We should be consistent with the braces, both clauses are single-line. Personally I'd refactor the code and make this an early return instead.

187 ↗

(On Diff #260751)

Not your fault, but it seems weird that we're doing this check after using the resolved_module_spec and only if it failed.. Shouldn't we try to read it first and stop early if that fails?

[labath]

Yes, this is definitely better.

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

187 ↗

(On Diff #260751)

In an ideal world the actual operation which tries to create the module would return an error and we would use that. That way we wouldn't be accessing the file twice, and creating opportunities for inconsistencies (or races). In that sense, this code is still trying to "reverse engineer" why the earlier operation failed, but I think it is a step forward, as it reduces the amount if reversing.

195 ↗

(On Diff #260751)

I guess this should be also updated to use the error message from the os(?)

lldb/test/API/commands/target/basic/TestTargetCommand.py

329

Now that the error message comes from the OS, we might need to adjust the expectation based on that. Just an FYI, in case you get some failure message from some bot (most likely the windows one).

342

This is not testing an unowned file anymore, and is probably redudant given the test above it. Given that the new implementation does not do anything special wrt. ownership, I don't think an explicit test for those.

[mib]

Removed redundant test.
Refactored PlatformPOSIX following Jonas and Pavel's comments.

[JDevlieghere]

Looks good to me.

[labath]

I'm not sure this new version is for the better :(.

Error and Expected objects have a contract where you're supposed to "consume" each object before you destroy or reassign it. The complex control flow in that function makes it very hard to see that this is really the case. I marked the cases where I believe you didn't do that, but I can't be sure I found all of them. For this kind of pattern to work there'd need to be more refactoring of the function in order to make the lifetime and state of the expected object more obvious. That's why I wasn't pushing for this direction in my previous comment...

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

96 ↗

(On Diff #261384)

here you'll need to do a consumeError(resolved_module.takeError()) before you can reuse the object.

124 ↗

(On Diff #261384)

error not consumed here

197 ↗

(On Diff #261384)

error not consumed here.

[mib]

Add error "consumption" following @labath comment.

[mib]

As agreed on IRC with @labath, the changes done to PlatformPOSIX::ResolveExecutable will be removed from this patch and submitted separately.

[labath]

Yeah, as I said, I think this is already an improvement, which I am not sure that could be said of the other version, because of the expected-handling complexities. LGTM, modulo the overzealous conversion to AppendError.

lldb/source/Commands/CommandObjectTarget.cpp

417

I guess this should stay as AppendMessage

[mib]

Landed in 015117411e11

lldb/source/Commands/CommandObjectTarget.cpp

 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     const size_t argc = command.GetArgumentCount();
     FileSpec core_file(m_core_file.GetOptionValue().GetCurrentValue());
     FileSpec remote_file(m_remote_file.GetOptionValue().GetCurrentValue());
 
     if (core_file) {
-      if (!FileSystem::Instance().Exists(core_file)) {
-        result.AppendErrorWithFormat("core file '%s' doesn't exist",
-                                     core_file.GetPath().c_str());
-        result.SetStatus(eReturnStatusFailed);
-        return false;
-      }
-      if (!FileSystem::Instance().Readable(core_file)) {
-        result.AppendErrorWithFormat("core file '%s' is not readable",
-                                     core_file.GetPath().c_str());
+      auto file = FileSystem::Instance().Open(
+          core_file, lldb_private::File::eOpenOptionRead);
+
+      if (!file) {
+        result.AppendErrorWithFormatv("Cannot open '{0}': {1}.",
+                                      core_file.GetPath(),
+                                      llvm::toString(file.takeError()));
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
     }
 
     if (argc == 1 || core_file || remote_file) {
       FileSpec symfile(m_symbol_file.GetOptionValue().GetCurrentValue());
       if (symfile) {
-        if (FileSystem::Instance().Exists(symfile)) {
-          if (!FileSystem::Instance().Readable(symfile)) {
-            result.AppendErrorWithFormat("symbol file '%s' is not readable",
-                                         symfile.GetPath().c_str());
-            result.SetStatus(eReturnStatusFailed);
-            return false;
-          }
-        } else {
-          char symfile_path[PATH_MAX];
-          symfile.GetPath(symfile_path, sizeof(symfile_path));
-          result.AppendErrorWithFormat("invalid symbol file path '%s'",
-                                       symfile_path);
+        auto file = FileSystem::Instance().Open(
+            symfile, lldb_private::File::eOpenOptionRead);
+
+        if (!file) {
+          result.AppendErrorWithFormatv("Cannot open '{0}': {1}.",
+                                        symfile.GetPath(),
+                                        llvm::toString(file.takeError()));
           result.SetStatus(eReturnStatusFailed);
           return false;
         }
       }
 
       const char *file_path = command.GetArgumentAtIndex(0);
       static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
       Timer scoped_timer(func_cat, "(lldb) target create '%s'", file_path);
if (argc == 1 || core_file || remote_file) {
         debugger.GetTargetList().SetSelectedTarget(target_sp.get());
         if (must_set_platform_path) {
           ModuleSpec main_module_spec(file_spec);
           ModuleSP module_sp =
               target_sp->GetOrCreateModule(main_module_spec, true /* notify */);
           if (module_sp)
             module_sp->SetPlatformFileSpec(remote_file);
         }
+
         if (core_file) {
-          char core_path[PATH_MAX];
-          core_file.GetPath(core_path, sizeof(core_path));
-          if (FileSystem::Instance().Exists(core_file)) {
-            if (!FileSystem::Instance().Readable(core_file)) {
-              result.AppendMessageWithFormat(
-                  "Core file '%s' is not readable.\n", core_path);
-              result.SetStatus(eReturnStatusFailed);
-              return false;
-            }
-            FileSpec core_file_dir;
-            core_file_dir.GetDirectory() = core_file.GetDirectory();
-            target_sp->AppendExecutableSearchPaths(core_file_dir);
 
-            ProcessSP process_sp(target_sp->CreateProcess(
-                GetDebugger().GetListener(), llvm::StringRef(), &core_file));
 
-            if (process_sp) {
-              // Seems weird that we Launch a core file, but that is what we
-              // do!
-              error = process_sp->LoadCore();
 
-              if (error.Fail()) {
-                result.AppendError(
-                    error.AsCString("can't find plug-in for core file"));
-                result.SetStatus(eReturnStatusFailed);
-                return false;
-              } else {
-                result.AppendMessageWithFormat(
-                    "Core file '%s' (%s) was loaded.\n", core_path,
-                    target_sp->GetArchitecture().GetArchitectureName());
-                result.SetStatus(eReturnStatusSuccessFinishNoResult);
-              }
-            } else {
-              result.AppendErrorWithFormat(
-                  "Unable to find process plug-in for core file '%s'\n",
-                  core_path);
-              result.SetStatus(eReturnStatusFailed);
-            }
-          } else {
-            result.AppendErrorWithFormat("Core file '%s' does not exist\n",
-                                         core_path);
+              result.AppendErrorWithFormatv(
+                  "Core file '{0}' ({1}) was loaded.\n", core_file.GetPath(),
+            result.AppendErrorWithFormatv(
+                "Unable to find process plug-in for core file '{0}'\n",
+                core_file.GetPath());
             result.SetStatus(eReturnStatusFailed);
           }
         } else {
           result.AppendMessageWithFormat(
               "Current executable set to '%s' (%s).\n",
               file_spec.GetPath().c_str(),
               target_sp->GetArchitecture().GetArchitectureName());
           result.SetStatus(eReturnStatusSuccessFinishNoResult);

lldb/test/API/commands/target/basic/TestTargetCommand.py

class targetCommandTestCase(TestBase):
     @no_debug_info_test
     def test_target_create_multiple_args(self):
         self.expect("target create a b", error=True,
                     substrs=["'target create' takes exactly one executable path"])
 
     @no_debug_info_test
     def test_target_create_nonexistent_core_file(self):
         self.expect("target create -c doesntexist", error=True,
-                    substrs=["core file 'doesntexist' doesn't exist"])
+                    substrs=["Cannot open 'doesntexist': No such file or directory"])
 
     # Write only files don't seem to be supported on Windows.
     @skipIfWindows
     @no_debug_info_test
     def test_target_create_unreadable_core_file(self):
         tf = tempfile.NamedTemporaryFile()
         os.chmod(tf.name, stat.S_IWRITE)
         self.expect("target create -c '" + tf.name + "'", error=True,
-                    substrs=["core file '", "' is not readable"])
+                    substrs=["Cannot open '", "': Permission denied"])
 
     @no_debug_info_test
     def test_target_create_nonexistent_sym_file(self):
         self.expect("target create -s doesntexist doesntexisteither", error=True,
-                    substrs=["invalid symbol file path 'doesntexist'"])
+                    substrs=["Cannot open '", "': No such file or directory"])
 
     @skipIfWindows
     @no_debug_info_test
     def test_target_create_invalid_core_file(self):
         invalid_core_path = os.path.join(self.getSourceDir(), "invalid_core_file")
         self.expect("target create -c '" + invalid_core_path + "'", error=True,
                     substrs=["Unable to find process plug-in for core file '"])
 
 
     # Write only files don't seem to be supported on Windows.
     @skipIfWindows
     @no_debug_info_test
     def test_target_create_unreadable_sym_file(self):
         tf = tempfile.NamedTemporaryFile()
         os.chmod(tf.name, stat.S_IWRITE)
         self.expect("target create -s '" + tf.name + "' no_exe", error=True,
-                    substrs=["symbol file '", "' is not readable"])
+                    substrs=["Cannot open '", "': Permission denied"])
 
     @no_debug_info_test
     def test_target_delete_all(self):
         self.buildAll()
         self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
         self.runCmd("file " + self.getBuildArtifact("b.out"), CURRENT_EXECUTABLE_SET)
         self.expect("target delete --all")
         self.expect("target list", substrs=["No targets."])