[lldb][Windows] Enforce exec permission using Platform::Install() from Windows host

[slydiman]

Target::Install() set 0700 permissions for the main executable file. Platform::Install() just copies permissions from the source. But the permission eFilePermissionsUserExecute is missing on the Windows host. A lot of tests failed in case of Windows host and Linux target because of this issue. There is no API to provide the exec flag. This patch set the permission eFilePermissionsUserExecute for all files installed via Platform::Install() from the Windows host. It fixes a lot of tests in case of Windows host and Linux target.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

Target::Install() set 0700 permissions for the main executable file. Platform::Install() just copies permissions from the source. But the permission eFilePermissionsUserExecute is missing on the Windows host. A lot of tests failed in case of Windows host and Linux target because of this issue. There is no API to provide the exec flag. This patch set the permission eFilePermissionsUserExecute for all files installed via Platform::Install() from the Windows host. It fixes a lot of tests in case of Windows host and Linux target.

---

Full diff: https://github.com/llvm/llvm-project/pull/91887.diff

1 Files Affected:

• (modified) lldb/source/Target/Platform.cpp (+4)

diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 4af4aa68ccd01..0e7739b3712d7 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -1227,6 +1227,10 @@ Status Platform::PutFile(const FileSpec &source, const FileSpec &destination,
   if (permissions == 0)
     permissions = lldb::eFilePermissionsFileDefault;
 
+#if defined(_WIN32)
+  permissions |= lldb::eFilePermissionsUserExecute;
+#endif
+
   lldb::user_id_t dest_file = OpenFile(
       destination, File::eOpenOptionCanCreate | File::eOpenOptionWriteOnly |
                        File::eOpenOptionTruncate | File::eOpenOptionCloseOnExec,

[labath]

The ifdef in the generic code is not exactly ideal, and I'm wondering if we should just do the same thing as Target::Install does (i.e., set the execute flag unconditionally). Looking at the history, it appears that the Target::Install code was introduced to fix the same problem you are having, and it's quite possible this code path was not noticed because we were still in the very early stages of cross-debugging bringup (or the function did even not exist back then).

In fact, if I'm reading this correctly, you should be able to remove the Target::Install workaround if you put the code here.

WDYT?

[slydiman]

Target::Install() and Platform::Install() are used indirectly in many cases. For example look at the test lldb/test/API/python_api/hello_world/TestHelloWorld.py. target.LaunchSimple() uses Target::Install() and there is no problem with the exec permission. spawnSubprocess() uses the class _RemoteProcess and finally Platform::Install(). spawnSubprocess() is used in 27 test files and they are failed is case of Windows host and Linux target.

Target::Install does (i.e., set the execute flag unconditionally)

Target::Install() checks is_main_executable enumerating all modules.
But Target::Install()'s logic is not applicable in most cases where Platform::Install() is used.
I think Target::Install is not a workaround and we cannot remove this code.
I'd say this patch is a workaround for the case host=Windows and target!=Windows. We can even add a comment FIXME:... if someone will have an idea how to fix it better way.

[labath]

I'm not sure I'm following your reasoning. I'm not saying we should replace/remove Target::Install. I'm saying we should make everything installed by Platform::Install executable by default (which, by extension, includes everything installed by Target::Install, because it delegates to this function.

And you're saying that's not a good idea, because what? That not everything installed through Platform::Install is an actual executable?

[slydiman]

@labath, Sorry for not being clear with my comment. Let me re-phrase.

I think unconditionally setting the executable flag for everything installed by Platform::Install by default for all platforms is overkill.

BTW, there is no API to change this behavior, so by default means always.

Implementation details aside. Test files have the correct permissions set if the host allows it. Platform::Install copies the host file permissions to the target. This seems a correct behavior except for the case when the host has a smaller set of permissions than the target, or permission sets on the host and the target do not match significantly. I'm aware of the only such case: Windows host and POSIX target. Do you know more?

[slydiman]

I have updated the patch w/o #ifdef. Note if GetPermissions() returned 0 it means Windows host in most cases.

[labath]

I like this new version, thanks for sticking with me.