read line function outputs weird results

[WStoreyII]

the following code is supposed to read a whole line upto a new line
char from a file. however it does not work. it is producing weird
results. please help. I had error checking in there for mallocs and
ect, but i removed them to help me debug. thanks.

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

void freadl ( FILE *stream, char **string ) {

char next = fgetc ( stream );
char *line;

line = (char*) malloc (1*sizeof(char));

while ( next != '\n' ) {

line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );
line [strlen(line)] = next;
next = fgetc (stream );
}

*string = (char*) malloc((strlen(line))*sizeof(char));
strcpy (*string,line);
free(line);

}

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

FILE *fp;
fp = fopen ( "fread.c", "r" );

char *str;
freadl ( fp , &str );

printf ( "str = %s, with size of %d\n", str, strlen (str) );

int i;

for ( i = 0; i < strlen (str); i++ ){

char now = str;
printf ( "chr = %c, # = %d\n", now,now);

}

}

 

[Ian Collins]

the following code is supposed to read a whole line upto a new line
char from a file. however it does not work. it is producing weird
results. please help. I had error checking in there for mallocs and
ect, but i removed them to help me debug. thanks.

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

void freadl ( FILE *stream, char **string ) {

char next = fgetc ( stream );
char *line;

line = (char*) malloc (1*sizeof(char));

Why oh why are people still adding these extraneous casts? Not only
that, but sizeof(char) is, buy definition 1.

while ( next != '\n' ) {

line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );

You are calling strlen on an array of char that isn't null terminated.

line [strlen(line)] = next;

You are calling strlen on an array of char that isn't null terminated.

next = fgetc (stream );
}

*string = (char*) malloc((strlen(line))*sizeof(char));

You are calling strlen on an array of char that isn't null terminated.

 

[Joachim Schmitz]

Why oh why are people still adding these extraneous casts? Not only
that, but sizeof(char) is, buy definition 1.

For several reasons: it is mentiond in lots of books about C and it is
required by C++
I admit that neither is a good reason (the books are just wrong and C++ is
OT here), but at least understandable. And a minor issue, there are more
important things to moan about, won't you think?

Bye, Jojo.

 

[santosh]

the following code is supposed to read a whole line upto a new line
char from a file. however it does not work. it is producing weird
results. please help. I had error checking in there for mallocs and
ect, but i removed them to help me debug. thanks.

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

void freadl ( FILE *stream, char **string ) {

char next = fgetc ( stream );

fgetc() returns an int value. This is because, it has no way of
indicating failure within the range of a char value, since it could be
a legal. Hence it returns an out-of-band value, represented
symbolically as EOF. So you should test each invocation of fgetc(),
getc(), getchar() etc. like:

int next;
if((next = fgetc(stream)) == EOF) /* handle error */

or something similar.

char *line;

line = (char*) malloc (1*sizeof(char));

The casts are not needed in C. Do:
line = malloc(1 * sizeof *line);

while ( next != '\n' ) {

line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );

There're multiple error here. First, if realloc() happens to fail, you
overwrite the address of the original buffer with a null pointer
value, thus loosing access to it. You should always backup the address
of the buffer before attempting to reallocating it. Secondly, as
above, the cast is once again uneccessary.

Now coming to the main mistake. You're passing a unbounded array to
strlen(). strlen() is used to find the length of C strings and you
must supply it a nul terminated array. While all strings are arrays,
all arrays are not strings, i.e. nul terminated, as in this case.

I suggest increasing the buffer by a factor of two. You must keep
track of the size of the buffer yourself, presumably with a size_t
object.

line [strlen(line)] = next;

Same mistake. The buffer you're using is *not* storing a string.
You're just reading a sequence of characters. strlen() is the wrong
function to use. You have to keep track of the buffer length manually.

next = fgetc (stream );
}

*string = (char*) malloc((strlen(line))*sizeof(char));

No need for an unneccessary allocation here. Dynamically allocated
objects persist until they're free()'ed or until program termination.
Hence, all you need to do is to pass a pointer to the actual buffer
back to the calling function, along with the size of the line read
into the buffer.

strcpy (*string,line);
free(line);

}

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

FILE *fp;
fp = fopen ( "fread.c", "r" );

Check the return values of standard library functions. Just going
ahead assuming success is asking for difficult to find bugs.

char *str;
freadl ( fp , &str );

printf ( "str = %s, with size of %d\n", str, strlen (str) );

int i;

