HELP!!

[totoro2468]

Here is a code I am writing. I keep geting segmentation fault.
I'm not sure what i'm doing wrong.
Can you explain it to me in PLAIN ENGLISH??

void ReadString (char *filename, int *lengthPtr, char *textFilePtr)
{
FILE *ifp;
char *text;
char txtFile[FILELENGTH];

char c;
int x = 0, y = 0, items;

text = txtFile;

printf ("Enter the name of the file to analyze : ");
fgets (filename, FILES, stdin);

printf ("%s", filename);

ifp = fopen(filename, "r");

text = malloc(FILELENG TH * sizeof(char)); **I keep
getting Segmentation fault here**

if(ifp == NULL)
{
fprintf (stderr, "Error opening file\n");
exit (-2);
}

while (fscanf (ifp, "%c", &c)!= EOF)
{
sprintf (txtFile, "%c", c);
y++;
}

for (x = 0; x < FILELENGTH; x++);
{
printf ("%c", txtFile[x]);
}

fclose (ifp);
}

[james of tucson]

to********@gmai l.com wrote:

Here is a code I am writing. I keep geting segmentation fault.
I'm not sure what i'm doing wrong.
Can you explain it to me in PLAIN ENGLISH??

char txtFile[FILELENGTH];

text = malloc(FILELENG TH * sizeof(char)); **I keep
getting Segmentation fault here**

Well, you didn't define FILELENGTH for us, for one thing.

How do you know your segfault is exactly here? Did you single-step
through this in your debugger?

[Keith Thompson]

to********@gmai l.com writes:

Here is a code I am writing. I keep geting segmentation fault.
I'm not sure what i'm doing wrong.
Can you explain it to me in PLAIN ENGLISH??

You didn't show us a complete program, so we can only offer limited
help. We can't see the definitions of FILELENGTH and FILES, and we
have no idea what arguments you're passing to ReadString.

void ReadString (char *filename, int *lengthPtr, char *textFilePtr)
{
FILE *ifp;
char *text;
char txtFile[FILELENGTH];

char c;
int x = 0, y = 0, items;

text = txtFile;

Here you make text point to the beginning of the aray txtFile ...

printf ("Enter the name of the file to analyze : ");
fgets (filename, FILES, stdin);

This will fail (invoke undefined behavior) if filename doesn't point
to an array of sufficient size. You're implicitly requiring the
caller to ensure that filename points to such an array; we can't tell
whether it does so. It looks like FILES is the size in bytes of the
array that filename points to; if so, FILES is a misleading name.

printf ("%s", filename);

ifp = fopen(filename, "r");

text = malloc(FILELENG TH * sizeof(char)); **I keep
getting Segmentation fault here**

.... and here you overwrite the value of text. Reconsider your design.

How do you know that this is where the segmentation fault occurs?

sizeof(char) is 1 by definition. Change the line to either
text = malloc(FILELENG TH * sizeof *txt);
or, if text is always going to be a character pointer:
text = malloc(FILELENG TH);

malloc() returns a null pointer to indicate that the allocation
failed. *Always* check for this. (Anyone who tells you that it's
sometimes not necessary is wrong.)

If malloc() actually dies with a segmentation fault rather than
reporting a failure by returning a null pointer value, then chances
are the error is somewhere else in your program. You may have done
something else that invoked undefined behavior and messed up the
internal data structures used by malloc() and free(). This is part of
the reason why we need to see a complete, self-contained program that
exhibits the problem. If you knew where the probelm is, you wouldn't
need to ask us for help; since you don't know, you can't be sure that
the code fragment you've shown us is what's actually causing the
problem.

if(ifp == NULL)
{
fprintf (stderr, "Error opening file\n");
exit (-2);
}

I'm glad to see you're checking whether fopen() failed. As a matter
of style, I'd do so immediately after the call; if it fails, there's
no point in doing the malloc() call.

exit(-2) is not portable. The only portable arguments to exit() are
0, EXIT_SUCCESS, and EXIT_FAILURE. In this case, you want
exit(EXIT_FAILU RE). (Better yet, this function should probably use
its result to report the error to its caller rather than killing the
entire program.)

while (fscanf (ifp, "%c", &c)!= EOF)
{
sprintf (txtFile, "%c", c);
y++;
}

for (x = 0; x < FILELENGTH; x++);
{
printf ("%c", txtFile[x]);
}

fscanf(), sprintf(), and printf() are overkill here. In the fscanf()
call, you're just reading a single character at a time; just use
fgetc(). The sprintf() call writes/assigns as single character into
txtFile. Furthermore, it repeatedly assigns successive characters to
the same location; is that what you want? The printf() call can be
replaced with a simpler putchar() call.

fclose (ifp);
}

