Skip to content

Commit

Permalink
- fixed potential access to freed memory on map loading
Browse files Browse the repository at this point in the history
MapData could destruct FResourceLump objects before accessing them
Loading of map .wad from .pk3 file is example of this case

https://forum.zdoom.org/viewtopic.php?t=60972
  • Loading branch information
alexey-lysiuk committed Jun 22, 2018
1 parent eddb179 commit 9b4e8ef
Showing 1 changed file with 19 additions and 7 deletions.
26 changes: 19 additions & 7 deletions src/p_setup.h
Expand Up @@ -36,6 +36,25 @@
struct MapData
{
private:
struct ResourceHolder
{
FResourceFile *data = nullptr;

~ResourceHolder()
{
delete data;
}

ResourceHolder &operator=(FResourceFile *other) { data = other; return *this; }
FResourceFile *operator->() { return data; }
operator FResourceFile *() const { return data; }
};

// The order of members here is important
// Resource should be destructed after MapLumps as readers may share FResourceLump objects
// For example, this is the case when map .wad is loaded from .pk3 file
ResourceHolder resource;

struct MapLump
{
char Name[8] = { 0 };
Expand All @@ -48,13 +67,6 @@ struct MapData
bool isText = false;
bool InWad = false;
int lumpnum = -1;
FResourceFile * resource = nullptr;

~MapData()
{
if (resource != nullptr) delete resource;
resource = nullptr;
}

/*
void Seek(unsigned int lumpindex)
Expand Down

2 comments on commit 9b4e8ef

@coelckers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you didn't use a std::unique_ptr for this?

@alexey-lysiuk
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it's too many changes for nothing. To make it right and future-proof, FResourceFile should be owned by std::shared_ptr everywhere. I'm not sure that we really need this.

Please sign in to comment.