Don't crash when user selects wrong map format #1537

Closed
negke opened this Issue Dec 11, 2016 · 7 comments

Comments

Projects
None yet
3 participants
Contributor

negke commented Dec 11, 2016 edited by kduske

Sigh, okay.

Trying to open Shamblernaut's Retro jam5 map, current TB2 beta crashes due to the brush syntax. The map was saved with J.A.C.K.

Error message: Expected integer or decimal, but got '[' (raw data '[') [line 33, column 66]

Brush syntax looks like this:
( 3584 -4096 3840 ) ( 3584 -4096 -3840 ) ( 3584 3840 3840 ) SKY4 [ 0 -1 -0 0 ] [ 0 0 -1 0 ] 0 1 1

Crash report: trenchbroom-crash.txt

Owner

kduske commented Dec 11, 2016

Did you select "Valve" map format when you opened the file? Because it works for me when I do.

Contributor

negke commented Dec 11, 2016

D'oh. Yes, that works. I ignored that selection box, because I was under the impression it's only needed if the map has a .wcf or .rmf (or what it is) extension....
Still, I suppose it shouldn't outright crash if loading a file in the wrong format.

kduske changed the title from Crash when opening a Hammer-format map (JACK) to Don't crash when user selects wrong map format Dec 11, 2016

Owner

kduske commented Dec 11, 2016

Yeah, that's true.

kduske added this to the TrenchBroom 2.0.0 milestone Dec 11, 2016

kduske self-assigned this Dec 27, 2016

Owner

kduske commented Dec 27, 2016

@ericwa I don't have access to a Windows machine at the moment. Could you have a look at this?

kduske removed their assignment Dec 27, 2016

Collaborator

ericwa commented Dec 28, 2016

Sure. I tested with https://ci.appveyor.com/api/buildjobs/kll2h75ca7talv1s/artifacts/cmakebuild%2FTrenchBroom-Win32-2.0.0-Interim-0e1a90a-RelWithDebInfo.7z

It doesn't crash; I get the parse error dialog box, and then I opened the map again as valve, and it worked.

Autodetecting the format would be a nice improvement, but I guess this crash was fixed by some other change.

ericwa closed this Dec 28, 2016

ericwa removed the Status:Ready label Dec 28, 2016

Owner

kduske commented Dec 28, 2016

Same here. Thanks for checking!

@ericwa ericwa added a commit that referenced this issue Jan 3, 2017

@ericwa ericwa #1537: Fix ~MapFrame
Make virtual, and remove a redundant DestroyChildren() call
(one of the superclass destructors handles that)
723ae02
Collaborator

ericwa commented Jan 3, 2017

Hmm, now I can reproduce this on fd7b50e on a debug build on Windows, seems to be some kind of invalid read where MapFrame gets activated after its destructor runs.

ericwa reopened this Jan 3, 2017

ericwa self-assigned this Jan 3, 2017

@ericwa ericwa added a commit that referenced this issue Jan 3, 2017

@ericwa ericwa #1537: Make ~MapFrame virtual 725de14

@ericwa ericwa added a commit that referenced this issue Jan 3, 2017

@ericwa ericwa #1537: Call SendDestroyEvent() at the beginning of ~MapFrame
This makes IsBeingDeleted() return true.

The bug was that the DestroyChildren() call in ~MapFrame was causing
MapFrame::OnChildFocus() to be called, but the MapFrame was in a
partially invalid state (m_mapView pointer dangling since it was
already destroyed by DestroyChildren), but IsBeingDeleted() was not
returning true yet, so MapFrame still attempted to handle the OnChildFocus
c4eea45

kduske added the Status:Ready label Jan 3, 2017

kduske closed this Jan 3, 2017

kduske removed the Status:Ready label Jan 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment