"invalid null pointer" exception downloading mission list in Win32 Debug build

[Zunzster]

I don't know whether this truly qualifies as a bug, maybe it's more of a gotcha, a difference of assumptions between pugixpath and the VC++ Debug STL implementation.

 

When compiling gamex86 as a debug build, downloading the mission list will cause a "invalid null pointer" exception.

 

The problem is triggered by the following line in MissionManager.cpp in the CMissionManager::LoadModListFromXml method.

 

// Localisation packs

pugi::xpath_node_set l10PackNodes = node.select_nodes("localisationPack");

 

This produces an empty xml node set from the query which it attempts to return via xpath_node_set::operator=()

 

This routine sets m_begin and m_end to NULL before falling through and calling into xpath_node_set::append() with an empty set.

 

m_begin = m_end = m_eos = 0;

m_type = ns.m_type;

 

if (ns.size() == 1)

{

m_storage = *ns.m_begin;

m_begin = &m_storage;

m_end = m_eos = &m_storage + 1;

}

else

{

append(ns.begin(), ns.end());

}

 

This is arguably a bit dubious since if you look at the xpath_node_set constructor, m_begin and m_end are not NULL there but point within the m_storage single static node.

 

The problem with them being NULL is that the Debug version of std::copy() in the VC library asserts that it's destination is not NULL even when the source iterators imply an empty collection.

 

Changing xpath_node_set::operator=() to the following avoids the issue:

 

m_begin = m_end = &m_storage; m_eos = &m_storage + 1;

m_type = ns.m_type;

 

if (ns.size() == 1)

{

m_storage = *ns.m_begin;

m_end = &m_storage + 1;

}

else

{

append(ns.begin(), ns.end());

}

 

As far as I can tell, the documentation on std::copy is silent on whether NULL is a valid destination iterator when the source range is empty.

 

Given that pugixpath is an included package, maybe passing this issue upstream is a better course.

Edited January 15, 2012 by Zunzster

[greebo]

Yup, I fixed this one sometime last month, it's this patch:

 

Index: pugixpath.cpp

===================================================================

--- pugixpath.cpp (revision 5076)

+++ pugixpath.cpp (revision 5077)

@@ -784,8 +784,11 @@

m_eos = storage + capacity;

}

 

- std::copy(begin, end, m_end);

- m_end += count;

+ if (m_end != NULL)

+ {

+ std::copy(begin, end, m_end);

+ m_end += count;

+ }

}

 

void xpath_node_set::truncate(iterator it)

 

I immediately ran into this when switching to VC++ 2010, since that version added the assertion to the std::copy implementation. I don't know if the C++ standard allows, disallows or even requires this.

[Zunzster]

From a brief bit of googling, the standard appears to be silent on this issue so I'd say it's implementation defined whether passing NULL to std::copy is allowed. For anything other than an empty source, it will fault so I think what VC++ is doing is perfectly defensible and what pugixpath is doing is very dubious.

 

Given that m_end is never NULL other than due to that specific line in operator=(), it is essentially breaking it's own invariant IMHO. A minor bit of bad design that happens to survive in practice most of the time only to leap up and bite innocent parties down the road :-)

 

I've logged an issue with pugixpath on googlecode.

http://code.google.c...s/detail?id=143

 

Maybe they'll agree and maybe they won't :-)

Edited January 15, 2012 by Zunzster

[Zunzster]

OK. It turns out this is a known issue and is fixed in pugixpath version 1.0 - TDM is currently using version 0.5.

Given the version drift and potential interface and/or behaviour changes, I'm not sure if it's worth the churn of upgrading given that it's only a few lines to patch.

[greebo]

Well, yes it's only a few files, so I'll try to plug it into our codebase and see if any interfaces were changed.

 

edit: it was really easy, no interface changes so far, I've commit this to darkmod_src.