string::size crash outside debugger environment

[linde12]

Hello! As the title says when i run my code the program crashes if not running in it through Microsoft Visual C++.

When i try to run the file normally(double-click) it prints my debugmessages up to "ReadElement5 result.size = " and there it crashes, it prints ReadElement5 result.size = but not the value of the string::size.

Im making a half-crappy Ini-write/read for my program... Here is the code:

//Ini.h
#include <iostream>
#include <Windows.h>
using namespace std;
class Ini {
	int cursor, eof, MAX_INI_LENGTH;
	string szIniData;
	FILE *fFile;
public:
	Ini();
	bool Select(char *parent);
	bool Read();
	void Clear();
	char *ReadElement(char *element);
	bool Write(char *element, char *value);
	bool WriteParent(char *parent);
	bool Open(char *file);
	bool Close();
};

//Ini.cpp
#include "Ini.h"


Ini::Ini(){
	MAX_INI_LENGTH = 1024*4;
	eof = 0;
	cursor = 0;
	//szIniData = "";
}
bool Ini::Open(char *file){
	if(fFile = fopen(file, "r+")){
		if(fFile != NULL){
			return true;
		}
	}
	return false;
}

bool Ini::Select(char *parent){
	char *formattedParent = new char[strlen(parent)+2];
	ZeroMemory(formattedParent, strlen(parent)+2);
	cout << "Select1\n";
	sprintf(formattedParent, "[%s]\r\n", parent);//this defines a parent, then next \r\n is from element
	cout << "Select2\n";
	//cout << "Comparing: " << szIniData.c_str() << "!!! to " << formattedParent << "!!!\n";
	if(szIniData.find(formattedParent) != szIniData.npos){
		cout << "Select3\n";
		if(cursor != 0)
			cursor = szIniData.find(formattedParent) + strlen(parent) + 8 + 1;//8 byte = \r\n\r\n[]\r\n and then the next byte as target(+1)
		else
			cursor = szIniData.find(formattedParent) + strlen(parent) + 4 + 1;//4 byte = [] and \r\n and then the next byte as target(+1)

		cout << "Select4\n";
		//delete formattedParent;
		return true;
	}
	cout << "Select5(error)\n";
	//delete formattedParent;
	return false;
}

bool Ini::Read(){
	long lSize;
	cout << "Read1\n";
	fseek(fFile, 0, SEEK_END);
	cout << "Read2\n";
	lSize = ftell(fFile);
	cout << "Read3\n";
	rewind(fFile);
	cout << "Read4\n";

	if(lSize < MAX_INI_LENGTH){
		char *buffer = new char[lSize];
		fread(buffer, 1, lSize, fFile);
		szIniData = buffer;

		//delete buffer;
		return true;
	}
	cout << "Read5(error)\n";
	return false;
}

char *Ini::ReadElement(char *element){
	int pointer = 0, offset = 0, nextElement = 0;
	cout << "ReadElement1\n";
	if(szIniData.find(element) == szIniData.npos)
		return NULL;

	if((char) szIniData[szIniData.find(element)-1] != '\n'){
		cout << "ReadElement2(Special Path)\n";
		string part;
		part.assign(szIniData);
		cout << "Char before Pass is " << (char) part[part.find(element)-1] << "!!!";
		while((char) part[part.find(element)-1] != '\n'){
			//pointer = szIniData.find(element+offset) + strlen(element) + 3;//the +3 are the the <SP>=<SP>
			int partSize = part.size();
			offset += strlen(element);
			part.assign(part.substr((int)part.find(element) + offset, partSize-((int) element)));
			nextElement = part.find(element);
			pointer = szIniData.find(element)+1 + nextElement + strlen(element) + 6;
		}
	}else{
		cout << "ReadElement3(After special)\n";
		pointer = szIniData.find(element) + strlen(element) + 3;//the +3 are the the <SP>=<SP>
	}
	int pointer2 = szIniData.find("\r\n", pointer);
	string result;
	result.assign(szIniData.substr(pointer, pointer2-pointer));
	cout << "ReadElement4\n";
	cout << result.c_str() << endl;
	cout << result.c_str() << endl;
	cout << result.c_str() << endl;
	cout << "ReadElement5 result.size = " << result.size() << "\n";
	char *cElement = new char[result.size()];
	cout << "ReadElement6\n";
	strcpy(cElement, result.c_str());
	cout << "ReadElement7\n";
	getchar();
	return cElement;
}

bool Ini::Write(char *element, char *value){
	cout << "Write1\n";
	fseek(fFile, cursor, SEEK_SET);
	cout << "Write2\n";
	fputs("\r\n", fFile);
	fputs(element, fFile);
	fputs(" = ", fFile);
	fputs(value, fFile);
	cout << "Write3\n";
	cursor += strlen(element) + strlen(value) + 6;
	cout << "Write4\n";
	return true;
}

