Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RDP file not being copied in some cases #3

Open
kimmknight opened this issue Sep 29, 2019 · 21 comments
Open

RDP file not being copied in some cases #3

kimmknight opened this issue Sep 29, 2019 · 21 comments
Assignees
Labels
bug Something isn't working

Comments

@kimmknight
Copy link
Owner

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?

@kimmknight kimmknight added the bug Something isn't working label Sep 29, 2019
@MrBrianGale
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Collaborator

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
Copy link
Owner Author

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

@MrBrianGale
Copy link
Collaborator

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
Copy link
Collaborator

MrBrianGale commented Nov 14, 2019

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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
Copy link

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

MrBrianGale added a commit to MrBrianGale/remoteapptool that referenced this issue May 5, 2020
Fixes Issue#3
  *  After some testing and fiddling with things, there is a case that can occur where the rdpTempPath and rdpFilePath can be the same.  This results in the file lock check saying the file is NOT locked (as it is not at that point) BUT the application still throwing a JIT error
  *  Correction is to check if the file path and temp path are the same and if so, rename the temp path.  As it is temp, we can name it anything we want.  I append a _2 to the end.
MrBrianGale added a commit to MrBrianGale/remoteapptool that referenced this issue May 20, 2020
Fixes Issue#3
  *  After some testing and fiddling with things, there is a case that can occur where the rdpTempPath and rdpFilePath can be the same.  This results in the file lock check saying the file is NOT locked (as it is not at that point) BUT the application still throwing a JIT error
  *  Correction is to check if the file path and temp path are the same and if so, rename the temp path.  As it is temp, we can name it anything we want.  I append a _2 to the end.
MrBrianGale added a commit to MrBrianGale/remoteapptool that referenced this issue May 20, 2020
Fixes Issue#3
  *  After some testing and fiddling with things, there is a case that can occur where the rdpTempPath and rdpFilePath can be the same.  This results in the file lock check saying the file is NOT locked (as it is not at that point) BUT the application still throwing a JIT error
  *  Correction is to check if the file path and temp path are the same and if so, rename the temp path.  As it is temp, we can name it anything we want.  I append a _2 to the end.
@MrBrianGale
Copy link
Collaborator

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

MrBrianGale added a commit to MrBrianGale/remoteapptool that referenced this issue Jun 19, 2020
Fixes Issue#3
  *  After some testing and fiddling with things, there is a case that can occur where the rdpTempPath and rdpFilePath can be the same.  This results in the file lock check saying the file is NOT locked (as it is not at that point) BUT the application still throwing a JIT error
  *  Correction is to check if the file path and temp path are the same and if so, rename the temp path.  As it is temp, we can name it anything we want.  I append a _2 to the end.
MrBrianGale added a commit to MrBrianGale/remoteapptool that referenced this issue Jun 19, 2020
Fixes Issue#3
  *  After some testing and fiddling with things, there is a case that can occur where the rdpTempPath and rdpFilePath can be the same.  This results in the file lock check saying the file is NOT locked (as it is not at that point) BUT the application still throwing a JIT error
  *  Correction is to check if the file path and temp path are the same and if so, rename the temp path.  As it is temp, we can name it anything we want.  I append a _2 to the end.
@MrBrianGale MrBrianGale self-assigned this Jun 26, 2020
MrBrianGale added a commit to MrBrianGale/remoteapptool that referenced this issue Jul 16, 2020
Fixes Issue#3
  *  After some testing and fiddling with things, there is a case that can occur where the rdpTempPath and rdpFilePath can be the same.  This results in the file lock check saying the file is NOT locked (as it is not at that point) BUT the application still throwing a JIT error
  *  Correction is to check if the file path and temp path are the same and if so, rename the temp path.  As it is temp, we can name it anything we want.  I append a _2 to the end.
@ksuviper
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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 added a commit to MrBrianGale/remoteapptool that referenced this issue Jul 24, 2020
Fixes Issue kimmknight#3
This commit fixes issue kimmknight#3 by checking for the file lock on the source and destination files.  If a lock is found, it asks the user what they want to do.  After it is determined that no locks exist, it attempts a file copy.  If the file copy fails, it renames the file to append _NEW onto the name and tries the copy a second time.  If it fails the second time, it gives up but lets the user know to put in an issue on github and what information to provide.
With my quick testing, it appears to work when the destination file is locked to rename it.
The renamed version of the file is passed back to the calling function along with a success value.

This should correct issue kimmknight#3 or at least give end users a chance to fix things on their own (delete the locked file manually).
Also fixed a typo in the lock checking loop.  Not entirely sure how it wasn't stuck in an infinite loop previously when checking for locks.
@MrBrianGale
Copy link
Collaborator

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
Copy link

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

@kimmknight
Copy link
Owner Author

@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
Copy link

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
Copy link
Collaborator

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants