Patch #984

Fix 'failed to rename file' problem caused by virus scanner in windows

Added by Axel Waggershauser almost 3 years ago. Updated about 2 years ago.

Status:ClosedStart date:27 Aug 2014
Priority:NormalDue date:
Assignee:Robin Mills% Done:

100%

Category:basicio
Target version:0.25

Description

exiv2 fails regularly under windows to update a target file when that file is simultaneously checked by a virus scanner or indexer. The problem happens when an other process opens the target file with FILE_SHARE_DELETE. The 'replace the original with the temp file'-part inside FileIo::transfer() fails because the remove() of the original does not actually remove the file if that is opened with FILE_SHARE_DELETE and hence the subsequent rename() fails.

There is some more background to the problem here: http://stackoverflow.com/a/11023068.

The solution is to use the special purpose Win32 API function ReplaceFile(). The attached patch does that.

Attached is also a small sample program that does what a virus scanner typically does and helps see the problem: call the helper program with a jpg filename and try to modify the same file with exiv2.

Might also be related to http://dev.exiv2.org/boards/3/topics/502 and http://dev.exiv2.org/boards/3/topics/880.

ReplaceFile.patch Magnifier (2.6 KB) Axel Waggershauser, 27 Aug 2014 08:06

main.c Magnifier - sample code to demonstrate the problem (471 Bytes) Axel Waggershauser, 27 Aug 2014 08:06

ReplaceFile_2.patch Magnifier (4.49 KB) Thomas Beutlich, 04 Sep 2014 05:01

ReplaceFile_3.patch Magnifier (1.3 KB) Thomas Beutlich, 04 Sep 2014 05:33


Related issues

Related to Exiv2 - Bug #979: Temporary File Rename Issue on Windows/MSVC Closed 12 Aug 2014

Associated revisions

Revision 3345
Added by Robin Mills almost 3 years ago

Issue: #984. Thank You, Axel for reporting this, the patch, and the test code.

Revision 3349
Added by Robin Mills almost 3 years ago

Issue: #984. Thank You, Thomas for the patch.

History

#1 Updated by Robin Mills almost 3 years ago

  • Status changed from New to Assigned
  • Assignee set to Robin Mills
  • Target version set to 0.25

Thanks very much for reporting this. Very interesting problem and solution. Thanks for the effort you have put into this and thank you for posting a solution and patch.

I'll have a look at this and if it passes our test suite, I'll submit the code. I particularly appreciate your main.c code to recreate the system conflict which triggers this issue. Several of our sample programs are used in our test suite. This issue creates a new situation when we need an external program that doesn't use the exiv2 library. I will think about build your main.c and calling it from the test suite.

#2 Updated by Robin Mills almost 3 years ago

Thanks Axel for reporting this, the patch and the test code. Submitted r:3345

Two comments:
1) I had to modify the patch to deal with older versions of MSVC (example 2005) which does not provide the symbol REPLACEFILE_IGNORE_MERGE_ERRORS
2) I have added your test code to msvc2005/tools/issue984

I hope, but don't promise, to add main.c as a prebuilt binary issue984.exe. When this is done, I'll call it from bugfixes-test.sh on Windows. I will defer marking this bug as "resolved" until I do this work.

#3 Updated by Axel Waggershauser almost 3 years ago

Two comments:
1) I had to modify the patch to deal with older versions of MSVC (example 2005) which does not provide the symbol

I don't think this parameter is really necessary for the patch to have it's intended effect. I just read through the documentation and thought this should not hurt. But as long as the next published binary release is compiled with a newer version, I don't mind this exclusion of MSVC 2005.

2) I have added your test code to msvc2005/tools/issue984

You would very likely want to tune the sleep(10) call in an automated test setting. Maybe use something more appropriate than a sleep.

Thanks for applying the patch.

#4 Updated by Robin Mills almost 3 years ago

Axel

We support MSVC 2003/5/8/10/12 and intend to support 2013 for v0.25. So, although it's not necessary to change your patch for more recent versions of Visual Studio, I don't want to break support for earlier compilers without a compelling reason.

I've renamed main.c as issue984.cpp, built it with MSVC2005 and submitted issue984.exe. r3348.

For sure, the interaction of issue984.exe with Exiv2 is an interesting puzzle. It will be fun to play with that and add something to our test harness. However, it's not essential to do this, so regrettably, I can't spend more time on this at the moment. I hope we'll be "code complete" for v0.25 by the end of September and to return to this in October.

Robin

#5 Updated by Thomas Beutlich almost 3 years ago