bool Ini::WriteParent(char *parent){
	fseek(fFile, cursor, SEEK_SET);
	if(cursor!=0)
		fputs("\r\n\r\n", fFile);//+4bytes

	fputs("[", fFile);//+1byte
	fputs(parent, fFile);//+11 byte
	fputs("]", fFile);//+1byte
	fputs("\r\n", fFile);//+2byte
	//total 15 (+ 4) byte
	//cursor += strlen(parent) + 4;
	return true;
}

bool Ini::Close(){
	fclose(fFile);
	return true;
}

void Ini::Clear(){
	fputs("\r\n", fFile);//to ignore the last trash read. caused by ??? look into
}

//and the main file looks something like this:
Ini *ini = new Ini();
ini->Open(szIniFile);//Open the file
	ini->Read();
	if(ini->Select("ConnectionInfo")){
		ini->ReadElement("IP");
                ini->ReadElement("Port");
                ini->ReadElement("User");
                ini->ReadElement("Pass");
	}


	if(ini->Select("LinkingInfo")){
		ini->ReadElement("URLPath");
		ini->ReadElement("Path");
	}
	ini->Close();//Close the file

Please help me with solving this issue ASAP!

Regards, Oscar

[linde12]

I think it is related to Ini::Select because when i comment it out i can get the size() but after Ini::Select() string::size wont work! Been trying to solve this for hours now and still no solution.
Don't hesitate questioning/answering!

[linde12]

Kind of selfspam but

char formattedParent[256] = "";
	//char *formattedParent = new char[strlen(parent)+2];
	ZeroMemory(formattedParent, strlen(parent)+2);
	cout << "Select1\n";
	sprintf(formattedParent, "[&#37;s]\r\n", parent);//this defines a parent, then next \r\n is from element

instead of

	char *formattedParent = new char[strlen(parent)+2];
	ZeroMemory(formattedParent, strlen(parent)+2);
	cout << "Select1\n";
	sprintf(formattedParent, "[%s]\r\n", parent);//this defines a parent, then next \r\n is from element

makes the program hold... anyone any idea why and better solutions? I dont wanna allocate everything into the stack, don't see why i cant allocate it to the heap and then use sprintf...

[Paul McKenzie]

Kind of selfspam but

There is no need to dynamically allocate memory if you use the proper container classes, such as std::vector.

bool Ini::Select(char *parent)
{
   std::vector<char> formattedParent(strlen(parent)+2, 0);
   sprintf(&formattedParent, "[&#37;s]\r\n", parent);
   cout << "Select2\n";
   if(szIniData.find(&formattedParent[0]) != szIniData.npos)
//...
}

There is now no need to delete[] anything, as std::vector takes care of all of that. Also, to get the underlying array within vector, you pass the address of the first element (see the &formattedParent[0]). Also, if you return or an exception is thrown for any reason, the vector will destroy itself correctly, instead of you putting "delete[]" at every single return point.

But your Select() function has a flaw right from the beginning. If parent is NULL, your code will more than likely crash.

Right now, your code is difficult to read, as you have commented code. If you're going to post code, post it without the commented lines, as they do not add any information to us -- instead it clutters things.

Regards,

Paul McKenzie

[Lindley]

I'm curious. If you use a std::string in one place (your class definition), why are you using char arrays elsewhere? Sticking to one type of string seems to make more sense.

[Paul McKenzie]

I'm curious. If you use a std::string in one place (your class definition), why are you using char arrays elsewhere? Sticking to one type of string seems to make more sense.

From what little the OP posted, I would hazard a guess that there is no need whatsoever for char*, either dynamically allocated or as parameter types.

Everything could be done as std::string, or if it needs to be modified std::vector<char>. Any API or external functions that require char* or const char* can be gotten from std::string or std::vector easily.

Regards,

Paul McKenzie

[Philip Nicoletti]

One problem area ... you have:

if (lSize < MAX_INI_LENGTH)
{
   char *buffer = new char[lSize];
   fread(buffer, 1, lSize, fFile);
   szIniData = buffer;

   //delete buffer;
   return true;
}

1. You need delete [] buffer to prevent a memory leak

2. the assignment to szIniData keeps going until a NULL character
is encountered, so there is a good chanch you are getting lots of
extra characters in your string. You can do the following instead:

szIniData.assign(buffer,buffer+lSize);

3. I believe the latest version of C++ guarentees that std::string is
contiguous, so you could read directly into the string ... something like:

szIniData.resize(lSize);
fread(&szIniData[0], 1, lSize, fFile);

[Martin O]

Kind of selfspam but

char formattedParent[256] = "";
	//char *formattedParent = new char[strlen(parent)+2];
	ZeroMemory(formattedParent, strlen(parent)+2);
	cout << "Select1\n";
	sprintf(formattedParent, "[&#37;s]\r\n", parent);//this defines a parent, then next \r\n is from element

instead of

	char *formattedParent = new char[strlen(parent)+2];
	ZeroMemory(formattedParent, strlen(parent)+2);
	cout << "Select1\n";
	sprintf(formattedParent, "[%s]\r\n", parent);//this defines a parent, then next \r\n is from element

makes the program hold.

What does 'hold' mean?

