stack smashing dealing with files...

hello everybody!
I wanted to post this thread just to ask for your comments about this code, but unfortunately i just realized that i have also a bug in it so i have to make a double question for help and comments...
The excercise should be (is) very simple: open a file and copy it to another file with the same name followed by ".short" by leaving at most 1 empty line when more than 1 consecutive such lines are met.
Example:
[\n]
hello
[\n]
[\n]
should become the same without the last [\n].

I wanted to write a program which checked all the bad cases (thing which usually i don't do, above all while writing files.. ) and this is my program:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main (void)
{
	FILE *fPtr,*tmpPtr;
	char filename[]="examXX906.txt";

	if ((fPtr=fopen(filename,"r"))!=NULL)
	{
		if ((tmpPtr=fopen("tmpXX906.txt","w+"))!=NULL)
		{
			int ch,isempty=1,emptylines=0;

			while ((ch=fgetc(fPtr))!=EOF)
			{
				if (ch!='\n')
				{
					emptylines=0;
					isempty=0;
					if (fputc(ch,tmpPtr)==EOF)
					{
						printf("ERROR WRITING THE TEMP FILE\n");
						exit(1);
					}
				}
				else
				{
					if ((isempty==1))
					{
						emptylines++;
						if (emptylines<2)
						{
							if (fputc(ch,tmpPtr)==EOF)
							{
								printf("ERROR WRITING THE TEMP FILE\n");
								exit(1);
							}
						}
					}
					else
					{
						isempty=1;
						if (fputc(ch,tmpPtr)==EOF)
						{
							printf("ERROR WRITING THE TEMP FILE\n");
							exit(1);
						}
					}
				}
			}
			if (ferror(fPtr))
			{
				printf("ERROR WHILE READING SOURCE FILE\n");
				exit(1);
			}
			else
			{
				FILE *targetPtr;
				strcat(filename,".short");

				if ((targetPtr=fopen(filename,"w+"))!=NULL)
				{
					rewind(tmpPtr);
					while ((ch=fgetc(tmpPtr))!=EOF)
					{
						if (fputc(ch, targetPtr)==EOF)
						{
							printf("ERROR WRITING THE TARGET FILE\n");
							exit(1);
						}
					}

					if (ferror(tmpPtr))
					{
						printf("ERROR WHILE READING TEMP FILE\n");
						exit(1);
					}

					fclose(fPtr);
					fclose(targetPtr);
					fclose(tmpPtr);
					remove("tmpXX906.txt");
				}
				else
				{
					fclose(fPtr);
					fclose (tmpPtr);
					remove("tmpXX906.txt");
					printf("Couldn't create target file\n");
					exit(1);
				}
			}
		}	
		else
		{
			fclose(fPtr);
			printf("Couldn't open temp file\n");
			exit(1);
		}
	
	}
	else
	{
		printf("Couldn't open source file\n");
		exit(1);
	}

	return 0;
}

it seems to work but i get a very worrying "STACK SMASH DETECTED. Aborted" error....
what is wrong there?

and above all, since on saturday i will be asked to write a (maybe) similar program on a sheet of paper, i'd kindly like to ask you hints for writing programs that deal with file by minimizing the possibilities of mistakes (if you forget a ";" you don't pass this exam... )..
For example, i thought to solve this same excercise by using FGETS() but i think that it's better to have a small set of functions which have not a wide variety of worst-cases while writing on paper, do you agree?

many thanks for all your help/hints!
Bye

Code:

strcat(filename,".short");

You haven't got space to extend filename, as you give the compiler the task of figuring out the size when you say "char filename[] = ..." - the compiler will use the assigned string to give the array the size, and when you add another 5 letters onto it, it will overflow it's allocated space.

As a side note, try to generalize your code to not repeat this sequence:

Code:

					fclose(fPtr);
					fclose (tmpPtr);
					remove("tmpXX906.txt");

--
Mats

Originally Posted by matsp

Code:

strcat(filename,".short");

You haven't got space to extend filename, as you give the compiler the task of figuring out the size when you say "char filename[] = ..." - the compiler will use the assigned string to give the array the size, and when you add another 5 letters onto it, it will overflow it's allocated space.

As a side note, try to generalize your code to not repeat this sequence:

Code:

					fclose(fPtr);
					fclose (tmpPtr);
					remove("tmpXX906.txt");

--
Mats

i figured this problem and tried to recompile before posting... maybe i recompiled without saving the file because now works..
anyway, thanks a lot!