I see two new problems with ReplaceFile.
  1. ReplaceFile is only available on WinXP or newer. See access of function GetFileInformationByHandle in the same file for a general solution.
  2. If the original file (lpReplacedFileName) is no longer available than ReplaceFile returns 0 with last error code set to ERROR_FILE_NOT_FOUND. This currently raises an exception
    C:\test\DSC00583.JPG3320: Failed to rename file to C:\test\DSC00583.JPG: No error (errno = 0)
    
    but was handled by a simple rename in the old implementation.
    The attached patch fixes both issues.

#6 Updated by Robin Mills almost 3 years ago

Thanks Thomas. That's very interesting. Thank you for dealing with that. I'm submitted your patch. r3349.

#7 Updated by Thomas Beutlich almost 3 years ago

  • Not sure if \msvc2005\tools\issue984\ is the right place for such test/debugging problems.
  • There is a little typo in issue984.cpp: ac*c*ess
  • Instead of having the exe file in the svn I would rather see the vc(x)proj file.

#8 Updated by Thomas Beutlich almost 3 years ago

Please call FreeLibrary(hKernel); before exceptions are raised. ANSI case is OK, but Unicode case is not. See attached patch.

#9 Updated by Axel Waggershauser almost 3 years ago

Hi Thomas, thanks for double checking my patch.

I knew that ReplaceFile was introduced at some point in time (after Windows 1.0 ;)) but did not know the project was still targeting older versions than XP.

As for the unexpected removal of the original file: is there any way for that to happen, except for an external process removing the file right while exiv2 is working on it? And if that process is e.g. the explorer, because the user wanted that file to be gone, wouldn't it be better then to have exiv2 not make that file reappear when it has just been deliberately deleted? (For my use case, I don't mind your patch at all, just asking...)

#10 Updated by Robin Mills almost 3 years ago

Thomas. You are quite right. My error. I should not have changed your code. I apologise. I've restored it to the state in ReplaceFile_2.patch. r3350.

Very good team work here, guys. That's what open source is all about. Thank You Thomas and Axel. Good Stuff.

#11 Updated by Thomas Beutlich almost 3 years ago

Hi Axel,

actually I am not sure what the minimum OS requirement for exiv2 is but I could remember the GetFileInformationByHandle patch. So it is now there for sake of consistency.

It could also be the AV tool that deleted the original file while exiv2 is working on it. Not sure if the rename then still is preferable but at least you do not want to see that silly exception message.

Regards,
Thomas

#12 Updated by Robin Mills almost 3 years ago

Thomas

I put the test code into tools/issue984 because I put the depends32/64 code there a few years ago. I'm not sure I want to put external programs such as issue984.cpp into the build and with only a single file to compile and link, a vcproj file isn't really needed when it can be build with > nmake issue984.exe And it's probably time to retire depends32/64 as I no longer use it in the test suite.

I'm really impressed with your attention to detail. If you'd like to contribute to Exiv2, I'd be delighted to welcome you to the team. You're welcome to email me directly to discuss this off-line

Robin

#13 Updated by Thomas Beutlich almost 3 years ago

Robin,

I built issue984.cpp in VS to have it in debugger with break point before CloseHandle(). That is the reason why I did not need the exe file but the VS project file. Never mind.

Not sure if I can contribute that much at all. I have this little [[http://www.totalcmd.net/plugring/iptcwdx.html TC content plugin]] running for some years and basically that's it.

Regards,
Thomas

#14 Updated by Robin Mills almost 3 years ago

Thomas

I am quite certain that you could make a very valuable contribution to Exiv2 with your MSVC skills. I am the only member of the team who deals with this and I have rather a lot of work at the moment for the v0.25 release. It's a good thing that I am retired, because I probably spend 20-30 hours every week working on the project. Here's the plan: http://dev.exiv2.org/boards/3/topics/1765

Specifically, about issue984.exe, you'll see it does a Sleep(10); I'd like to replace that with executing system("exiv2 ...."), or execvp. The program arguments would be
issue984.exe file exiv2 bla bla. And of course we have to update the test harness to use this stuff. In all honesty, I doubt if I will find the time to deal with this.

For the v0.25 release, I am currently integrating the GSoC2013 code to trunk and implementing a Jenkins build server which builds/tests when code is submitted. This will be a big time saver when it's complete. Jenkins currently builds with MSVC 2005/Mac/Linux/Cygwin. Nehal is working on MinGW. And between us we have 2008/10/12/13 to add. http://exiv2.dyndns.org:8080

So there's lots to be done and another Windows Engineer would be a very welcome addition to the team.

Robin

#15 Updated by Robin Mills almost 3 years ago

  • Status changed from Assigned to Resolved

I'm going to mark this as resolved.

#16 Updated by Robin Mills about 2 years ago

  • % Done changed from 0 to 100

#17 Updated by Andreas Huggel about 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF

Redmine Appliance - Powered by TurnKey Linux