for ( i = 0; i < strlen (str); i++ ){

char now = str;
printf ( "chr = %c, # = %d\n", now,now);

}

}

The main mistake you're making is assuming that you can find out the
length of any object with strlen(). The latter only works on objects
terminated by a nul character, '\0'. For other objects, you'll have to
track their length by other methods.

 

[pete]

the following code is supposed to read a whole line upto a new line
char from a file. however it does not work. it is producing weird
results. please help. I had error checking in there for mallocs and
ect, but i removed them to help me debug. thanks.

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

void freadl ( FILE *stream, char **string ) {

char next = fgetc ( stream );
char *line;

line = (char*) malloc (1*sizeof(char));

while ( next != '\n' ) {

line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );
line [strlen(line)] = next;
next = fgetc (stream );
}

*string = (char*) malloc((strlen(line))*sizeof(char));
strcpy (*string,line);
free(line);

}

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

FILE *fp;
fp = fopen ( "fread.c", "r" );

char *str;
freadl ( fp , &str );

printf ( "str = %s, with size of %d\n", str, strlen (str) );

int i;

for ( i = 0; i < strlen (str); i++ ){

char now = str;
printf ( "chr = %c, # = %d\n", now,now);

}

}

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

void freadl(FILE *stream, char **string)
{
unsigned long counter = 0;
char *line = NULL;
int next = fgetc(stream);

do {
next = fgetc(stream);
if (next == EOF) {
free(line);
break;
}
++counter;
line = realloc(line, counter + 1);
if (line == NULL) {
puts("line == NULL");
exit(EXIT_FAILURE);
}
line[counter - 1] = (char)next;
} while (next != '\n');
line[counter - 1] = '\0';
*string = malloc(strlen(line) + 1);
if (*string == NULL) {
puts("*string == NULL");
} else {
strcpy(*string, line);
}
free(line);
}

int main (void)
{
char *str;
FILE *fp = fopen("fread.c", "r");

if (fp != NULL) {
freadl(fp , &str);
printf("str = %s, with length of %u\n",
str, (unsigned)strlen(str));
} else {
puts("fp == NULL");
}
return 0;
}

 

[WStoreyII]

the following code is supposed to read a whole line upto a new line
char from a file. however it does not work. it is producing weird
results. please help. I had error checking in there for mallocs and
ect, but i removed them to help me debug. thanks.

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

void freadl ( FILE *stream, char **string ) {

char next = fgetc ( stream );
char *line;

line = (char*) malloc (1*sizeof(char));

while ( next != '\n' ) {

line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );
line [strlen(line)] = next;
next = fgetc (stream );
}

*string = (char*) malloc((strlen(line))*sizeof(char));
strcpy (*string,line);
free(line);

}

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

FILE *fp;
fp = fopen ( "fread.c", "r" );

char *str;
freadl ( fp , &str );

printf ( "str = %s, with size of %d\n", str, strlen (str) );

int i;

for ( i = 0; i < strlen (str); i++ ){

char now = str;
printf ( "chr = %c, # = %d\n", now,now);

}

}

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

void freadl(FILE *stream, char **string)
{
unsigned long counter = 0;
char *line = NULL;
int next = fgetc(stream);

do {
next = fgetc(stream);
if (next == EOF) {
free(line);
break;
}
++counter;
line = realloc(line, counter + 1);
if (line == NULL) {
puts("line == NULL");
exit(EXIT_FAILURE);
}
line[counter - 1] = (char)next;
} while (next != '\n');
line[counter - 1] = '\0';
*string = malloc(strlen(line) + 1);
if (*string == NULL) {
puts("*string == NULL");
} else {
strcpy(*string, line);
}
free(line);

}

int main (void)
{
char *str;
FILE *fp = fopen("fread.c", "r");

if (fp != NULL) {
freadl(fp , &str);
printf("str = %s, with length of %u\n",
str, (unsigned)strlen(str));
} else {
puts("fp == NULL");
}
return 0;}

thanks for all of the help. i will try these out later. as far as
the cast goes and using sizeof(char). i am new to c (obviously) and
everything that i have read so far has said to use these methods. so
as with anything there are those who are strict and those who are
not. i have actually got compiler warnings though for not having the
casts on malloc.

thanks again for your help.

 

[Christopher Layne]

thanks for all of the help. i will try these out later. as far as
the cast goes and using sizeof(char). i am new to c (obviously) and
everything that i have read so far has said to use these methods. so
as with anything there are those who are strict and those who are
not. i have actually got compiler warnings though for not having the
casts on malloc.

