Skip to content

Commit

Permalink
[lldb/Host] Improve error messages on unowned read files
Browse files Browse the repository at this point in the history 
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

https://reviews.llvm.org/D78712

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>
  • Loading branch information
@medismailben
medismailben committed May 4, 2020
1 parent 1af8d34 commit 74f2a9a
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 60 deletions.
91 changes: 35 additions & 56 deletions lldb/source/Commands/CommandObjectTarget.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,15 +271,13 @@ class CommandObjectTargetCreate : public CommandObjectParsed {
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;
}
Expand All @@ -288,18 +286,13 @@ class CommandObjectTargetCreate : public CommandObjectParsed {
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;
}
Expand Down Expand Up @@ -401,48 +394,34 @@ class CommandObjectTargetCreate : public CommandObjectParsed {
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);
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));
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 (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);
if (error.Fail()) {
result.AppendError(
error.AsCString("can't find plug-in for core file"));
result.SetStatus(eReturnStatusFailed);
return false;
} else {
result.AppendMessageWithFormatv("Core file '{0}' ({1}) was loaded.\n", core_file.GetPath(),
target_sp->GetArchitecture().GetArchitectureName());
result.SetStatus(eReturnStatusSuccessFinishNoResult);
}
} else {
result.AppendErrorWithFormat("Core file '%s' does not exist\n",
core_path);
result.AppendErrorWithFormatv(
"Unable to find process plug-in for core file '{0}'\n",
core_file.GetPath());
result.SetStatus(eReturnStatusFailed);
}
} else {
Expand Down
8 changes: 4 additions & 4 deletions lldb/test/API/commands/target/basic/TestTargetCommand.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ def test_target_create_multiple_args(self):
@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
Expand All @@ -335,12 +335,12 @@ 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
Expand All @@ -357,7 +357,7 @@ 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):
Expand Down

0 comments on commit 74f2a9a

Please sign in to comment.