FileSpec::Resolve should not turn a filename-only FileSpec into a qualified FileSpec if that file doesn't exist

[jasonmolenda]

Summary

FileSpec::Resolve used to use the realpath() POSIX API to fully qualify filenames. If realpath() was passed "ls" as an argument, if there was no "ls" in the current working directory, it would not change the argument.

The switch to llvm::sys::fs::make_absolute() changed this behavior. Now if you pass in "ls", make_absolute() will prepend the current working directory path and return that.

I can see the argument for make_absolute()'s behavior. If I were *creating* a file, so it didn't exist yet, then the make_absolute() behavior would be reasonable. But this isn't how lldb is using this -- we want to find a file that actually exists on the filesystem.

The failure mode for this is that a user does "lldb ls". Previously lldb would search through $PATH and find /bin/ls. Now when lldb creates a FileSpec for "ls", it resolves it into current-working-directory/ls which doesn't exist and it fails.

This is the second bug clearly tied to this change in behavior (see r222498 where I was fixing another one a couple months ago) - and it's a problematic landmine to leave in place given what lldb means when it says "Resolve" for a FileSpec.

Zachary, I'm not very familiar with the SmallVector/SmallString classes, this is probably not the best way to accomplish what I want to do here. What do you think?

[jasonmolenda]

Forgot to include lldb-commits on the subscription list.

[zturner]

Could we add a test that verifies the behavior of this function? Greg created an SBFileSpec test recently in lldb/test/functionalities/paths/TestPaths.py. As long as we don't use a builtin command like ls or something like it should be fine. Instead you could use something like sys.executable, then the os.path module to get out the filename, then verify that after you resolve it, it matches sys.executable.

source/Host/common/FileSpec.cpp

167–173

You can write this like this:

llvm::SmallString<PATH_MAX> original_path(path.begin(), path.end());

172–178

I'm preeeety sure that make_absolute() guarantees the result will be null terminated. But not quite 100%. If you can determine that it's guaranteed to be null terminated, you can replace all of this with path.data(), which returns a char*. If you want to be extra certain though, you can still make this shorter by writing this instead:

path.push_back(0);
path.pop_back();
if (stat(path.data())) // path.data() is now definitely null terminated.

185–186

This can just be path = original_path;

[zturner]

http://lists.cs.uiuc.edu/pipermail/lldb-dev/2014-October/005551.html

Here's a thread that provides some context for why this was changed.

Your failure mode mentions invoking "lldb ls". If FileSpec::Resolve just
ends up returning "ls" unchanged, who actually resolves it? In the thread i
linked above, it seems i was also dealing with the same issue. Perhaps it's
time to add a function to resolve an executable using PATH? We could put it
in llvm::sys::fs and call it llvm::sys::resolve_executable(StringRef Exe,
bool use_path). Seems generally useful outside of lldb. The LLDB could just
always call this function before constructing a FileSpec to a target
executable.

[zturner]

Fwiw though, i agree if it doesn't exist in the current dir, we shouldn't
be prepending the working dir to it. Let me test this CL on Windows Monday
before you commit though, if you don't mind.

[jasonmolenda]

Thanks for all the comments, no rush on this. I need to update the patch to integrate your suggested changes and try to write a test case still.

[jasonmolenda]

This comment was removed by jasonmolenda.

[jasonmolenda]

Updated patch to include the fixes Zachary suggested, added a simple test case for this behavior to functionality/paths.

[zturner]

source/Host/common/FileSpec.cpp

173

nit: s/nul/null/

179–180

After you call path.clear(), the length is 0. So you can change these two lines to path.append(original_path.begin(), original_path.end()). I think path = original_path will also work.

test/functionalities/paths/TestPaths.py

32–35

Admittedly this is pretty unlikely. You might be able to make it a *tad* better by using tempfile.mktemp() to generate the name of a file which definitely doesn't exist in the current directory. then using something from os.path to strip the directory name from it, and then passing that. I don't feel too strongly though.

[jasonmolenda]

Updated patch to use path.clear(), path.append(original_path.begin(), original_path.end()); as suggested by Zachary.

Kept the "be sure we have a nul terminated string". nul is '\0'. null is (void*)0. We want a nul terminated cstring here.

Left the testsuite as-is, I agree it would be better to get a randomly generated temp name but this is going to be fine.

[jasonmolenda]

Sending source/Host/common/FileSpec.cpp
Sending test/functionalities/paths/TestPaths.py
Transmitting file data ..
Committed revision 230451.

source/Host/common/FileSpec.cpp

         ResolveUsername(path);
 #endif // #ifdef LLDB_CONFIG_TILDE_RESOLVES_TO_USER
 
+    // Save a copy of the original path that's passed in
+    llvm::SmallString<PATH_MAX> original_path(path.begin(), path.end());
+
     llvm::sys::fs::make_absolute(path);
+
+    
+    path.push_back(0);  // Be sure we have a nul terminated string
+    path.pop_back();
+    struct stat file_stats;
+    if (::stat (path.data(), &file_stats) != 0)
+    {
+        path.clear();
+        path.append(original_path.begin(), original_path.end());
+    }
 }
 
 FileSpec::FileSpec() : 

test/functionalities/paths/TestPaths.py

             # No directory path types should have the filename set
             self.assertTrue (f.GetFilename() == None);
 
+    def test_filespec_resolve_doesnt_prepend_cwd_if_file_doesnt_exist (self):
+        file_only = lldb.SBFileSpec("VeryUnlikelToExistInTheCurrentWorkingDirectory", True)
+        # SBFileSpec(path, True) should not prepend the current-working-directory to the
+        # file path if it doesn't exist in the current directory.
+        self.assertTrue (file_only.GetDirectory() == None)
+
     @unittest2.skipUnless(sys.platform.startswith("win32"), "Test for windows only")
     def test_windows_double_slash (self):
         '''Test to check the path with double slash is handled correctly '''