by the way, i realized now that i should add a control for in the case of a file with an empty line at the end.. it gets added to the file anyway..

Just something I want to mention. You end up with several levels of indentation in your code. I generally go by the rule that code that's running 90% of the time, should be in the lowest level of indentation. Hence, rather than writing:

Code:

rcode = foo();
if (rcode == 0) {
   rval = bar();
   if (rval == 0) {
       /* Do 
           other 
               stuff, 
               introducing 
           more 
           nesting */
   } else {
       printf ("bar returned non-zero!\n");
       return 1;
   }
} else {
   printf ("foo returned non-zero!\n");
   return 1;
}

You should do this:

Code:

rcode = foo();
if (rcode != 0) {
   printf ("foo returned non-zero\n");
   return 1;
}
rval = bar ();
if (rval != 0) {
   printf ("bar returned non-zero\n");
   return 1;
}

/* Now do all the other stuff */

In other words, don't enter an if statement because everything is hunky-dory, but rather when some sort of unusual condition occurs (such as non-zero rcodes);

QuantumPete

Originally Posted by QuantumPete

In other words, don't enter an if statement because everything is hunky-dory, but rather when some sort of unusual condition occurs (such as non-zero rcodes);

This is more a point of argument than you make it sound. I personally have no problem with early returns, and so I code a lot like your second example. But other respectable people believe that early exits are stylistically poor. These people write code sort of like your first example, where the rule is to keep the returns as low in the function as possible, and to minimize their number as much as possible.

You might think this would lead to crazy deep indentation levels, and it does. However, most programmers who use this practice take that as an indication that their code needs to be broken into smaller functions. And that's a good thing anyway.

Originally Posted by brewbuck

This is more a point of argument than you make it sound.

I only meant to illustrate that nesting if statements like that can be avoided. I prefer the second version and that's what I would always teach people, but you're right, there's nothing *wrong* with the first version. And I agree that in the end it always boils down to a pointless argument like whether the opening brace should be at the end of the line or on the next one.

QuantumPete

Hello my friends!
Thanks again for your hints! now it looks much clearer above all if i have to write it on paper!
I did another excercise: Read from a file which contains numbers written in ternary, convert them to decimal and copy the result in a second file whose name is the same of the source file with extension ".result".
I coded with the QuantumPete's 2nd indentation and here is the result.
The only thing which is still not really clear to me is about the way to append the ".result" extension to the filename.
The program run 2 or 3 times and then i got the error i report below (which looks like a linking problem?). What did i wrong? thanks again!

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <math.h>

unsigned int ternary2dec (unsigned int n);
int fileisempty(FILE *f);

int main(int argc, char *argv[])
{

	if (argc!=2)
	{
		printf("USAGE: ./esameX307 sourcefilename\n");
		exit(1);
	}

	FILE *srcPtr,*tgtPtr;
	if ((srcPtr=fopen(argv[1],"r"))==NULL)
	{
		printf ("Couldn't open file %s\n",argv[1]);
		exit(1);
	}

	if (fileisempty(srcPtr)==1)
	{
		printf("Source file is empty!\n");
		exit(1);
	}

	if (fileisempty(srcPtr)==2)
	{
		printf("There was a proble while reading source file!\n");
		exit(1);
	}
	
	size_t tgtfilenamesize=sizeof(argv[1]);
	tgtfilenamesize+=8; //.result\0
	char *tgtFN = (char *) malloc (tgtfilenamesize*sizeof(char));
	
	if (tgtFN==NULL)
	{
		printf("Couldn't get memory for the target filename\n");
		exit(1);
	}
	
	strcpy(tgtFN,argv[1]);
	strcat(tgtFN,".result");
	puts(tgtFN);
	if ((tgtPtr=fopen(tgtFN,"w+"))==NULL)
	{
		printf ("Couldn't open file %s\n",argv[1]);
		exit(1);
	}
	
	unsigned int sum=0;
	unsigned int num;

	while (fscanf(srcPtr,"%u",&num)!=EOF)
	{
		sum+=ternary2dec(num);
	}
	printf("SUM %d\n",sum);
	fprintf(tgtPtr,"RESULT: %u",sum);
	fclose(srcPtr);
	fclose(tgtPtr);
	return 0;

}

unsigned int ternary2dec (unsigned int n)
{
	unsigned int ret=0;
	int i=0;
	while (n>0)
	{
		ret += pow(3,i)*(n%10);
		i++;
		n/=10;
		
	}
	return ret;

}

