Segmentation faults out the ****

i have the following:

Code:

myfile=fopen("getlin.txt","r+");
char*compare=(char*)malloc(sizeof(char*)*20);
int*choice=(int*)malloc(sizeof(int*)*2);
scanf("%d",choice);
printf("\nWhat would you like to search for?");
scanf("%s",compare);
printf("%s\n",compare);
ReadCatalogue(myfile,*choice,*compare);    /*Call to function, ReadCatalogue, (this is were it segments)*/



void ReadCatalogue(FILE *myfile, int choose, char**compare)     /*The called function, ReadCatalogue*/

is anyone able to suggest a reason for this?

i know it has something to do with my arguments when calling ReadCatalogue cause i get this warning when compiling

warning: passing arg 3 of `ReadCatalogue' makes pointer from integer without a cast.

this wasnt covered in the lecture notes and i havent a clue what it means.

Also before you go and make fun at my stupid errors, remember i have been using C for less than 20 hours total.

Thanx for your help and please post before you die of laughter cause i need a solution.

Your compare variable is a pointer, but ReadCatalogue expects a pointer to a pointer. Change the call to this and see how it works:

Code:

ReadCatalogue(myfile,*choice,&compare);

nope soz dude that didnt help at all, still segmenting.

i now give you permission to laugh at my code

Well I would, if there were something to laugh at. By the way, I have a way to call malloc() that's easier to get right:

Code:

char*compare=(char*)malloc(sizeof(char*)*20);
int*choice=(int*)malloc(sizeof(int*)*2);

Becomes:

Code:

char *compare = malloc(20 * sizeof *compare);
int *choice = malloc(2 * sizeof *choice);

Your calls should have used sizeof(char) and sizeof(int), respectively. But with my way you don't have to worry about that.

ok, i chagnged it so that you dont get the compare string in the main function, but in the ReadCatalouge function, thus reducing the amount of things being passed to ReadCatalogue.

segementation area is highlighted in blue.

Code:

myfile=fopen("getlin.txt","r+");
int*choice=(int*)malloc(sizeof(int*)*2);
scanf("%d",choice);
ReadCatalogue(myfile,1); 

void ReadCatalogue(FILE *myfile, int choose)
char*compare=(char*)malloc(sizeof(char*)*20);
printf("\nWhat would you like to search for?");
scanf("%s",compare);

i have highlighted in red the part i think is causeing the error.

So show us the ReadCatalogue function, and not just your call to it.


Quzah.

the above post is no longer valid, ive change the code soo much.

ok heres the new porblem.

Code:

char*Catalogue = (char*)malloc(sizeof(char*)*10);
scanf("%s",&Catalogue);
if((myfile = fopen(&Catalogue,"r+"))==NULL)
{
   printf("\nFile could not be loaded.\n");
}
else
{
   printf("\nFile opened and has been read.\n");
   ReadCatalogue(myfile);
   fclose(myfile);
}
printf("hehe");

this is inside a loop whos condition is independant of the file useage, it is controled by what number a user selects earlier in the program and thus should not be affected by the opening of the file.

i input text.txt, which exists and has the following text in it.

Code:

reddas
red,da
25/04/1986
4.55

the output when this is run is:

Code:

File opened and has been read.
hehe11>

why does it close the program?

it should return to the top of the loop and ask me to select another option.

please help, i know that it calls ReadCatalogue, reads the stuff from the .txt file and returns to main, but it quits on me and that isnt acceptable.

Code:

char*Catalogue = (char*)malloc(sizeof(char*)*10);
scanf("%s",&Catalogue);

%s with scanf expects a char*. Therefore, you should not give it the address of Catalogue which makes a char**. You had this correct in your original code.

Also, when using %s you should specify a maximum number of characters to read. Otherwise, scanf can read an unlimited number of characters, possibly overflowing your buffer.

malloc is generally not required when you want to create a fixed size allocation. Just use an array.

Finally, it's a good idea to use spaces in your code. This makes it much easier to read.

Code:

char Catalogue[50];
scanf("%49s", Catalogue);

~~~~

Code:

if((myfile = fopen(&Catalogue,"r+"))==NULL)

Again, fopen expects a char* so don't give it the address of Catalogue.

Code:

if((myfile = fopen(Catalogue, "r+")) == NULL)

Originally Posted by Ubber_C_Noob

ok heres the new porblem.

Code:

char*Catalogue = (char*)malloc(sizeof(char*)*10);
scanf("%s",&Catalogue);

Well it's two of them anyway. No, make that more...

1) Catalogue is a pointer to a character. You allocate 10 pointers to characters, not 10 characters.
2) You typecast malloc. Which if this was right, would be wrong. So really you end up allocating space for 10 pointers to characters, but you treat them as characters. I suppose technically you and get away with that, but it's still wrong.
3) You don't use & when you already have a pointer that you're using with scanf. You're passing it the address of a pointer, not a pointer.

Originally Posted by Ubber_C_Noob

Code:

if((myfile = fopen(&Catalogue,"r+"))==NULL)
{
   printf("\nFile could not be loaded.\n");
}

You're passing the address of a pointer again, not the pointer itself...

Originally Posted by Ubber_C_Noob

Code:

else
{
   printf("\nFile opened and has been read.\n");
   ReadCatalogue(myfile);
   fclose(myfile);
}
printf("hehe");

No, it actually hasn't been read at this point. Also, wait a second...

Originally Posted by quzah

So show us the ReadCatalogue function, and not just your call to it.

Yeah, I still don't see ReadCatalogue any place...


Quzah.
[edit] Beaten twice. This is what happens when you harvest felled high plains arbor while you type... [/edit]

ok, you wanted the function ReadCatatlogue here it is:

Code:

void ReadCatalogue(FILE *myfile)
{
   char*loadedfile=(char*)malloc(sizeof(char*)*600);
   char*temp = (char*)malloc(sizeof(char*)*20);
   while(fgets(temp,sizeof(temp),myfile))
   {
      strcat(loadedfile,temp);
   }
   printf(loadedfile);
}

this i know is correct and i know that the file has been read cause i get a print out of the file before the thing quits on me.

that printf() statment above is supposed to tell the user that the file has been opened and read only if it was able to be opened, since were only useing txt files i dont see how you can open the file and not be able to read it in the future.

Code:

char*loadedfile=(char*)malloc(sizeof(char*)*600);
char*temp = (char*)malloc(sizeof(char*)*20);

I'm willing to bet you're not allocating the amount of memory you think you are here. As someone else already pointed out, the proper way should be:

Code:

char *loadedfile = malloc(sizeof(*loadedfile)*600);
char *temp = malloc(sizeof(*temp)*20);

You should really read the comments people are posting here, because you keep making the same mistakes. Your segfault is most likely occuring because you are overwriting the loadedfile buffer with the line:

Code:

strcat(loadedfile,temp);

You never check to see if you are adding more characters to loadedfile than there is room for.

what? remember, total programing time < 20 hours, speak slo and use small words please.

and wouldnt 599 slots be enough? i would have thought it would.

and anyway, wouldnt that go wrong if i read a file with more than 599 ch? my file has 30 chars atm.

A couple of comments
1) The practice;

Code:

   type *somename = (type *)malloc(sizeof(type *)*some_constant);

is wrong. You've done this in a few places (with type as an int or as a char) Depending on the size of type vs size of a pointer to type, the code that uses "somename" may or may not give a buffer overrun. In C the typecast is optional (and viewed as bad style). In C++ it is mandatory. In C, use;

Code:

   type *somename = malloc(sizeof(type)*some_constant);

The latest version of ReadCatalogue is broken(). The following is it with comments added.

Code:

void ReadCatalogue(FILE *myfile)
{
   char*loadedfile=(char*)malloc(sizeof(char*)*600);    /* refer note above */
   char*temp = (char*)malloc(sizeof(char*)*20);           /* refer note above */
   while(fgets(temp,sizeof(temp),myfile))                       /* sizeof(temp) will typically be 4  on 32 bit systems.  You probably want a value of about 19 */
   {
      strcat(loadedfile,temp);                                          /* undefined behaviour.   The buffer pointed at by loadedfile is not initialised */
   }
   printf(loadedfile);
}

and wouldnt 599 slots be enough?

First of all, you're not allocating 599 slots. You're allocating 600 * sizeof(char*) slots which probably ends up being 600 * 4 = 2400 depending on your machine.

Second of all, how do you know the memory you've allocated is enough? You never check the file size beforehand, or keep track of how many bytes you are copying in to the loadedfile buffer.

Here is a better version of your function

Code:

void ReadCatalogue(FILE *myfile)
{
	char		*loadedfile;
	char		temp[512];
	int		buffersize = 600;
	int		bytescopied = 1; // start at 1 to account for null term

	loadedfile = malloc(buffersize);
	loadedfile[0] = '\0';  // so strcat() doesn't tank the first through the loop

	while(fgets(temp,sizeof(temp),myfile))
	{
		bytescopied += strlen(temp);
		if(bytescopied > buffersize)
			break; // break from while loop if buffer isnt big enough to copy everything
		strcat(loadedfile,temp);
	}
	printf(loadedfile);
}