thanks again for your help.

BTW: Is this for an intended purpose? If not, you can just use fgets().

 

[Richard Heathfield]

WStoreyII said:

i have actually got compiler warnings though for not having the
casts on malloc.

That is almost certainly caused by one of two problems:

a) you forgot to #include <stdlib.h>

b) you're using a C++ compiler, not a C compiler

If it's a), then add the include. If it's b), you have two reasonable
courses of action: i) use a C compiler instead, or ii) write in C++
instead.

If you choose to write in C++ instead, you should probably consider
malloc to be obsolete, and use new, new[], or std::vector instead. If
you choose the C++ route, please direct further discussions thereof to
comp.lang.c++ rather than this newsgroup.

 

[pete]

i have actually got compiler warnings though for not having the
casts on malloc.

The typical cause of that warning is the failure to write
#include <stdlib.h>
prior to the call to malloc.
The compiler then assumes that malloc returns type int,
and then the warning is about attempting
to assign an integer type value to a pointer.

That's why casting the return value of malloc is bad.
It tends to conceal the real error,
which is the failure to include stdlib.h.

 

[Flash Gordon]

Joachim Schmitz wrote, On 14/02/07 09:45:

For several reasons: it is mentiond in lots of books about C and it is
required by C++
I admit that neither is a good reason (the books are just wrong and C++ is
OT here), but at least understandable. And a minor issue, there are more
important things to moan about, won't you think?

As shown by a later reply of WStoreyII's the cast was actually hiding
one of a couple of serious errors. The errors being either using a
compiler in C++ mode for C or failing to include stdlib.h when needed.
So the comment about the cast was important and has shown up a serious
error. It is because of the hiding of serious errors that most people
here recommend against casting the result of malloc (or any other cast
that is not actually required).

 

[CBFalconer]

.... snip ...
i have actually got compiler warnings though for not having the
casts on malloc.

Unless your compiler is faulty, those are probably due to failing
to #include <stdlib.h>, and have nothing to do with casts.

--
<http://www.cs.auckland.ac.nz/~pgut001/pubs/vista_cost.txt>
<http://www.securityfocus.com/columnists/423>

"A man who is right every time is not likely to do very much."
-- Francis Crick, co-discover of DNA
"There is nothing more amazing than stupidity in action."
-- Thomas Matthews

 

[Barry Schwarz]

On 14 Feb 2007 05:46:37 -0800, "WStoreyII" <[email protected]>
wrote:

snip ~ 100 lines

thanks for all of the help. i will try these out later. as far as
the cast goes and using sizeof(char). i am new to c (obviously) and
everything that i have read so far has said to use these methods. so
as with anything there are those who are strict and those who are
not. i have actually got compiler warnings though for not having the
casts on malloc.

thanks again for your help.

While quoting context is important, there is no need to quote a
hundred lines just to say thank you..


Remove del for email

 

[WStoreyII]

WStoreyII wrote:

... snip ...

Unless your compiler is faulty, those are probably due to failing
to #include <stdlib.h>, and have nothing to do with casts.

--
<http://www.cs.auckland.ac.nz/~pgut001/pubs/vista_cost.txt>
<http://www.securityfocus.com/columnists/423>

"A man who is right every time is not likely to do very much."
-- Francis Crick, co-discover of DNA
"There is nothing more amazing than stupidity in action."
-- Thomas Matthews

as you see in the original code above i did not fail to include
stdlib.h and gcc is my compiler and is one of the most popular c
compilers.

 

[Richard Heathfield]

WStoreyII said:

as you see in the original code above i did not fail to include
stdlib.h and gcc is my compiler and is one of the most popular c
compilers.

gcc definitely gets this right (does *not* produce a diagnostic message
when malloc is properly used), so I can only presume you are using gcc
in C++ mode.

 

[CBFalconer]

as you see in the original code above i did not fail to include
stdlib.h and gcc is my compiler and is one of the most popular c
compilers.

gcc is many compilers. If you don't run it correctly it could be a
C++ compiler, which is a faulty C compiler.

--
<http://www.cs.auckland.ac.nz/~pgut001/pubs/vista_cost.txt>
<http://www.securityfocus.com/columnists/423>

"A man who is right every time is not likely to do very much."
-- Francis Crick, co-discover of DNA
"There is nothing more amazing than stupidity in action."
-- Thomas Matthews