The best fix is what others have said--use std::string instead of char arrays.

That being said, you're overwriting your buffer in the 2nd code:

If parent = "abc" then strlen(parent) is 3.
So your code would be:

	char *formattedParent = new char[5];
	ZeroMemory(formattedParent, 5);
	cout << "Select1\n";
	sprintf(formattedParent, "[%s]\r\n", parent);

formattedParent can fit 5 characters but you're trying to fill it with 8 characters:
[ = 1,
%s = 3,
] = 1,
\r = 1,
\n = 1,
\0 (the null terminator) = 1.
Total = 8. You've overrun the buffer.

[Paul McKenzie]

Hello! As the title says when i run my code the program crashes if not running in it through Microsoft Visual C++.

As others have stated, use std::string. If you have the urge to use anything with char* or new char[], you'll have to fight this habit and investigate how to properly use std::string for that purpose. Only if there is a compelling reason to use dynamic allocation should you do so.

Let's look at your code.

1) Use the proper headers.
2) Remove using namespace std from the header file.
3) As others stated, use std::string, not char*.

#include <iostream>
#include <cstdio>
#include <Windows.h>

class Ini 
{
	int cursor, eof, MAX_INI_LENGTH;
	std::string szIniData;
	FILE *fFile;

public:
	Ini();
	bool Select(const std::string& parent);
	bool Read();
	void Clear();
	std::string ReadElement(const std::string& element);
	bool Write(const std::string& element, const std::sring& value);
	bool WriteParent(cconst std::string& parent);
	bool Open(const std::string& file);
	bool Close();
};

That is a starting interface. Now what would the implementation look like?

Note the use of streams instead of sprintf(), and the check for a file being NULL (which you didn't do).

#include "ini.h"
#include <iostream>
#include <sstream>

using namespace std;

Ini::Ini() :	MAX_INI_LENGTH(1024*4), eof(0), cursor(0), fFile(0)
{ }

bool Ini::Open(const std::string& file)
{
   fFile = fopen(file.c_str(), "r+");
   return fFile?true:false;
}

bool Ini::Select(const std::string& parent)
{
   string formattedParent;
   cout << "Select1\n";
   ostringstream strm;
   strm <<  "[" << parent << "]\r\n";
   formattedParent = strm.str();
   cout << "Select2\n";
   if(szIniData.find(formattedParent) != szIniData.npos)
   {
	cout << "Select3\n";
	if(cursor != 0)
	     cursor = szIniData.find(formattedParent) + strlen(parent) + 8 + 1;
	else
             cursor = szIniData.find(formattedParent) + strlen(parent) + 4 + 1;
	cout << "Select4\n";
	return true;
    }
    cout << "Select5(error)\n";
   return false;
}

bool Ini::Read()
{
    if ( !fFile )
        return false;
  
    long lSize;
    cout << "Read1\n";
    fseek(fFile, 0, SEEK_END);
    cout << "Read2\n";
    lSize = ftell(fFile);
    cout << "Read3\n";
    rewind(fFile);
    cout << "Read4\n";

    if (lSize < MAX_INI_LENGTH)
    {
          std::vector<char> buffer(lSize);
          fread(&buffer[0], 1, lSize, fFile);
          szIniData = string(buffer, lSize);
          return true;
    }
    cout << "Read5(error)\n";
    return false;
}

std::string Ini::ReadElement(const std::string& element)
{
    size_t elementSize = element;
    int pointer = 0, offset = 0, nextElement = 0;
    cout << "ReadElement1\n";
    if(szIniData.find(element) == szIniData.npos)
   	return "";

    if((char) szIniData[szIniData.find(element)-1] != '\n')
    {
        cout << "ReadElement2(Special Path)\n";
	string part;
	part.assign(szIniData);
	cout << "Char before Pass is " << (char) part[part.find(element)-1] << "!!!";
	while((char) part[part.find(element)-1] != '\n')
        {
            int partSize = part.size();
            offset += elementSize;
            part.assign(part.substr((int)part.find(element) + offset, partSize-((int) element)));
            nextElement = part.find(element);
            pointer = szIniData.find(element)+1 + nextElement + elementSize + 6;
       }
    }
    else
    {
	cout << "ReadElement3(After special)\n";
	pointer = szIniData.find(element) + elementSize + 3;
    }
    int pointer2 = szIniData.find("\r\n", pointer);
    string result;
    result.assign(szIniData.substr(pointer, pointer2-pointer));
    cout << "ReadElement4\n";
    cout << result.c_str() << endl;
    cout << result.c_str() << endl;
    cout << result.c_str() << endl;
    cout << "ReadElement5 result.size = " << result.size() << "\n";
    std::string cElement = result;
    return cElement;
}

This is just some of the (uncompiled) code. But the point is that you need no dynamic allocation. The only thing that you may need to watch out for is memory overwrite, as using vector doesn't prevent you from making those mistakes.

However, the problem of dynamic allocation is solved completely if you remove from your code your usage of using new[]/delete[]

Regards,

Paul McKenzie