kimmknight / remoteapptool

Create and manage RemoteApps hosted on Windows 7, 8, 10, 11, XP and Server. Generate RDP and MSI files for clients.
Other
2.91k stars 379 forks source link

RDP file not being copied in some cases #3

Closed kimmknight closed 1 month ago

kimmknight commented 5 years ago

https://groups.google.com/d/msg/remoteapptool/B6Ni3Xf_K68/o4ZlUHuhCAAJ

WinIO error when trying to copy/move the RDP file. Maybe antivirus? Is there a way to avoid this type of problem?

MrBrianGale commented 5 years ago

Was looking at this one and have some thoughts on how to correct it, but not sure they are good ones as I cannot reproduce the bug. My thoughts are, first, line 72 of RDP2MSILib.vb, rather than looking if the rdpParentFolder is the same as TempPath and then setting rdpInTemp to true, does it make more sense to do a FileExists(rdpTempPath) to determine if the file already exists in the temp folder? And if it does, I am thinking that attempting to delete the file may fix the problem. If the delete fails, throw a user friendly error (such as a message box pop up telling the end user the file is in use and cannot be overwritten).

What I am thinking is happening is for some reason, windows is trying to be helpful (unsuccessfully) and is providing 2 different TEMP folders; one for RemoteApp Tool and one for RDP2MSI. So rdpParentFolder and TempPath are coming back with 2 different paths so rdpInTemp is false. This causes the file to be copied with overwrite while it is potentially in use by something (antivirus, windows indexer, end user leaving it open, etc).

The other way I can see to fix this is to toss a TRY CATCH around the CopyFile calls on line 76 and 79 (as either one could fail) and in the event the copy fails, we catch it, change the rdpTempPath and/or icoTempPath to something unique (such as TempPath & "\" & ProductBaseFileName & DateTime.Now.ToString("yyyyMMddHHmmss") &".rdp" or ".ico". Probably want to do something similar with the MoveFile on line 98 as well as there is a small chance that could fail too.

The problem I have with making this change is I am not able to reproduce the problem, so I have no idea if my suggestions will fix it or not and therefore I am not confident about making a pull request for this fix.

MrBrianGale commented 5 years ago

Do you know if this issue still exists? Part of me is wondering if it is an issue with Wix 3.10.3? Also, do you know if anyone has been able to reproduce the issue? I am going to spin up a Windows Server 2016 VM tonight and put the latest version of wix on it and see if I can reproduce the problem with no additional software.

kimmknight commented 5 years ago

Very good ideas! It's a shame we can't replicate the issue to test which one will work best.

The reports on this have been inconsistent. I have not once been able to replicate the issue - I have built several Server 2016 VMs with no issues using the tool.

My initial thought was that because Server 2016 includes Windows Defender real-time scanning, perhaps on slower systems, the temp file was still being scanned while RemoteApp Tool is trying to do something with it.

People have emailed me reporting the problem but insisting they don't have any antivirus installed (but I suspect they might have been overlooking Windows Defender).

Nobody has reported the problem for a while! Could be because it is resolved, or maybe just that people aren't using the tool on 2016.

I didn't suspect WiX because it's all happening before WiX is called, but perhaps .NET/Windows queues the copy operation and it hasn't quite finished before the code continues.

I'd be interested to use your TRY/CATCH idea to continually attempt the copy/delete operation until it succeeds (perhaps with a timeout/error). This would prevent future antivirus issues, even if that isn't the cause of the current problem. But also, happy to try any ideas, we just need to replicate the problem first!

MrBrianGale commented 5 years ago

May have found a better solution to this: http://csharphelper.com/blog/2017/01/see-processes-file-locked-c/

The code is C# specific, but I can convert that to VB.NET and should be able to implement it in this project. Basically, before we copy, check for file locks. If there is a file lock, wait for some arbitrary amount of time (5 seconds or something... an AV scan should complete in that timeframe as an RDP file is tiny) and try again. If it is still locked, present the locking information to the end user and let them close whatever is locking the file OR send us a copy of the log so we can fix things.

Should I work on implementing that? It looks pretty easy to do.

kimmknight commented 5 years ago

I like that this would not only potentially fix the issue but also give users (and us) an insight into the problem!

MrBrianGale commented 5 years ago

Probably not going to have time to implement this today, but should have something in place by Friday. I think telling the end user what application(s) are locking the file and giving them a retry option is probably the safest bet. Grab all of the processes locking the file, if the count of them is greater than 0, present the end user with a messagebox telling them what is locking the file and asking if they want to try again (yes/no). If they say yes, we try things a second time. If they say no, we cancel out of the creation process and bring them back to the main form... or should it bring them back to the create RDP/MSI form?

MrBrianGale commented 5 years ago

Making progress on this. Don't have it working 100% yet, but it is coming along. Hopefully have something ready to test at some point tomorrow.

UPDATE - Got it working to report back file locks, application that is causing the lock and the PID of the application as some things (like Excel) can have multiple PIDs. Did a quick test on it with an excel file and if I had it open, I got an error, if it was closed, no error. So now just need to implement this over to any place we call copy file , move file, create file or delete file. Tomorrow, I'll update all of the locations I can find for copy, move, create or delete to check for a file lock prior to any of those operations. This does NOT close this issue, but gives us a better launching point for determining what is causing the file locks.

MrBrianGale commented 4 years ago

Pull request in place. Got the code working and tested. Feel free to go in and change things if you don't like the way I implemented it.

This does NOT fix issue 3, just helps us debug what is going on.

abcbarryn commented 4 years ago

I appear to be experiencing this issue.

Here is the debug output...

See the end of this message for details on invoking just-in-time (JIT) debugging instead of this dialog box.

** Exception Text ** System.IO.IOException: The process cannot access the file 'C:\Users\s-bnelson\AppData\Local\Temp\2\run_centralink.rdp' because it is being used by another process. at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath) at System.IO.File.InternalCopy(String sourceFileName, String destFileName, Boolean overwrite, Boolean checkHost) at Microsoft.VisualBasic.FileIO.FileSystem.CopyOrMoveFile(CopyOrMove operation, String sourceFileName, String destinationFileName, Boolean overwrite, UIOptionInternal showUI, UICancelOption onUserCancel) at RDP2MSIlib.RDP.CreateMSI(String DestinationPath) at RemoteApp_Tool.RemoteAppCreateClientConnection.CreateButton_Click(Object sender, EventArgs e) at System.Windows.Forms.Control.OnClick(EventArgs e) at System.Windows.Forms.Button.OnClick(EventArgs e) at System.Windows.Forms.Button.OnMouseUp(MouseEventArgs mevent) at System.Windows.Forms.Control.WmMouseUp(Message& m, MouseButtons button, Int32 clicks) at System.Windows.Forms.Control.WndProc(Message& m) at System.Windows.Forms.ButtonBase.WndProc(Message& m) at System.Windows.Forms.Button.WndProc(Message& m) at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)

** Loaded Assemblies ** mscorlib Assembly Version: 4.0.0.0 Win32 Version: 4.7.3460.0 built by: NET472REL1LAST_B CodeBase: file:///C:/Windows/Microsoft.NET/Framework64/v4.0.30319/mscorlib.dll

RemoteApp Tool Assembly Version: 5.3.0.0 Win32 Version: 5.3.0.0 CodeBase: file:///C:/Program%20Files%20(x86)/RemoteApp%20Tool/RemoteApp%20Tool.exe

Microsoft.VisualBasic Assembly Version: 10.0.0.0 Win32 Version: 14.6.1590.0 built by: NETFXREL2 CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/Microsoft.VisualBasic/v4.0_10.0.0.0__b03f5f7f11d50a3a/Microsoft.VisualBasic.dll

System Assembly Version: 4.0.0.0 Win32 Version: 4.7.3416.0 built by: NET472REL1LAST_B CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/System/v4.0_4.0.0.0__b77a5c561934e089/System.dll

System.Core Assembly Version: 4.0.0.0 Win32 Version: 4.7.3570.0 built by: NET472REL1LAST_B CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/System.Core/v4.0_4.0.0.0__b77a5c561934e089/System.Core.dll

System.Windows.Forms Assembly Version: 4.0.0.0 Win32 Version: 4.7.3394.0 built by: NET472REL1LAST_C CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/System.Windows.Forms/v4.0_4.0.0.0__b77a5c561934e089/System.Windows.Forms.dll

System.Drawing Assembly Version: 4.0.0.0 Win32 Version: 4.7.3394.0 built by: NET472REL1LAST_C CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/System.Drawing/v4.0_4.0.0.0__b03f5f7f11d50a3a/System.Drawing.dll

System.Configuration Assembly Version: 4.0.0.0 Win32 Version: 4.7.3394.0 built by: NET472REL1LAST_C CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/System.Configuration/v4.0_4.0.0.0__b03f5f7f11d50a3a/System.Configuration.dll

System.Xml Assembly Version: 4.0.0.0 Win32 Version: 4.7.3394.0 built by: NET472REL1LAST_C CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/System.Xml/v4.0_4.0.0.0__b77a5c561934e089/System.Xml.dll

System.Runtime.Remoting Assembly Version: 4.0.0.0 Win32 Version: 4.7.3394.0 built by: NET472REL1LAST_C CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/System.Runtime.Remoting/v4.0_4.0.0.0__b77a5c561934e089/System.Runtime.Remoting.dll

