BUG: MFC 7.0's CByteArray::Serialize() reads too much data

Introduction

In the recently-released version of Visual Studio.NET, there is a bug in MFC's new implementation of CByteArray::Serialize(). The function has been enhanced from the previous version so that it can read more than INT_MAX bytes. This is great, but apparently a line of code was mistakenly copied and pasted in the portion of the code that loads archives. The result is that CByteArray::Serialize() reads twice as much data from the archive as it's supposed to.

This article shows one way to work around the problem until Microsoft can correct it. I've searched the Knowledge Base and found no mention of it, so I hope Microsoft is aware of the problem.

Current Implementation

This is the current (buggy) implementation of CByteArray::Serialize from MFC's array_b.cpp. In the source below, note the extra ar.Read(pbData, nBytesToRead) statement in bold.

void CByteArray::Serialize(CArchive& ar)
{
	UINT_PTR nBytesLeft;
	UINT nBytesToWrite;
	UINT nBytesToRead;
	LPBYTE pbData;

	ASSERT_VALID(this);

	CObject::Serialize(ar);

	if (ar.IsStoring())
	{
		ar.WriteCount(m_nSize);
		nBytesLeft = m_nSize*sizeof(BYTE);
		pbData = m_pData;
		while(nBytesLeft > 0)
		{
			nBytesToWrite = UINT(min(nBytesLeft, INT_MAX));
			ar.Write(pbData, nBytesToWrite);
			pbData += nBytesToWrite;
			nBytesLeft -= nBytesToWrite;
		}
	}
	else
	{
		DWORD_PTR nOldSize = ar.ReadCount();
		SetSize(nOldSize);
		nBytesLeft = m_nSize*sizeof(BYTE);
		pbData = m_pData;
		while(nBytesLeft > 0)
		{
			nBytesToRead = UINT(min(nBytesLeft, INT_MAX));
			ar.Read(pbData, nBytesToRead);
			pbData += nBytesToRead;
			nBytesLeft -= nBytesToRead;
			ar.Read(pbData, nBytesToRead);
		}
	}
}

Effects

This has disastrous effects on any application that reads a CByteArray from an archive. In my case, I upgraded my Alarm++ software from VC++ 6.0 to VC++.NET and found the application was crashing in very unusual places. I tracked it down to getting bogus data from an archive, but the real culprit was an earlier CByteArray.Serialize() call that read too far into the archive and caused all the remaining objects to be garbage.

Workaround

One solution is to delete that line and rebuild the MFC libraries. I'm a bit leery of that solution because who knows what else might be introduced by changing the released MFC libraries. Plus, it would introduce the whole issue of shipping your own version of the MFC DLLs (if you use them, which I don't, so maybe I'll consider this solution later).

For the moment, I prefer to fix it in my own code, wait for a service release to fix it, and then take it out. Fortunately, Serialize() is virtual so we can derive our own class from CByteArray and use it instead. Of course, CByteArray is used internally in MFC, so the problem may turn up in other places as well (another reason to re-compile the MFC libraries).

class MyCByteArray : public CByteArray 
{
public:
	MyCByteArray() {}
	virtual ~MyCByteArray() {}

	virtual void Serialize(CArchive& ar)
	{
		UINT_PTR nBytesLeft;
		UINT nBytesToRead;
		LPBYTE pbData;

		ASSERT_VALID(this);

		CObject::Serialize(ar);

		if (ar.IsStoring())
		{
			// We can just call the base implementation here.
			CByteArray::Serialize(ar);
		}
		else
		{
			DWORD_PTR nOldSize = ar.ReadCount();
			SetSize(nOldSize);
			nBytesLeft = m_nSize*sizeof(BYTE);
			pbData = m_pData;
			while(nBytesLeft > 0)
			{
				nBytesToRead = UINT(min(nBytesLeft, INT_MAX));
				ar.Read(pbData, nBytesToRead);
				pbData += nBytesToRead;
				nBytesLeft -= nBytesToRead;
				//BUG: ar.Read(pbData, nBytesToRead);
			}
		}
	}
};

Example

I didn't think this was worth posting a whole sample project for, but here's a small code snippet to show you that it can be used the same way as CByteArry.

// create a byte array
MyCByteArray bytes;
for (int i = 0; i < 5; ++i)
   bytes.Add(i);

// write out the byte array
CFile f("test.ar", CFile::modeCreate | CFile::typeBinary | CFile::modeWrite);
CArchive arStore(&f, CArchive::store);
bytes.Serialize(arStore);
arStore.Close();
f.Close();

bytes.RemoveAll();   // clear the array

// read the array back
f.Open("test.ar", CFile::typeBinary | CFile::modeRead);
CArchive arLoad(&f, CArchive::load);
bytes.Serialize(arLoad);

// display it
for (int i = 0; i < bytes.GetSize(); ++i)
   cout << "Byte[" << i << "] = " << hex << (int) bytes[i] << dec << endl;

Conclusion

If your code serializes CByteArray objects, you should be aware of this problem in Visual Studio.NET MFC 7.0's implementation of CByteArray::Serialize().