Copy buffer

[h4ever]

I am trying to analyze somebody's source code because I would like to remove bug which happens when I try to copy item in tree view. When I try to (copy and) paste item in Debug configuration of project (VS) so it crashes. When I did it on release so it worked but the copied name of item contains some strange characters after the name itself. Here I got:

// Locks a global memory object (clip_data) 
// and returns a pointer to the first byte of the 
// object's memory block. This means that the
// the adress of the clipboard is copied to to trig_data
// There are no data in the variable yet.
trig_data = static_cast<char*>(GlobalLock(clip_data));
if (trig_data)
{
   try
   {
   // copy the trigger data from pointer to clipboard
   // (trig_data) to trigger object t.
   Trigger t(MemBuffer(trig_data, clip_size));
   // insert the trigger object to scen.triggers
   index = scen.insert_trigger(&t, index_sel);
   GlobalUnlock(clip_data);

   pasted = TrigTree_AddTrig(treeview, index, selected);

   TreeView_SelectItem(treeview, pasted);
	}
}

Notice: I have checked that the t is copied and the data seems to be OK to me. But I am not a pro, just beginner.

The MemBuffer.cpp:

#include "MemBuffer.h"

#include <stdexcept>

using std::out_of_range;

/* MemBuffer */

MemBuffer::MemBuffer(char *b, int l)
:	blen(l)
{
	buffer = b;
	pos = buffer;
	mine = false;
}
/*
MemBuffer::MemBuffer(const char *b, int l)
:	blen(l)
{
}
*/
MemBuffer::MemBuffer(int l)
:	blen(l)
{
	buffer = new char[blen];
	pos = buffer;
	mine = true;
}

MemBuffer::~MemBuffer()
{
	if (mine)
		delete [] buffer;
}

void MemBuffer::read(void *dest, const size_t length)
{
	checkFits(length);

	memcpy(dest, pos, length);
	pos += length;
}

void MemBuffer::reads(char *dest, const size_t lensize)
{
	size_t len = 0;

	checkFits(lensize);

	memcpy(&len, pos, lensize);
	pos += lensize;

	checkFits(len);
	if (len)
		memcpy(dest, pos, len);
	else
		*dest = '\0';

	pos += len;
}

void MemBuffer::write(const void *source, const size_t length)
{
	checkFits(length);

	memcpy(pos, source, length);
	pos += length;
}

void MemBuffer::writes(const char *source, const size_t lensize)
{
	size_t len = strlen(source);

	checkFits(lensize + len);

	memcpy(pos, &len, lensize);
	pos += lensize;

	memcpy(pos, source, len);
	pos += len;
}

void MemBuffer::skip(size_t offset)
{
	checkFits(offset);

	pos += offset;
}

void MemBuffer::fill(int value, size_t length)
{
	checkFits(length);

	memset(pos, value, length);
	pos += length;
}

void MemBuffer::checkFits(size_t length) const
{
	if (pos + length > buffer + blen)
		throw out_of_range("not enough space in MemBuffer");
}

I would be glad if you tell me what function is called here:
MemBuffer(trig_data, clip_size)
and what exactly is happening here. Do you see some problem there in the MemBuffer what could cause the crash of the program? It crashes on line
index = scen.insert_trigger(&t, index_sel);
but I cannot see any problem in the scen and scen.triggers . index_sel is 0x00000015 seems fine.

[kbw]

There doesn't appear to be any methods to handle MemBuffer copies, but you copy it.

Has this code ever worked?

[h4ever]

In the release version It worked. It was possible to paste the trigger, but it has strange character on end of the name of trigger. I run debug version now.

I see memcpy there in read(), but this one is not called here.

If I understand it right, so this function is called:

MemBuffer::MemBuffer(char *b, int l)
:	blen(l)
{
	buffer = b;
	pos = buffer;
	mine = false;
}

There is this header:

#include "Buffer.h"

/* An implementation of Buffer that uses a statically-sized chunk of memory as
 * its backing store. */
class MemBuffer : public Buffer
{
public:
	MemBuffer(char *b, int l);
	MemBuffer(int l);
	~MemBuffer();

	void read(void *dest, size_t length);
	void reads(char *dest, size_t lensize);
	void skip(size_t offset);

	void write(const void *source, size_t length);
	void writes(const char *source, size_t lensize);
	void fill(int value, size_t length);


	const char *get() const	{ return buffer; }

private:
	bool mine;	//did i create memory?
	bool writeable;
	char *buffer;
	char *pos;
	int blen;