RemoteAppLib Assembly Version: 1.0.0.1 Win32 Version: 1.0.0.1 CodeBase: file:///C:/Program%20Files%20(x86)/RemoteApp%20Tool/RemoteAppLib.DLL

IconLib Assembly Version: 0.73.0.0 Win32 Version: 0.73.0.0 CodeBase: file:///C:/Program%20Files%20(x86)/RemoteApp%20Tool/IconLib.DLL

RDP2MSIlib Assembly Version: 1.0.0.0 Win32 Version: 1.0.0.0 CodeBase: file:///C:/Program%20Files%20(x86)/RemoteApp%20Tool/RDP2MSIlib.DLL

Accessibility Assembly Version: 4.0.0.0 Win32 Version: 4.6.1590.0 built by: NETFXREL2 CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_MSIL/Accessibility/v4.0_4.0.0.0__b03f5f7f11d50a3a/Accessibility.dll

RDPFileLib Assembly Version: 1.0.0.0 Win32 Version: 1.0.0.0 CodeBase: file:///C:/Program%20Files%20(x86)/RemoteApp%20Tool/RDPFileLib.DLL

System.Web Assembly Version: 4.0.0.0 Win32 Version: 4.7.3394.0 built by: NET472REL1LAST_C CodeBase: file:///C:/Windows/Microsoft.Net/assembly/GAC_64/System.Web/v4.0_4.0.0.0__b03f5f7f11d50a3a/System.Web.dll

** JIT Debugging ** To enable just-in-time (JIT) debugging, the .config file for this application or computer (machine.config) must have the jitDebugging value set in the system.windows.forms section. The application must also be compiled with debugging enabled.

For example:

When JIT debugging is enabled, any unhandled exception will be sent to the JIT debugger registered on the computer rather than be handled by this dialog box.

MrBrianGale commented 4 years ago

To confirm, which antivirus are you running? If you navigate to the path "C:\Users\s-bnelson\AppData\Local\Temp\2\", do you see a .rdp file in that location? If so, can you manually delete the file or is it still locked?

abcbarryn commented 4 years ago

Running Windows defender. Yes there is an rdp file in that location. Yes, I can delete it.

MrBrianGale commented 4 years ago

Did some work and have better handling now of the file locking which will hopefully fix the issue. Can I get someone to test out this version: https://github.com/MrBrianGale/remoteapptool/releases/tag/AdvancedOptions%7C_RDPOnly

ksuviper commented 4 years ago

I am having the same issue as abcbarryn on Windows Server 2016 and 2019. I also tried your test version and the error seems to have moved to the Icon file instead of the rdp file.

I have downloaded a copy of the code to look into the issue as well. I'll update if I find anything out.

Here is a copy of the error.

System.IO.IOException: The process cannot access the file 'C:\Users\astaggenborg\AppData\Local\Temp\ABAS2018.ico' because it is being used by another process. at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath) at System.IO.File.InternalCopy(String sourceFileName, String destFileName, Boolean overwrite, Boolean checkHost) at Microsoft.VisualBasic.FileIO.FileSystem.CopyOrMoveFile(CopyOrMove operation, String sourceFileName, String destinationFileName, Boolean overwrite, UIOptionInternal showUI, UICancelOption onUserCancel) at Microsoft.VisualBasic.MyServices.FileSystemProxy.CopyFile(String sourceFileName, String destinationFileName, Boolean overwrite) at RDP2MSIlib.RDP.CreateMSI(String DestinationPath) in C:\isgitlab\remoteapptool_bmg\RDP2MSILib\RDP2MSILib.vb:line 125 at RemoteApp_Tool.RemoteAppCreateClientConnection.CreateButton_Click(Object sender, EventArgs e) in C:\isgitlab\remoteapptool_bmg\remoteapp-tool\RemoteAppCreateClientConnection.vb:line 270 at System.Windows.Forms.Control.OnClick(EventArgs e) at System.Windows.Forms.Button.OnClick(EventArgs e) at System.Windows.Forms.Button.OnMouseUp(MouseEventArgs mevent) at System.Windows.Forms.Control.WmMouseUp(Message& m, MouseButtons button, Int32 clicks) at System.Windows.Forms.Control.WndProc(Message& m) at System.Windows.Forms.ButtonBase.WndProc(Message& m) at System.Windows.Forms.Button.WndProc(Message& m) at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)

MrBrianGale commented 4 years ago

So the issue is not fixed in the test builds, it just moves off of the rdp file and jumps over to the ico. So that means (to me) progress! Just need to implement similar logic in the ico copy section and we may be able to squish this bug! I will have a look at this sometime this week and see what I can come up with. The application first checks the rdp file then checks the ico file, so I think that since you got past the rdp copy, the ico copy should be fairly easy to fix.

