read line function outputs weird results

Discussion in 'C Programming' started by WStoreyII, Feb 14, 2007.

  1. WStoreyII

    WStoreyII Guest

    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);

    }

    }
    WStoreyII, Feb 14, 2007
    #1
    1. Advertising

  2. WStoreyII

    Ian Collins Guest

    WStoreyII wrote:
    > 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.

    --
    Ian Collins.
    Ian Collins, Feb 14, 2007
    #2
    1. Advertising

  3. "Ian Collins" <> schrieb im Newsbeitrag
    news:...
    > WStoreyII wrote:
    >> 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.

    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.
    Joachim Schmitz, Feb 14, 2007
    #3
  4. WStoreyII

    santosh Guest

    WStoreyII wrote:
    > 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.
    santosh, Feb 14, 2007
    #4
  5. WStoreyII

    pete Guest

    WStoreyII wrote:
    >
    > 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;
    }
    --
    pete
    pete, Feb 14, 2007
    #5
  6. WStoreyII

    WStoreyII Guest

    On Feb 14, 5:06 am, pete <> wrote:
    > WStoreyII wrote:
    >
    > > 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;}
    >
    > --
    > pete



    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.
    WStoreyII, Feb 14, 2007
    #6
  7. WStoreyII wrote:

    > 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().
    Christopher Layne, Feb 14, 2007
    #7
  8. 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.

    --
    Richard Heathfield
    "Usenet is a strange place" - dmr 29/7/1999
    http://www.cpax.org.uk
    email: rjh at the above domain, - www.
    Richard Heathfield, Feb 14, 2007
    #8
  9. WStoreyII

    pete Guest

    WStoreyII wrote:

    > 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.

    --
    pete
    pete, Feb 14, 2007
    #9
  10. WStoreyII

    Flash Gordon Guest

    Joachim Schmitz wrote, On 14/02/07 09:45:
    > "Ian Collins" <> schrieb im Newsbeitrag
    > news:...
    >> WStoreyII wrote:
    >>> 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.

    > 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).
    --
    Flash Gordon
    Flash Gordon, Feb 14, 2007
    #10
  11. WStoreyII

    CBFalconer Guest

    WStoreyII wrote:
    >

    .... 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
    CBFalconer, Feb 14, 2007
    #11
  12. On 14 Feb 2007 05:46:37 -0800, "WStoreyII" <>
    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
    Barry Schwarz, Feb 15, 2007
    #12
  13. WStoreyII

    WStoreyII Guest

    On Feb 14, 9:50 am, CBFalconer <> wrote:
    > WStoreyII wrote:
    >
    > ... 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



    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.
    WStoreyII, Feb 15, 2007
    #13
  14. WStoreyII said:

    > On Feb 14, 9:50 am, CBFalconer <> wrote:
    >> WStoreyII wrote:
    >>
    >> ... 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.

    >
    > 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.

    --
    Richard Heathfield
    "Usenet is a strange place" - dmr 29/7/1999
    http://www.cpax.org.uk
    email: rjh at the above domain, - www.
    Richard Heathfield, Feb 15, 2007
    #14
  15. WStoreyII

    CBFalconer Guest

    WStoreyII wrote:
    > CBFalconer <> wrote:
    >> WStoreyII wrote:
    >>
    >> ... 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.

    >
    > 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
    CBFalconer, Feb 15, 2007
    #15
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Hugo
    Replies:
    10
    Views:
    1,289
    Matt Humphrey
    Oct 18, 2004
  2. kaushikshome
    Replies:
    4
    Views:
    749
    kaushikshome
    Sep 10, 2006
  3. scad
    Replies:
    23
    Views:
    1,136
    Alf P. Steinbach
    May 17, 2009
  4. John Smith
    Replies:
    2
    Views:
    111
    jake kaiden
    Apr 20, 2011
  5. Replies:
    4
    Views:
    76
    Tad McClellan
    Nov 27, 2005
Loading...

Share This Page