int fileisempty(FILE *f)
{
	//1: file empty
	//2: other errors
	//0: file not empty
	rewind(f);
	int ch;
	ch = fgetc(f);
	if (ch==EOF)
	{
		if (feof(f))
		{
			printf("FILE IS EMPTY\n");
			rewind(f);
			return 1;
		}
		else
		{
			printf("Other errors reading source file\n");
			rewind(f);
			return 2;
		}
	}
	rewind(f);
	return 0;
}

Code:

./esameX307 esameX307.txt
esameX307.txt.result
SUM 134
*** glibc detected *** ./esameX307: free(): invalid next size (normal): 0x0804a180 ***
======= Backtrace: =========
/lib/tls/i686/cmov/libc.so.6[0xb7de1d65]
/lib/tls/i686/cmov/libc.so.6(cfree+0x90)[0xb7de5800]
/lib/tls/i686/cmov/libc.so.6(fclose+0x134)[0xb7dd06f4]
./esameX307[0x8048899]
/lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe0)[0xb7d8e050]
./esameX307[0x8048621]
======= Memory map: ========
08048000-08049000 r-xp 00000000 08:02 3409197    /home/federico/università/programmazione C/esercizi/esameX307
08049000-0804a000 rw-p 00000000 08:02 3409197    /home/federico/università/programmazione C/esercizi/esameX307
0804a000-0806b000 rw-p 0804a000 00:00 0          [heap]
b7c00000-b7c21000 rw-p b7c00000 00:00 0
b7c21000-b7d00000 ---p b7c21000 00:00 0
b7d77000-b7d78000 rw-p b7d77000 00:00 0
b7d78000-b7ebc000 r-xp 00000000 08:02 1049547    /lib/tls/i686/cmov/libc-2.6.1.so
b7ebc000-b7ebd000 r--p 00143000 08:02 1049547    /lib/tls/i686/cmov/libc-2.6.1.so
b7ebd000-b7ebf000 rw-p 00144000 08:02 1049547    /lib/tls/i686/cmov/libc-2.6.1.so
b7ebf000-b7ec2000 rw-p b7ebf000 00:00 0
b7ec2000-b7ee5000 r-xp 00000000 08:02 1049551    /lib/tls/i686/cmov/libm-2.6.1.so
b7ee5000-b7ee7000 rw-p 00023000 08:02 1049551    /lib/tls/i686/cmov/libm-2.6.1.so
b7ee8000-b7ef2000 r-xp 00000000 08:02 1015875    /lib/libgcc_s.so.1
b7ef2000-b7ef3000 rw-p 0000a000 08:02 1015875    /lib/libgcc_s.so.1
b7ef3000-b7ef4000 rw-p b7ef3000 00:00 0
b7ef5000-b7ef7000 rw-p b7ef5000 00:00 0
b7ef7000-b7f11000 r-xp 00000000 08:02 1015821    /lib/ld-2.6.1.so
b7f11000-b7f13000 rw-p 00019000 08:02 1015821    /lib/ld-2.6.1.so
bf98f000-bf9a5000 rw-p bf98f000 00:00 0          [stack]
ffffe000-fffff000 r-xp 00000000 00:00 0          [vdso]
Aborted (core dumped)

Code:

	size_t tgtfilenamesize=sizeof(argv[1]);

You probably want

Code:

	size_t tgtfilenamesize=strlen(argv[1]);

Code:

if (fileisempty(srcPtr)==1)
	{
		printf("Source file is empty!\n");
		exit(1);
	}

	if (fileisempty(srcPtr)==2)
	{
		printf("There was a proble while reading source file!\n");
		exit(1);
	}

instead of calling same function twice in a row - call it once, store the result in some temp var and check the variable value to be 1 or 2

> size_t tgtfilenamesize=sizeof(argv[1]);
Maybe you mean strlen, not sizeof

Originally Posted by Dave_Sinkula

Code:

	size_t tgtfilenamesize=sizeof(argv[1]);

You probably want

Code:

	size_t tgtfilenamesize=strlen(argv[1]);

ops... i really hoped it was not a conceptual mistake!
THX!

Originally Posted by smoking81

ops... i really hoped it was not a conceptual mistake!
THX!

Well, confusing sizeof() and strlen() could be viewed as a conceptual mistake.

Originally Posted by brewbuck

Well, confusing sizeof() and strlen() could be viewed as a conceptual mistake.

i know it could be conceptual, fortunately i had strlen() in my mind..