	// throws std::out_of_range if the specified length won't fit in the buffer
	void checkFits(size_t length) const;
};

char * b - points to 0x001611b0 . Po rozbalení vidím 0x00 (to nevím co je)
int l - 0x0000017c
buffer = b = pos
bool mine - false

MemBuffer is constructor, right? I guess because I don't see return type declared anywhere.

So correction, the MemBuffer does not make copy, it is the following command:

Trigger::Trigger(Buffer& buffer)
{
	/* We use std::vectors here for RAII. */
	using std::vector;

	int num;

	buffer.read(&state, 14);
	buffer.fill(0, sizeof(long));
	description.read(buffer, sizeof(long));
	buffer.reads(name, sizeof(long));

	/* effects */

	buffer.read(&num, sizeof(long));
	vector<Effect> effects;				// temporarily holds Effects before we get to order info
	effects.reserve(num);

	for (int i = 0; i < num; i++)
		effects.push_back(Effect(buffer));

	while (num--)
	{
		long index;
		buffer.read(&index, sizeof(index));
		this->effects.push_back(effects[index]);
	}

	effects.clear(); // free the memory early

	/* conditions */

	buffer.read(&num, sizeof(long));
	vector<Condition> conditions;
	conditions.reserve(num);

	for (int i = 0; i < num; i++)
		conditions.push_back(Condition(buffer));

	while (num--)
	{
		long index;
		buffer.read(&index, sizeof(index));
		this->conds.push_back(conditions[index]);
	}
}

void Trigger::read(FILE *in)
{
	using std::vector;

	long n_effects, n_conds;

	// TODO: read in as struct
	readbin(in, &state);
	readbin(in, &loop);
	readbin(in, &u1);
	readbin(in, &obj);
	readbin(in, &obj_order);
	readunk<long>(in, 0, "trigger zeroes", true);

	description.read(in, sizeof(long));
	readcs<unsigned long>(in, name, sizeof(name));

	//read effects
	readbin(in, &n_effects);

	if (n_effects > 0)
	{
		// construct them all so we can directly call read()
		vector<Effect> effects(n_effects);

		for (int i = 0; i < n_effects; i++)
		{
			try
			{
				effects[i].read(in);
			}
			catch (std::exception &)
			{
				printf("Effect %d invalid.\n", i);
				throw;
			}
		}

		while (n_effects--)
		{
			long order = readval<long>(in);
			// I keep the effects in the proper order in memory, unlike AOK.
			this->effects.push_back(effects[order]);
		}
	}

	//read conditions
	readbin(in, &n_conds);

	if (n_conds > 0)
	{
		// construct them all so we can directly call read()
		vector<Condition> conditions(n_conds);

		for (int i = 0; i < n_conds; i++)
		{
			try
			{
				conditions[i].read(in);
			}
			catch (std::exception &)
			{
				printf("Condition %d invalid.\n", i);
				throw;
			}
		}

		while (n_conds--)
		{
			long order = readval<long>(in);
			conds.push_back(conditions[order]);
		}
	}
}

[h4ever]

The copy trigger command does this:

MemBuffer::read() -> calls checkFits(length) ... if (pos+length>buffer+blen)...
                              -> memcpy(dest,pos,length)
                              -> pos+=length
MemBuffer::fill() -> again checkFits
                          -> memset(pos,value,length)
                          -> pos+=length

[helios]

1. At line 11, how do you ensure that 'name' is big enough to hold the stored string?
2. reads() writes the null terminator when the length of the stored string is zero. If the length is not zero, does it include the null terminator? Maybe you should dest[len] = '\0'; regardless of the value of len.

[h4ever]

After the trigger is copied, there is command to append

index = scen.insert(&t, index_sel); // index_sel is 0x00000015
// it calls insert_trigger, which calls triggers.append(*t), which uses copy constructor _AC source(item) ... this will get me to Trigger::Trigger... strcpy,name.t.name) which gets me to SString::SString... put(s.data) and this one is the code bellow.

I will debug the trigger.append command of the Scenario::insert_trigger:

size_t Scenario::insert_trigger(Trigger *t, size_t after)
{
	size_t tindex = triggers.append(*t);

	// TODO: does the -1 work?
	if (after == -1)
	{
		t_order.push_back(tindex);
	}
	else
	{
		// Find the /after/ value in t_order.
		vector<unsigned long>::iterator where =
			std::find(t_order.begin(), t_order.end(), after);

		// If the /after/ value was actually there, insert after it.
		// (Otherwise, we'll insert at the end, i.e. append.)
		if (where != t_order.end())
			++where;

		t_order.insert(where, tindex);
	}

	return tindex;
}

