Page MenuHomeWildfire Games

Prevent CXeromyces ConvertFile crash on Win10 with G Data Internet Security
Needs RevisionPublic

Authored by LukeV1 on Jan 29 2018, 3:15 PM.

Details

Reviewers
Itms
Imarok
elexis
Trac Tickets
#4802
Summary

This is my first revision, please don't blame me if I missed some style guidelines..

As reported by at least myself (heard rumors more people have that problem) pyrogenesis is crashing without any error message on some Win10 machines which have G Data Internet Security installed.
Forum thread
This doesn't prevent the error message at startup which is also described in the forum post. Its just a solution for the crash. This also means that #4847 is unrelated to this.

This patch isn't actually fully preventing the appcrash, but logging it properly (preventing would probably introduce some unsightly code which would be rarely used as only the svn version is affected)

Test Plan
  • start pyrogenesis.exe
  • if necessary start a new game
  • check that pyrogenesis isn't crashing without an error log entry

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 8730
Build 14303: Vulcan BuildJenkins

Event Timeline

LukeV1 created this revision.Jan 29 2018, 3:15 PM
LukeV1 edited the summary of this revision. (Show Details)
elexis added a subscriber: elexis.Jan 29 2018, 6:40 PM

We could save the return value of SetEndOfFile(hFile) in a variable and show a human-readable string if it failed.
This way people won't create new tickets with this hard to resolve issue.
There are some similar occurences in the code already, the status should be INFO::OK and one can print a LOGERROR("blabla") before doing that RETURN_STATUS_IF_ERR.

To clarify, you're saying it loads all files successfully despite the anti-virus scanner?
In that case displaying a list of failed files using LOGERROR or LOGWARNING would indeed be preferable over a scarly ENSURE breakpoint.
I assume it only tries to load the files once and then uses some blank fallback (for instacnce for textures and models?)
(With the filenames we could find out)

In D1274#51517, @elexis wrote:

To clarify, you're saying it loads all files successfully despite the anti-virus scanner?

Yes, it seems so. But something is going wrong with the the "old" error handling for the handle, because the process dies spontaneously somewhere in the WARN_IF_FALSE-makro - when the anti-virus scanner is active.
With the macro RETURN_STATUS_IF_ERR however it is fully working...

In D1274#51517, @elexis wrote:

I assume it only tries to load the files once and then uses some blank fallback (for instance for textures and models?)

I'll check which files are failing, but if I remember correctly it crashed on *.xml files...

I must admit that I never worked with handles at all, I'm just experimenting. Actually all of this is way above my knowledge :P

Maybe WARN_IF_ERR rather than WARN_IF_FALSE was intended to be used? The former only tests for status 0, the latter againts everything smaller than 0.

In source/lib/status.cpp we see the possible Status types. INFO::OK is actually 0 and the errors are all negative.

I wouldn't be surprised if the SetFilePointerEx statement one line prior already returns an uncaught error.

LukeV1 updated this revision to Diff 5610.Jan 31 2018, 9:40 PM

Diff update: Using WARN_IF_ERR instead of RETURN_STATUS_IF_ERR as suggested by elexis

LukeV1 added a comment.EditedJan 31 2018, 10:01 PM

Thanks for pointing me to status.cpp - now i've realized the status is just a 64 bit signed int. Interestingly enough the status isn't negative but plus 1 most of the time (and sometimes - quite rarely - it is actually zero). I've used debug_prtinf with %I64d for output what should be working?!
To be more specific: The status of SetFilePointerEx and SetEndOfFile are 1 most of the times, but while WARN_IF_FALSE(SetFilePointerEx( don't cause a crash WARN_IF_FALSE(SetEndOfFile( does. Replacing this line with the diff or just entirely removing the Macro (which doesn't sound that good) avoids crashing and the game is working then.

I have also checked for a specific filetype where the status gets non-zero: there is no such type, its randomly happening with xml, dae, png, cfg
EDIT: It crashes on random xml files.

elexis added a comment.Feb 4 2018, 6:48 PM

Those functions are booleans though, not returning a numeric Status, so +1 makes sense as that is true (according to §7.16 from C99 supposedly).
https://msdn.microsoft.com/en-US/library/windows/desktop/aa365542(v=vs.85).aspx
https://msdn.microsoft.com/en-US/library/windows/desktop/ms724211(v=vs.85).aspx
So WARN_IF_ERR must be baloney and we should just do the appropriate thing when we receive false.

This doesn't prevent the error message at startup which is also described in the forum post. Its just a solution for the crash. This also means that #4847 is unrelated to this.

You mean the ticket is unrelated because it's impossible to change the anti-virus software from letting us load the file properly (and just skipping the file not being an option in case of simulation templates)?

This function might not want to throw a debug warning under any circumstance, but rather return the result back to the caller and let the caller decide if and how it's able to cope with the file situation.
If a file loading failed, there's likely not much we can do in terms of correcting that.
But in this case a retry might do already? (Could be attempted with for try = 0 to 5 loop inside the function or in the caller of the function. Not sure if we need to go down that route if we have the human-readable warning.)

Showing the user a "could not load file X/Y" would help more than some technical term that only means something to developers after some research.
Then he might start looking for the anti-virus software, hardware or file-permission issues.
It would enable us to close the tickets with this callstack as "won't fix" more quickly.

At last, we update programming.json with every new contributor. You can include it in the patch if you want a specific name there.

LukeV1 added a comment.Feb 6 2018, 6:05 PM

Ok, now this makes sense, thanks for the links. And yes, my diff is absolutely useless because I just ignore the error now...

In D1274#52168, @elexis wrote:

You mean the ticket is unrelated because it's impossible to change the anti-virus software from letting us load the file properly (and just skipping the file not being an option in case of simulation templates)?

Umm, we are talking about different issues now. In the forum thread I mentioned two problems (here) which seemd related. But as far as I know, there aren't. This patch should only fix the 2. issue - the complete crash and not the Problem whith the stange callstack somwhere in HWDetect, to which #4847 is referring to.
Also the problem this diff is meant for does not cause a callstack at all. Pyrogenesis just stops working and that's it.

For now I will make an attempt if a retry would help and where to implement the warning message.

LukeV1 updated this revision to Diff 5709.EditedFeb 7 2018, 10:08 PM

Complete rework. Had to introduce a new macro to convert the "+1" error to a "-1" error.

Not sure which return value (const) to use in Xeromyces.cpp since the used one doesn't work that well (produces no stacktrace / nice info text in this case).

LukeV1 edited the test plan for this revision. (Show Details)Feb 8 2018, 9:06 PM
elexis added a comment.Feb 9 2018, 1:24 PM

According to the docs, the win functions only return 0 or -1 as far as I see, so RETURN_MINUS_1_IF_FALSE should be the same as RETURN_STATUS_IF_ERR and that is already used for once truncate call.

Notice the linux truncate function also returns -1 upon error and 0 on success, that is passed on, whereas the windows function only fails and returns success in the current code!
Since our game doesn't fall apart when running linux, it should be safe to correct the windows return value. Quite an important find, even if we can't optimize the failure behavior of every file operation.

I think I have to commit this if it uses RETURN_MINUS_1_IF_FALSE and PSRETURN_File_WriteFailed and if you confirm that it works on windows.

source/ps/XML/Xeromyces.cpp
195

This hunk also looks good, XML errors are worse than textures, models or soundfiles not being loaded.

It's not really an XML parsing error but a file writing one, so maybe PSRETURN_File_WriteFailed or one of his brothers in source/ps/Errors.cpp fits better.

Thanks for digging out this place. (Especially important that you do it as you can test it first hand)

LukeV1 updated this revision to Diff 5739.Feb 9 2018, 8:24 PM

Changed the error return value.
Unfortunately I had to include FileIo.h to be able to use PSRETURN_File_WriteFailed. Couldn't define it in Xeromyces.h since FileIo.h and Xeromyces.h are included simultaneously in some other files which would then lead to a redefinition of ERROR_GROUP(File);.

The macro RETURN_MINUS_1_IF_FALSE is needed since the WINAPI returns a bool value, what means that non-zero is "1" and not "-1" (you did also mention this above...).

LukeV1 added a comment.EditedFeb 9 2018, 8:31 PM

Unfortunately I couldn't fully test this revision since I stumbled across a quite strange problem:
(it stopped at the breakpoint and I klicked on "step into" once)

Any clue why this happens?!
EDIT: The debugger fooled me - it's just a display error but is doing the right thing....

elexis added a comment.Feb 9 2018, 8:42 PM
In D1274#52769, @LukeV1 wrote:

Changed the error return value.
Unfortunately I had to include FileIo.h to be able to use PSRETURN_File_WriteFailed. Couldn't define it in Xeromyces.h since FileIo.h and Xeromyces.h are included simultaneously in some other files which would then lead to a redefinition of ERROR_GROUP(File);.

Sure about this? We have these things that allow multiple inclusions of header files

#ifndef INCLUDED_STATUS
#define INCLUDED_STATUS
...

#ifndef INCLUDED_XEROMYCES
#define INCLUDED_XEROMYCES

The macro RETURN_MINUS_1_IF_FALSE is needed since the WINAPI returns a bool value, what means that non-zero is "1" and not "-1" (you did also mention this above...).

Right, then we need it. Perhaps we should call it RETURN_ERROR_IF_FALSE? -1 ok for me too if you prefer it.

source/ps/XML/Xeromyces.cpp
193

. -> :

source/ps/XML/Xeromyces.h
32 ↗(On Diff #5739)

Which file? Errors.h?

LukeV1 added a comment.EditedFeb 10 2018, 4:16 PM
In D1274#52775, @elexis wrote:

Sure about this? We have these things that allow multiple inclusions of header files

...
#ifndef INCLUDED_XEROMYCES
#define INCLUDED_XEROMYCES

Yes, I know about these preprocessor constructs. But I think this will not work because ifndef can't be used for macros (since they contain a #define statement). I did some basic test but couldn't get it working.

Perhaps we should call it RETURN_ERROR_IF_FALSE? -1 ok for me too if you prefer it.

Yes, I will change it.

source/ps/XML/Xeromyces.h
32 ↗(On Diff #5739)

Yes. I will change the comment

LukeV1 updated this revision to Diff 5770.Feb 11 2018, 11:12 PM
LukeV1 edited the summary of this revision. (Show Details)

Changed comments and macro name. Works on Windows, logs error correctly.
In many cases pyrogenesis is still crashing afterwards because other components try to read non-existent files. Therefore passing the error to the caller isn't enough and we should keep the LOGERROR line there to preserve the crash information.

LukeV1 updated this revision to Diff 5771.Feb 11 2018, 11:14 PM
LukeV1 marked an inline comment as done.

Fixed typo

LukeV1 marked 2 inline comments as done.Feb 11 2018, 11:16 PM
LukeV1 added inline comments.
source/ps/XML/Xeromyces.cpp
193

What does . -> : mean?

Thanks for the patch.

Could you get errors codes/texts with GetLastError function?

source/lib/sysdep/os/win/wposix/wfilesystem.cpp
287

It'd be good to have exactly code and text of the error, i.e. with GetLastError (https://msdn.microsoft.com/ru-ru/library/windows/desktop/ms679360(v=vs.85).aspx).

source/ps/XML/Xeromyces.h
32 ↗(On Diff #5771)

If you don't use included functions in the header file, you need to move the include to the source file.

LukeV1 updated this revision to Diff 5875.Feb 22 2018, 5:14 PM

Moved the include statement. Using the win fuction GetLastError() isn't that easy in this context, see code comment

LukeV1 added inline comments.Feb 22 2018, 5:32 PM
source/lib/sysdep/os/win/wposix/wfilesystem.cpp
287

GetLastError returns a positive error code (1224 in this case ->"The requested operation cannot be performed on a file with a user-mapped section open.") what means that it will not be caught as an error by the caller.
What should I do with this error code (or the error text)? Should I return the negative win error (i. e. -1224) and catch it in Xeromyces, print a detailed LOGERROR message and cast it to PSRETURN_File_WriteFailed for further returns?

May I still use a macro then? Because it would be windows specific...

vladislavbelov added inline comments.Feb 22 2018, 5:49 PM
source/lib/sysdep/os/win/wposix/wfilesystem.cpp
287
287

The whole file is Windows specific, so I think it'd be good to have a detailed error for debug.

Vulcan added a subscriber: Vulcan.Feb 22 2018, 5:49 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/68/display/redirect

Probably we need write to authors of G Data, because it looks strange to prevent an application generates own files.

LukeV1 added a comment.EditedFeb 22 2018, 10:53 PM

Regarding the use of GetLastError() for a more detailed error info there are several issues that should be considered first:

  • The Macro I set up and used is in status.h which doesn't is OS-specific (or I am wrong?!). When inserting GetLastError() there, we will introduce a function that can only be used for windows. Quite bad to supply this as a macro I think. Not using a macro will introduce quite some clutter or a new private function in wfilesystem.h
  • The error code produced by GetLastError() isn't compatible to the error code numbering scheme we use. That means that this function will return an non-understandable error code when debugging
  • Should we try to use the api call FormatMessage to convert the error code to a text message or hardcode the error message?

Probably we need write to authors of G Data, because it looks strange to prevent an application generates own files.

Yes, also thought about that. G Data released some insight tool for performance leaks which I want to try out first before contacting them - maybe it shows something interesting...

LukeV1 updated this revision to Diff 9219.EditedSun, Aug 4, 2:40 PM
LukeV1 edited the summary of this revision. (Show Details)
LukeV1 added a project: Windows Developers.

Upload patch again with new master revision and full file context.

The G Data insight tool for performance leaks didn't showed anything interesting (except of the proove that it is acutally checking the generated files)...

Vulcan added a comment.Sun, Aug 4, 3:11 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/status.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/lib/status.h
| 393| namespace·INFO·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceINFO{' is invalid C code. Use --std or --language to configure the language.

source/lib/sysdep/os/win/wposix/wfilesystem.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/303/display/redirect

elexis requested changes to this revision.Mon, Aug 12, 10:42 PM
elexis added inline comments.
source/lib/status.h
366

The do while loop is placebo?

For the current behavior of the fucntion: According to FAIL = -1 and OK = 0, it is RETURN_FAIL_IF_NOT_OK, and -1 should be FAIL then.

On the other hand one may consider the enum numbers to be arbitrary, i .e. coincidental that it is 0. The return value of the microsoft function is not linked to INFO::OK, so it seems wrong to put this function here suggesting that it checks for a status code when it checks for a ms function return value.

Perhaps just using if !x() return or if !x() && !y() && !z()) return ... or return x() && y() && z(); would avoid this.

source/lib/sysdep/os/win/wposix/wfilesystem.cpp
287

The macro is called "RETURN_ERROR", but it doesn't return an error code, but it returns -1.
You add it in status.h, so it should be used for these status codes i.e.

const Status FAIL = -1

is what your patch uses here.

If you really want to catch that error, I suppose one could introduce a new status code to namespace ERR, and then introduce a separate library filesystem function that loads the most recent error, which calls just this function on windows, and perhaps does something else sane on unix.

It sounds like this could be done in a separate patch by the one who wants to work on that.

I'm fine with returning FAIL = -1 until then.

289

I suppore the return value of wtruncate is meant to reflect the result of the ms API calls? i.e. x && y && z would return the correct non-zero result (no matter whether its signed or unsigned).

On the contrary, the only call to wtruncate is RETURN_STATUS_IF_ERR(wtruncate, i.e. implying status.h values.

That those MS booleans are +1 but we need FAIL = -1 is another indicator as to why it might be better to separate those two layers explicitly.

So the remaining question now is whether the RETURN_STATUS_IF_ERR(wtruncate is making this confusion as well.

Because it's value is int, not Status. So either it should be a non-zero or not-non-zero int, or it should be a Status type, like caller function
Status Store().

The rest of wfilesystem.cpp returns -1 on error, which is consistent with your proposed return value.

The user like file.cpp Status FileOpen do

if(fd < 0)
		return StatusFromErrno();	// NOWARN

So if you wanted to transport the error somehow, setting errno might be another consideration.

I see a StatusFromWin too. And that also happens to return the thing Vladislav spoke of, GetLastError, in form of a Status!

So is that RETURN_STATUS_IF_ERR(wtruncate call right? To me it looks inconsistent.

That line comes from rP9350.

All StatusFromWin calls are in some wposix, waio files. wopendir in this file already catches it this way.

Knowingly returning a Status however also means that the return type should be Status, not int.

Comparing further the functions in this file, they seem to catch the ms error only to report it instantly and then forget about it again and return -1. That would explain the return type.

The other functions here return the ms return value.

So I suppose we now know precisely what to do in this function?
(And what to do to that RETURN_STATUS_IF_ERR(wtruncate)

source/ps/XML/Xeromyces.cpp
195

"load or save" is only "save", no?

This revision now requires changes to proceed.Mon, Aug 12, 10:42 PM
elexis retitled this revision from Prevent appcrash on Win10 with G Data Internet Security to Prevent CXeromyces ConvertFile crash on Win10 with G Data Internet Security.Mon, Aug 12, 10:44 PM
Stan added a subscriber: Stan.Sat, Aug 17, 11:19 AM

I pinged them on twitter. We'll see.