Segmentation Fault when writing to a file larger than 15360 bytes

[Wes]

I'm running FreeBSD 6.1 RELEASE #2. The program is writting in C++.

The idea of the program is to open one file as input, read bytes from
it, do some bitwise operations on the bytes, and then write them to
this second file. However, when the second file is 15360 bytes long,
the program dies with a "Segmentation Fault (core dumped)" error!

I checked with gdb, and it says the last function to run was memcpy()
from libc, which would explain the segmentation fault I think (the
function was passed an invalid pointer maybe?).

So how do I fix it? Below is the function's code:

### BEGIN CODE ###
int encFile(char* file)
{
ifstream:os_type size;
char* buffer = new char[32];
char* ofile = new char[sizeof(file) + 4];
char* tbuff = new char[32];

ifstream plain(file, ios::in|ios::binary|ios::ate);

if(!plain.is_open())
{
cout << "Unable to open file '" << file
<< "'. Closing." << endl;
return 0;
}

size = plain.tellg();
plain.seekg(0, ios::beg);

for(int i = 0; i < sizeof(file); i++)
{
ofile = file;
}

ofile[sizeof(file) + 1] = '.';
ofile[sizeof(file) + 2] = 'e';
ofile[sizeof(file) + 3] = 'n';
ofile[sizeof(file) + 4] = 'c';

ofstream enc(ofile, ios:ut|ios::binary);

while(size - plain.tellg() > 32)
{
plain.read (buffer, 32);
tbuff = Encrypt(buffer);
enc.write(tbuff, 32); // this is where we die at 15360
}

size = size - plain.tellg();
plain.read(buffer, size);
enc.write(Encrypt(buffer), size);

plain.close();
enc.close();
delete buffer;
delete ofile;
return 0;
}
### END CODE ###

Any thoughts?

 

[Marcus Kwok]

char* ofile = new char[sizeof(file) + 4]; [snip]
ofile[sizeof(file) + 1] = '.';
ofile[sizeof(file) + 2] = 'e';
ofile[sizeof(file) + 3] = 'n';
ofile[sizeof(file) + 4] = 'c';

ofile can hold (sizeof(file)+4) entries, indexed from 0 to
(sizeof(file)+3). However, you index (sizeof(file)+4), which is one
past the end of ofile.

 

[Jay_Nabonne]