--
Keith Thompson (The_Other_Keit h) ks***@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <* <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.

[Jens Thoms Toerring]

to********@gmai l.com wrote:

Here is a code I am writing. I keep geting segmentation fault.
I'm not sure what i'm doing wrong.
Can you explain it to me in PLAIN ENGLISH??

void ReadString (char *filename, int *lengthPtr, char *textFilePtr)

What are 'lengthPtr' and 'textFilePtr' for? They aren't used anywhere
in this function.

{
FILE *ifp;
char *text;
char txtFile[FILELENGTH];

Where's FILELENGTH defined (and what to)?

char c;
int x = 0, y = 0, items;

What's 'items' good for? It's never used.

text = txtFile;

printf ("Enter the name of the file to analyze : ");
fgets (filename, FILES, stdin);

Where is FILES defined (and what to)? Is what 'filename' is pointing
to memory you "own" (i.e. it's either an array of chars you defined
in the caller or is memeory you received from malloc() or friends)
and has it at least room for FILES characters?

Are you aware that, if the file name isn't longer than (FILES - 2),
there's going to be a trailing newline character, which rather likely
isn't in the name of the file you want to open?

printf ("%s", filename);

ifp = fopen(filename, "r");

text = malloc(FILELENG TH * sizeof(char)); **I keep
getting Segmentation fault here**

If you get a segentation fault here then you must have had some error
when dealing with memory somewhere else in your program, probably
accidentally overwriting some of the bookkeeping information the
malloc()-family of functions needs to store. One possible place is
where you use fgets() above (if 'filename' isn't pointing to memory
large enough to hold FILES characters - but it also could be
happening in some other part of the program you're not showing.
And debugging something one can't see is a very difficult task;-)

Moreover, why made you 'text' first point to the 'txtFile' array and
now suddenly assign a different value to it? Moreover, what's the
malloc() good for at all? The 'text' variable is never used in the
following, so you simply can delete this line - all you do here is
create a memory leak since you don't even bother to free() the memory
later on.

if(ifp == NULL)
{
fprintf (stderr, "Error opening file\n");
exit (-2);
}

while (fscanf (ifp, "%c", &c)!= EOF)
{
sprintf (txtFile, "%c", c);

Do you really want to overwrite the first character of the 'txtFile'
array again and again? I have some inkling that you actually meant
to write

sprintf( txtFile + y, "%c", c);

But then you need to make sure that you never try to read more than
FILELENGTH characters from the file, even if the file is longer
than that...

y++;

I would recommend to use variable names that make a bit more sense,
here e.g. 'read_count' would help to make things easier to under-
stand.

}

for (x = 0; x < FILELENGTH; x++);
{
printf ("%c", txtFile[x]);

With your version (i.e. using only the first element of the
'txtFile' array) everything beside the first character you print
out will be garbage. But even if you correct that as pointed out
above and the number of chars you could read from the file was
less than FILELENGTH chars then you will still output garbage if
'x' beomes equal or larger than 'y'

}

fclose (ifp);
}

The whole function looks like you have started with something that
didn't work and then you went on fiddling with the code without
understanding why it didn't work. Now it's in a state that there's
one strange thing after another and it's going to be hard to untangle
the mess. I would recommend that you just comment it out, take a
deep breath, write down (in words) exactly what the function is
supposed to do and then start again from scratch, not looking at
what you already have written. Also write a minimal program that
just calls your function, nothing else, so you can test it without
being led astray by possible errors in other parts of the program.
If this new version doesn't work don't go messing around with it
more or less randomly but come back here and let's have a look
(and also post what you wrote down the function should do and the
short program you embedded it in to test it).

Best regards, Jens
--
\ Jens Thoms Toerring ___ [e-mail]
\______________ ____________ http://toerring.de

[Mark McIntyre]

On 5 Nov 2006 12:12:54 -0800, in comp.lang.c , to********@gmai l.com
wrote:

char *text;
char txtFile[FILELENGTH];

here you create a pointer and an array.

text = txtFile;

here you point the pointer AT the array.

printf ("Enter the name of the file to analyze : ");
fgets (filename, FILES, stdin);

here you read something into "filename". You don't check for overflow.

ifp = fopen(filename, "r");

.... nor do you check that you could open the file.

>
text = malloc(FILELENG TH * sizeof(char)); **I keep
getting Segmentation fault here**

here you throw away your previous assignment to text, and attempt to
malloc some memory. Other than the needless sizeof(char) [ char always
has size one, so you don't need that] this looks ok.

I expect the bug is somewhere else, your memory has already been
corrupted. My guess would be that filename is not large enough, or you
didn't allocatn any memory for it.

if(ifp == NULL)

why not do this BEFORE allocating memory?

while (fscanf (ifp, "%c", &c)!= EOF)
{
sprintf (txtFile, "%c", c);

this repeatedy overwrites the first character of txtFile.

for (x = 0; x < FILELENGTH; x++);
{
printf ("%c", txtFile[x]);

but here you try to print out all of them.
--
Mark McIntyre

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it."
--Brian Kernighan

[Christopher Benson-Manica]

to********@gmai l.com wrote:

void ReadString (char *filename, int *lengthPtr, char *textFilePtr)
{
FILE *ifp;
char *text;
char txtFile[FILELENGTH];

char c;
int x = 0, y = 0, items;

text = txtFile;

printf ("Enter the name of the file to analyze : ");
fgets (filename, FILES, stdin);

I do hope that FILES is something reasonable.

printf ("%s", filename);

ifp = fopen(filename, "r");

text = malloc(FILELENG TH * sizeof(char)); **I keep
getting Segmentation fault here**

1) sizeof char is always 1; no need to obfuscate things here. At
least consider

text = malloc( FILELENGTH * sizeof *text );

2) Segmentation faults in malloc() are usually a good indication that
you have done something wrong elsewhere in your program, including
such things as freeing a pointer twice. If you can post a complete
(!) program that demonstrates the problem, we might be able to find it
for your. (Make that a SMALL complete program.)

3) What if malloc() fails and returns a null pointer? (hint: Checking
is a good idea.)

4) The fact that you're assigning text a malloc()'ed value after
assigning text to a character array (txtFile) suggests that you are
either misunderstandin g something or didn't post your actual code.

if(ifp == NULL)
{
fprintf (stderr, "Error opening file\n");
exit (-2);
}

Why not check before you do call malloc()? It won't matter on a
reasonable implementation, but there's no reason not to avoid malloc()
if it makes sense to do so.

while (fscanf (ifp, "%c", &c)!= EOF)

Whoa! Why not use fgetc()?

{
sprintf (txtFile, "%c", c);

Or, possibly better yet, fgets()?

}

for (x = 0; x < FILELENGTH; x++);
{
printf ("%c", txtFile[x]);

If you can't trust txtFile to be a string you can simply print with
puts() or somesuch, or at least not character-by-character, perhaps
you should open your file in binary mode.

}

fclose (ifp);
}

--
C. Benson Manica | I *should* know what I'm talking about - if I
cbmanica(at)gma il.com | don't, I need to know. Flames welcome.