ksuviper commented 4 years ago

Now that I have everything setup on my dev machine, I ran from the code directly and it seems that the filelock code isn't catching the locked rdp file, and is crashing on the copyfile line. I will investigate it more tomorrow.

MrBrianGale commented 4 years ago

I have not had a lot of time to look into this but my guess is one of 2 things: 1 - the file lock is on the SOURCE file, not the destination file. I've seen stranger things than that before, so it wouldn't surprise me if windows won't allow the file copy from the source for that file type if the source is locked (likely locked by this tool). 2 - the file isn't actually "locked" by another file, but is being marked as protected by the OS for some reason.
3 - gremlins (someone fed them after midnight) 4 - the lock is taken out on the file AFTER the lock check

So the next revision I am going to work on is checking for locks on source and destination files and if locks are not in place it will do a try catch block over the copy. If the copy fails, it renames the destination file to have "_new" on the end. Then I can clean up that bit of code too and break it into a few more functions.

The way I tested the lock check functionality was by using a file type I KNOW gets locks (word document) and tried to overwrite the word doc with the rdp file. I got a pop up telling me the file is locked. In my test version, one thing I changed was that if the destination file existed, it appended a number onto the end and thus it would be a completely new file. Others indicated that made no difference, but now I am wondering if it just moved the copy failure from the rdp file (the one I appended a digit to) to the ico file (the one I DIDN'T append a digit to) and they just didn't notice the very minor change.

It is a fun one as I cannot reproduce it on any of my systems (windows server 2012, windows server 2019, windows 10), yet others seem to have no problem making it happen.

I do appreciate another set of eyes on this as mine have gone buggy trying to figure this out.

MrBrianGale commented 4 years ago

May have fixed this bug for good: https://github.com/MrBrianGale/remoteapptool/releases/tag/Issue3_Take2

Can I get someone out there to give that version a go? Side effects of my "fix" include that the actual .RDP file that is generated MAY contain _NEW in the file name, but this shouldn't cause anything to break due to it. If this does cause problems, I have a few other ideas that I could try (force-delete the file in the temp folder prior to the copy for example or to do some folder creation to store the file such as making a RAT (RemoteAppTool) folder and if it exists, make RAT1 and if it exists make RAT2 and so on and put the destination file in there). If that version tests well, I will make a pull request.

ksuviper commented 4 years ago

I just tried it out on my Server 2019 and it worked without issue.

kimmknight commented 1 year ago

@MrBrianGale, has wk6768 possibly found the issue and fixed it with the change in pull request #79 ?

Their original comment was:

When the user name is "Administrator" If rdpParentFolder = TempPath Then rdpInTemp = True will report an exception. Because the statement Dim rdpParentFolder = My.Computer.FileSystem.GetParentPath(rdpFilePath) returned the user name was "Administrator", but Dim TempPath = Environment.GetEnvironmentVariable("TEMP") returned was "Admin~1".

xtra1211 commented 1 year ago

Thanks for the howsome tool :) here's my feed back on that issue I met on my way.

if this can be of any help ...

logging to the Win10 ETS VM that host the app with an "adminxxxx" login name would lead to the issue. logging again with a non admin account to the VM then launching the app, (giving another admin account like "bobadm" to allow pass through UAC) then the application would generate the MSI with no issue... I don't really know what step did the trick ... but last comment did put me on the right way to do so ... looks like the temp path gets truncated or something like that as far as i understand if you use an account that name is adminABCD ...

When the user name is "Administrator" If rdpParentFolder = TempPath Then rdpInTemp = True will report an exception.

MrBrianGale commented 1 year ago

I think you may have got the nail on the head (maybe). My guess is that adminabcd is getting truncated to admin~1 which is the administrator folder, not the adminabcd folder... May need to do some digging into why it is truncating as that should not be required on any modern system/application...

kimmknight commented 1 month ago

I'm thinking we close this. I've seen no reports of the issue since the 6.1.0.0 release in Jan. Could just reopen the issue if needed later on. What do you think @MrBrianGale ?

abcbarryn commented 1 month ago

There are definitely still issues.

kimmknight commented 1 month ago

Thanks @abcbarryn. The previous discussions about this where quite a while ago. Can you confirm that you're using RemoteApp Tool 6.1.0.0 (or newer)? Also what version/edition of Windows is the running on the host?

abcbarryn commented 1 month ago

Actually, sorry, I confused this with another piece of software. This software works as expected. My apologies.

kimmknight commented 1 month ago

Actually, sorry, I confused this with another piece of software. This software works as expected. My apologies.

Great, thanks for letting us know 👍

MrBrianGale commented 1 month ago

I'm that case, I agree that this is safe to close. If the problem pops up again, a new issue can be created.