### BEGIN CODE ###
int encFile(char* file)
{
ifstream:os_type size;
char* buffer = new char[32];
char* ofile = new char[sizeof(file) + 4];

sizeof(file) is sizeof(char*), which is probably 4. Not quite what you
wanted. I think you mean "strlen(file)" (and use it for all the other
sizeof(file)'s below, too). You will also want to add one more for the
appended zero-terminator.

char* tbuff = new char[32];

ifstream plain(file, ios::in|ios::binary|ios::ate);

if(!plain.is_open())
{
cout << "Unable to open file '" << file
<< "'. Closing." << endl;
return 0;
}

size = plain.tellg();
plain.seekg(0, ios::beg);

for(int i = 0; i < sizeof(file); i++)
{
ofile = file;
}

Easier: strcpy(ofile, file);

ofile[sizeof(file) + 1] = '.';
ofile[sizeof(file) + 2] = 'e';
ofile[sizeof(file) + 3] = 'n';
ofile[sizeof(file) + 4] = 'c';

Easier: strcat(ofile, ".enc");

ofstream enc(ofile, ios:ut|ios::binary);

while(size - plain.tellg() > 32)
{
plain.read (buffer, 32);
tbuff = Encrypt(buffer);

Not sure what Encrypt returns, but it looks like you're overwriting the
pointer tbuff. You allocated it as 32 bytes initially, but this assignment
reassigns the pointer. Without seeing the code for Encrypt, it's difficult
to tell exactly what you meant it do. But it seems highly likely you will
need a static buffer being returned by Encrypt.

enc.write(tbuff, 32); // this is where we die at 15360
}

size = size - plain.tellg();

if (size != 0)
{

plain.read(buffer, size);
enc.write(Encrypt(buffer), size); }

plain.close();
enc.close();
delete buffer;

Should be: delete [] buffer;

delete ofile;

Should be: delete [] ofile;

return 0;
}
### END CODE ###

Any thoughts?

- Jay

 

[Jim Langston]

I'm running FreeBSD 6.1 RELEASE #2. The program is writting in C++.

The idea of the program is to open one file as input, read bytes from
it, do some bitwise operations on the bytes, and then write them to
this second file. However, when the second file is 15360 bytes long,
the program dies with a "Segmentation Fault (core dumped)" error!

I checked with gdb, and it says the last function to run was memcpy()
from libc, which would explain the segmentation fault I think (the
function was passed an invalid pointer maybe?).

So how do I fix it? Below is the function's code:

### BEGIN CODE ###
int encFile(char* file)
{
ifstream:os_type size;
char* buffer = new char[32];
char* ofile = new char[sizeof(file) + 4];

Ooops. You don't know the sizeof the file here. sizeof(file) is going to
be the size of the variable file, which is a character, and on most
implementations is going to be 1. So you are allocating 5 bytes. Not what
you wanted. At this point just keep the buffer to nothing.
char* buffer = NULL;

char* tbuff = new char[32];

ifstream plain(file, ios::in|ios::binary|ios::ate);

if(!plain.is_open())
{
cout << "Unable to open file '" << file
<< "'. Closing." << endl;
return 0;
}

size = plain.tellg();

Ahh, NOW we know the size of the file! It's in the variable size so now you
can do your:
buffer = new char[size + 4];

Rest of the code I"m not really checking.

plain.seekg(0, ios::beg);

for(int i = 0; i < sizeof(file); i++)
{
ofile = file;
}

ofile[sizeof(file) + 1] = '.';
ofile[sizeof(file) + 2] = 'e';
ofile[sizeof(file) + 3] = 'n';
ofile[sizeof(file) + 4] = 'c';

ofstream enc(ofile, ios:ut|ios::binary);

while(size - plain.tellg() > 32)
{
plain.read (buffer, 32);
tbuff = Encrypt(buffer);
enc.write(tbuff, 32); // this is where we die at 15360
}

size = size - plain.tellg();
plain.read(buffer, size);
enc.write(Encrypt(buffer), size);

plain.close();
enc.close();
delete buffer;
delete ofile;
return 0;
}
### END CODE ###

Any thoughts?

 

[Old Wolf]

char* buffer = new char[32];
char* ofile = new char[sizeof(file) + 4];
char* tbuff = new char[32];

for(int i = 0; i < sizeof(file); i++)
{
ofile = file;
}

ofile[sizeof(file) + 1] = '.';
ofile[sizeof(file) + 2] = 'e';
ofile[sizeof(file) + 3] = 'n';
ofile[sizeof(file) + 4] = 'c';

Buffer overflow

ofstream enc(ofile, ios:ut|ios::binary);

ofstream expects a zero-terminated string, so it is going to keep on
reading garbage until it hits a zero or gets a segfault.

while(size - plain.tellg() > 32)
{
plain.read (buffer, 32);
tbuff = Encrypt(buffer);
enc.write(tbuff, 32); // this is where we die at 15360
}

You just pointed tbuff to another buffer -- leaking the memory
that you allocated earlier.

delete buffer;
delete ofile;

delete can only be used for stuff you new'd. delete[] must be
used for stuff you new[]'d.


Why are you trying to use manual memory management anyway?
Have you considered:

char buffer[32];
string ofile;

ofile = file;
ofile += ".enc";

ofstream( ofile.c_str(), ios:ut | ios::binary );

/* no 'delete' required */

Also, you could improve a lot on your read loop, by not
relying on the file size (eg. what if the file is a pipe, or
changes while you are reading it?), and by not duplicating
your loop contents after your loop !

while ( 0 < (size = plain.readsome(buffer, sizeof buffer)) )
enc.write( Encrypt(buffer), size);

Note that this doesn't work for most block encryption
algorithms -- your last block will mess up if it doesn't
finish on a multiple of 32 (you have garbage in the end
of the input, and then you only write part of the output,
making it impossible to reconstruct the input).

For most block encryption algorithms, you would
null-pad the input and then always output a full block:

for (;
{
char buffer[32] = { 0 };
if ( 0 == plain.readsome(buffer) )
break;
enc.write( Encrypt(buffer), sizeof buffer );
}

 

[Earl Purple]

I'm running FreeBSD 6.1 RELEASE #2. The program is writting in C++.

No, its written in C except that you have used new instead of malloc
and used ifstream when you may as well have used FILE *.

So how do I fix it? Below is the function's code:

### BEGIN CODE ###
int encFile(char* file)

Wrong. You are not going to change the variable file. So should be int
encFile( const char * file ) or encFile ( const std::string & file )

{
ifstream:os_type size;
char* buffer = new char[32];

Why not just
char buffer[32];

if you want a local buffer of 32 characters. You might also use vector<
char >

char* ofile = new char[sizeof(file) + 4];

presumably you mean strlen( file ) + 4 and don't forget the null
terminator so probably strlen( file ) + 5.

Again you shouldn't be using new but because this time you don't know
the size in advance, you should either use string or vector<char>. Note
that if you are going to write this in C(99) then you may indeed create
a local array of a size determined at run-time. But in C there is no
vector.

char* tbuff = new char[32];

as before. What is tbuff?

ifstream plain(file, ios::in|ios::binary|ios::ate);

if(!plain.is_open())
{
cout << "Unable to open file '" << file
<< "'. Closing." << endl;
return 0;
}

size = plain.tellg();
plain.seekg(0, ios::beg);

for(int i = 0; i < sizeof(file); i++)
{
ofile = file;
}

ofile[sizeof(file) + 1] = '.';
ofile[sizeof(file) + 2] = 'e';
ofile[sizeof(file) + 3] = 'n';
ofile[sizeof(file) + 4] = 'c';

ofstream enc(ofile, ios:ut|ios::binary);

while(size - plain.tellg() > 32)
{
plain.read (buffer, 32);
tbuff = Encrypt(buffer);
enc.write(tbuff, 32); // this is where we die at 15360
}

if you use readsome instead of read you can see how many bytes were
read and act accordingly.

What does this do:

tbuff = Encrypt( buffer ) ?

tbuff has already been assigned in your program to point to a newly
allocated array of bytes and now you are pointing it at something else.
Of course, Encrypt should probably return a std::string. Note that
strings can contain nul characters.

size = size - plain.tellg();
plain.read(buffer, size);
enc.write(Encrypt(buffer), size);

How does Encrypt() know that in this case there are fewer than 32 bytes
that were read?

plain.close();
enc.close();

delete buffer;
delete ofile;

wrong form of delete, but then you shouldn't have to be calling it in
the first place.
tbuff was never deleted.

return 0;
}
### END CODE ###

Any thoughts?

Yes. Either program in C or program in C++, not in a hybrid.

 

[Old Wolf]

No, its written in C except that you have used new instead of malloc
and used ifstream when you may as well have used FILE *.

Yes. Either program in C or program in C++, not in a hybrid.

The original program, bugs aside, is a valid C++ program and
must be successfully compiled by a compiler that conforms to
the C++ standard. Whether it resembles a correct C program
is of no significance.

There is nothing in the C++ standard that says people are not
allowed to use pointers to char (I guess that is the point you
are trying to make).