datatypes.h

int append(const _AC &item)
{
	_AC source(item);	//needs one away from main list
	if (end <= next)
		expand();
	new (next++) _AC(source);
	return count() - 1;
}

trigger.cpp

Trigger::Trigger(const Trigger &t) // TODO: we can use the compiler's version
:	state(t.state), loop(t.loop), u1(t.u1), obj(t.obj), obj_order(t.obj_order),
	description(t.description), effects(t.effects), conds(t.conds)
{
	strcpy(name, t.name);
}

datatypes.cpp

SString::SString(const SString &s)
:	len(s.len)
{
	put(s.data);
}

datatypes.cpp:

void SString::put(const char *d)
{
     if (d)
	{
		data = new char[len + 1];
		strncpy(data, d, len);
		data[len] = '\0';
	}
     else
	data = NULL;
}

HERE I FOUND THIS:
pointer d is 0, wrong pointer, hence data is set to NULL
then strcpy in Trigger::Trigger calls vector (VS inner call)

THEN I continue to debug from datatype.h the int append(const _AC &item) function:
expand() is skipped
new (next++) _AC(source);
will crash. It crashes at Trigger::Trigger constructor at the line
strcpy(name, t.name);
strcpy calls SString::SString as above:

SString::SString(const SString &s)
:	len(s.len)
{
	put(s.data);
}

(s is the null pointer), put sets data to null pointer. Then second time strcpy is called, vector (VS inner function) is called and then it crashes on chkstk.asm line 98:

; Find next lower page and probe
cs20:
        sub     eax, _PAGESIZE_         ; decrease by PAGESIZE
        test    dword ptr [eax],eax     ; probe page.
        jmp     short cs10

the yellow arrow ended on line with word "test"

[h4ever]

RE: Helios:
Line 11 (bufffer.reads(name,size(long))); does this:

void MemBuffer::reads(char *dest, const size_t lensize)
{
	size_t len = 0;

	checkFits(lensize);

	memcpy(&len, pos, lensize);
	pos += lensize;

	checkFits(len);
	if (len)
		memcpy(dest, pos, len);
	else
		*dest = '\0';

	pos += len;
}

lensize is 0x00000004
checkFits passes fine
IMPORTANT:
after pos += lensize; pos is 0x001612a2 " dea BLACK{?}"
replace {?} for strange character, probably \r or \n . This is the old bug but I think this should not cause crash because in release configuration it works, just the strange character is pasted to the tree, so the item name in the view tree looks like " dea BLACK?" rectange instead "?"

before memcpy(&len, pos, lensize); is called, it contains these values:
len - 0x00000000
pos - 0x00161176 I can expand it and it shows: 0x0a (what's that?)
lensize - 0x00000004

as a result len is 0x0000000a (decimal 10), condition if (len) is true, memcpy(dest,pos,len) is performed. Then dest contains characters: " dea BLACKĚ....." Strange is that in the tree the items shows more characters after the item is copy/pasted. Looks like there should be incorrect len, but it is 10 and it is OK.

Finally pos+=len passes pos pointer to 0x00161184 and there is the strange char - it is the rectangle shape character. This is the only one character displayed by VS.

edit, now I got it. The strange character value is 0x03

CONCLUSION:
What reads did is: read 4 bytes of integer (size of the string name), shift pos += 4 bytes, checkFits, copy the string " dea BLACK" (10 characters total).

However why the dest contains more then 10 characters? There are Ě characters redundant showen in the string.

Maybe is there problem, that the author of the code forgot to paste null character on the end of name? This results that the name is not correct when the reads() returned. The len of the string is lost because its local variable of the reads(). Yet a note: name is private property of Trigger, limit to 128 chars.

[h4ever]

Wow, looks I have corrected it from crashing! However the bug with incorrect name of the trigger after paste is still there. Image:

http://oi59.tinypic.com/1zfh9fo.jpg

Anybody wants to test my project in VS?

What I have done is that I returned the length of the string which was copied (read):

size_t name_len = buffer.reads(name, sizeof(long));
if (name_len < MAX_TRIGNAME)
		name[name_len+1]='\0';

I had to change return type of the function.

EDIT:
Hey I have corrected it! I am so happy! I did not realized it why it is, but now I know

size_t name_len = buffer.reads(name, sizeof(long));
if (name_len < MAX_TRIGNAME)
		name[name_len]='\0';

that the index is counted from 0 so not +1