segmentation fault writing to array elements in structure

Discussion in 'C Programming' started by SP, Aug 8, 2006.

  1. SP

    SP Guest

    The following code crashes after I add the two nested FOR loops at the
    end, I am starting to learn about pointers and would like to understand
    what I'm doing wrong. I think the problem is the way I access the
    array elements.

    Thanks for your help.



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

    struct pgmimage {
    char *magic_str; /* File identifier */
    int nr, nc; /* Rows and columns in the image */
    int max_val; /* Max value of data */
    int or, oy; /* Origin */
    unsigned char **data; /* Pixel values */
    };

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

    FILE *fp;
    char line_in[3000];
    int count = 0;
    int row, col;
    struct pgmimage *pgm_in;

    if ( argc < 2 )
    {
    printf ("You need to enter a file name\n");
    printf ("\n");
    printf ("Usage: %s <file_name>\n", argv[0]);
    return 1;
    }

    fp = fopen( argv[1], "r");
    if (fp == NULL)
    {
    printf (" unable to open file: %s\n", argv[1]);
    return 1;
    }

    pgm_in = malloc(sizeof *pgm_in);
    pgm_in->magic_str = malloc(10 * sizeof *pgm_in->magic_str);

    while( fgets( line_in, 2999, fp) != NULL)
    {
    if (line_in[0] != '#')
    {
    count = count + 1;
    if (count == 1)
    {sscanf(line_in, "%s", &pgm_in->magic_str);}
    if (count == 2)
    {sscanf(line_in, "%i %i", &pgm_in->nr, &pgm_in->nc);}
    if (count == 3)
    {sscanf(line_in, "%i", &pgm_in->max_val); break;}
    }
    }

    printf("PGM Header: Magic # %s\n", &pgm_in->magic_str);
    printf("PGM Header: Rows # %i\n", pgm_in->nr);
    printf("PGM Header: Columns # %i\n", pgm_in->nc);
    printf("PGM Header: Max Value # %i\n", pgm_in->max_val);

    pgm_in->data = malloc( pgm_in->nc * pgm_in->nr * sizeof
    *pgm_in->data);
    if(!pgm_in->data)
    { printf("Memory allocation failed\n"); }

    for(row = 0; row <= pgm_in->nr-1; row++)
    for(col = 0; col <= pgm_in->nc-1;col++)
    {
    if(fscanf(fp, "%i", &pgm_in->data[row][col]) == EOF)
    {
    printf("reached EOF early at: Row:%i\n Col:%i",row,
    col);
    }
    }

    fclose (fp);
    return 0;

    }
    SP, Aug 8, 2006
    #1
    1. Advertising

  2. SP

    Dann Corbit Guest

    E:\>type blam.c
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>

    struct pgmimage {
    char *magic_str; /* File identifier */
    int nr, nc; /* Rows and columns in the image */
    int max_val; /* Max value of data */
    int or, oy; /* Origin */
    unsigned char **data; /* Pixel values */
    };

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

    FILE *fp;
    char line_in[3000];
    int count = 0;
    int row, col;
    struct pgmimage *pgm_in;

    if ( argc < 2 )
    {
    printf ("You need to enter a file name\n");
    printf ("\n");
    printf ("Usage: %s <file_name>\n", argv[0]);
    return 1;
    }

    fp = fopen( argv[1], "r");
    if (fp == NULL)
    {
    printf (" unable to open file: %s\n", argv[1]);
    return 1;
    }

    pgm_in = malloc(sizeof *pgm_in);
    pgm_in->magic_str = malloc(10 * sizeof *pgm_in->magic_str);

    while( fgets( line_in, 2999, fp) != NULL)
    {
    if (line_in[0] != '#')
    {
    count = count + 1;
    if (count == 1)
    {sscanf(line_in, "%s", &pgm_in->magic_str);}
    if (count == 2)
    {sscanf(line_in, "%i %i", &pgm_in->nr, &pgm_in->nc);}
    if (count == 3)
    {sscanf(line_in, "%i", &pgm_in->max_val); break;}
    }
    }

    printf("PGM Header: Magic # %s\n", &pgm_in->magic_str);
    printf("PGM Header: Rows # %i\n", pgm_in->nr);
    printf("PGM Header: Columns # %i\n", pgm_in->nc);
    printf("PGM Header: Max Value # %i\n", pgm_in->max_val);

    pgm_in->data = malloc( pgm_in->nc * pgm_in->nr * sizeof *pgm_in->data);
    if(!pgm_in->data)
    { printf("Memory allocation failed\n"); }

    for(row = 0; row <= pgm_in->nr-1; row++)
    for(col = 0; col <= pgm_in->nc-1;col++)
    {
    if(fscanf(fp, "%i", &pgm_in->data[row][col]) == EOF)
    {
    printf("reached EOF early at: Row:%i\n Col:%i",row,col);
    }
    }

    fclose (fp);
    return 0;

    }
    E:\>lin blam.c

    E:\>e:\lint\lint-nt +template(1) +fcp +v -ie:\lint\
    std.lnt -os(_lint.tmp) blam.c blam.c0 blam.c1 blam.c2 blam.c3
    blam.c4 blam.c5 blam.c6 b
    lam.c7 blam.c8 blam.c9 0 1 2
    PC-lint for C/C++ (NT) Ver. 7.00j, Copyright Gimpel Software 1985-1997
    --- Module: blam.c

    E:\>type _lint.tmp | more

    --- Module: blam.c
    _
    pgm_in = malloc(sizeof *pgm_in);
    blam.c(37) : Error 64: Type mismatch (assignment) (ptrs to void/nonvoid)
    _
    pgm_in->magic_str = malloc(10 * sizeof *pgm_in->magic_str);
    blam.c(38) : Error 64: Type mismatch (assignment) (ptrs to void/nonvoid)
    blam.c(38) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    {sscanf(line_in, "%s", &pgm_in->magic_str);}
    blam.c(46) : Warning 561: (arg. no. 3) indirect object inconsistent with
    format
    blam.c(46) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    {sscanf(line_in, "%i %i", &pgm_in->nr, &pgm_in->nc);}
    blam.c(48) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    blam.c(48) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    {sscanf(line_in, "%i", &pgm_in->max_val); break;}
    blam.c(50) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    printf("PGM Header: Magic # %s\n", &pgm_in->magic_str);
    blam.c(54) : Warning 559: Size of argument no. 2 inconsistent with format
    blam.c(54) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    printf("PGM Header: Rows # %i\n", pgm_in->nr);
    blam.c(55) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    printf("PGM Header: Columns # %i\n", pgm_in->nc);
    blam.c(56) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    printf("PGM Header: Max Value # %i\n", pgm_in->max_val);
    blam.c(57) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    pgm_in->data = malloc( pgm_in->nc * pgm_in->nr * sizeof *pgm_in->data);
    blam.c(59) : Info 737: Loss of sign in promotion from int to unsigned int
    blam.c(59) : Error 64: Type mismatch (assignment) (ptrs to void/nonvoid)
    blam.c(59) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    blam.c(59) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    blam.c(59) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    if(!pgm_in->data)
    blam.c(60) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    for(row = 0; row <= pgm_in->nr-1; row++)
    blam.c(63) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    blam.c(63) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    for(col = 0; col <= pgm_in->nc-1;col++)
    blam.c(64) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    blam.c(64) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    if(fscanf(fp, "%i", &pgm_in->data[row][col]) == EOF)
    blam.c(66) : Warning 561: (arg. no. 3) indirect object inconsistent with
    format
    blam.c(66) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    _
    fclose (fp);
    blam.c(72) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'
    blam.c(72) : Warning 613: Possible use of null pointer (pgm_in) in left
    argument to operator '->'

    --- Wrap-up for Module: blam.c

    Info 754: local structure member pgmimage::eek:r (line 9, file blam.c) not
    referenced
    Info 754: local structure member pgmimage::eek:y (line 9, file blam.c) not
    referenced
    Info 766: Header file 'C:\lang\VC98\include\string.h' not used in module
    'blam.c'
    Error 305: Unable to open module: blam.c0

    ---
    PC-lint for C/C++ output placed in _LINT.TMP
    E:\>
    E:\>splint blam.c
    Splint 3.0.1.6 --- 11 Feb 2002

    blam.c: (in function main)
    blam.c(38,11): Arrow access from possibly null pointer pgm_in:
    pgm_in->magic_str
    A possibly null pointer is dereferenced. Value is either the result of a
    function which may return null (in which case, code should check it is not
    null), or a global, parameter or structure field declared with the null
    qualifier. (Use -nullderef to inhibit warning)
    blam.c(37,14): Storage pgm_in may become null
    blam.c(46,39): Format argument 1 to sscanf (%s) expects char * gets char **:
    &pgm_in->magic_str
    Type of parameter is not consistent with corresponding code in format
    string.
    (Use -formattype to inhibit warning)
    blam.c(46,35): Corresponding format code
    blam.c(46,17): Return value (type int) ignored: sscanf(line_in, ...
    Result returned by function call is not used. If this is intended, can
    cast
    result to (void) to eliminate message. (Use -retvalint to inhibit warning)
    blam.c(48,16): Return value (type int) ignored: sscanf(line_in, ...
    blam.c(50,16): Return value (type int) ignored: sscanf(line_in, ...
    blam.c(54,46): Format argument 1 to printf (%s) expects char * gets char **:
    &pgm_in->magic_str
    blam.c(54,40): Corresponding format code
    blam.c(55,46): Field pgm_in->nr used before definition
    An rvalue is used that may not be initialized to a value on some execution
    path. (Use -usedef to inhibit warning)
    blam.c(56,44): Field pgm_in->nc used before definition
    blam.c(57,44): Field pgm_in->max_val used before definition
    blam.c(66,34): Index of possibly null pointer pgm_in->data: pgm_in->data
    blam.c(59,20): Storage pgm_in->data may become null
    blam.c(66,34): Value pgm_in->data[] used before definition
    blam.c(66,33): Format argument 1 to fscanf (%i) expects int * gets unsigned
    char *: &pgm_in->data[row][col]
    To make char and int types equivalent, use +charint.
    blam.c(66,29): Corresponding format code
    blam.c(66,33): Unallocated storage &pgm_in->data[][] passed as out parameter
    to
    fscanf: &pgm_in->data[row][col]
    blam.c(66,33): Attempt to set unuseable storage: pgm_in->data[][]
    blam.c(72,5): Return value (type int) ignored: fclose(fp)
    blam.c(73,14): Fresh storage pgm_in not released before return
    A memory leak has been detected. Storage allocated locally is not released
    before the last reference to it is lost. (Use -mustfreefresh to inhibit
    warning)
    blam.c(37,5): Fresh storage pgm_in allocated

    Finished checking --- 16 code warnings

    E:\>
    Dann Corbit, Aug 8, 2006
    #2
    1. Advertising

  3. On Tue, 8 Aug 2006 03:21:42 UTC, "SP" <> wrote:

    > The following code crashes after I add the two nested FOR loops at the
    > end, I am starting to learn about pointers and would like to understand
    > what I'm doing wrong. I think the problem is the way I access the
    > array elements.
    >
    > Thanks for your help.
    >

    The problem is in the bugs you've programmed in. See below.
    >
    > #include <stdio.h>
    > #include <stdlib.h>
    > #include <string.h>
    >
    > struct pgmimage {
    > char *magic_str; /* File identifier */
    > int nr, nc; /* Rows and columns in the image */
    > int max_val; /* Max value of data */
    > int or, oy; /* Origin */
    > unsigned char **data; /* Pixel values */
    > };
    >
    > int main ( int argc, char *argv[])
    > {
    >
    > FILE *fp;
    > char line_in[3000];
    > int count = 0;
    > int row, col;
    > struct pgmimage *pgm_in;
    >
    > if ( argc < 2 )
    > {
    > printf ("You need to enter a file name\n");
    > printf ("\n");
    > printf ("Usage: %s <file_name>\n", argv[0]);
    > return 1;
    > }
    >
    > fp = fopen( argv[1], "r");
    > if (fp == NULL)
    > {
    > printf (" unable to open file: %s\n", argv[1]);
    > return 1;
    > }
    >
    > pgm_in = malloc(sizeof *pgm_in);


    You have to check the result of malloc. malloc() may return a null
    pointer flagging that it can't enough memory to fullify your request.

    > pgm_in->magic_str = malloc(10 * sizeof *pgm_in->magic_str);


    Who says that malloc can>'t fail to give you enough memory?

    > while( fgets( line_in, 2999, fp) != NULL)


    This will eats up the whole file. The while as such seems to be
    superflous even as you needs to check for EOF and error.

    > {
    > if (line_in[0] != '#')
    > {
    > count = count + 1;
    > if (count == 1)
    > {sscanf(line_in, "%s", &pgm_in->magic_str);}


    Who says that you'll never can read more than 9 chars here?

    > if (count == 2)
    > {sscanf(line_in, "%i %i", &pgm_in->nr, &pgm_in->nc);}


    Who says that you got exactly 2 int?

    > if (count == 3)
    > {sscanf(line_in, "%i", &pgm_in->max_val); break;}


    Your programming style is horrible! Use at least 1 space to separate
    the brackets from anything else. Use separate lines for separate
    statements.

    > }
    > }
    >
    > printf("PGM Header: Magic # %s\n", &pgm_in->magic_str);
    > printf("PGM Header: Rows # %i\n", pgm_in->nr);
    > printf("PGM Header: Columns # %i\n", pgm_in->nc);
    > printf("PGM Header: Max Value # %i\n", pgm_in->max_val);
    >
    > pgm_in->data = malloc( pgm_in->nc * pgm_in->nr * sizeof
    > *pgm_in->data);
    > if(!pgm_in->data)
    > { printf("Memory allocation failed\n"); }
    >
    > for(row = 0; row <= pgm_in->nr-1; row++)


    Ends up in the lands of undefined behavior when pgm_in-> is 0.
    row < pgm_in->nr; is better

    > for(col = 0; col <= pgm_in->nc-1;col++)


    Ends up in the lands of undefined behavior when pgm_in->nc is 0.
    col < pgm_in_nc; is better.

    > {
    > if(fscanf(fp, "%i", &pgm_in->data[row][col]) == EOF)
    > {
    > printf("reached EOF early at: Row:%i\n Col:%i",row,
    > col);


    Reading further after printing an error seems meaningless.

    > }
    > }
    >
    > fclose (fp);
    > return 0;
    >
    > }
    >



    Use a C compiler to compile C, Don't use the C++ compiler.

    --
    Tschau/Bye
    Herbert

    Visit http://www.ecomstation.de the home of german eComStation
    eComStation 1.2 Deutsch ist da!
    Herbert Rosenau, Aug 8, 2006
    #3
  4. SP

    SP Guest

    Herbert Rosenau wrote:
    > On Tue, 8 Aug 2006 03:21:42 UTC, "SP" <> wrote:
    >
    > > The following code crashes after I add the two nested FOR loops at the
    > > end, I am starting to learn about pointers and would like to understand
    > > what I'm doing wrong. I think the problem is the way I access the
    > > array elements.
    > >
    > > Thanks for your help.
    > >

    > The problem is in the bugs you've programmed in. See below.
    > >
    > > #include <stdio.h>
    > > #include <stdlib.h>
    > > #include <string.h>
    > >
    > > struct pgmimage {
    > > char *magic_str; /* File identifier */
    > > int nr, nc; /* Rows and columns in the image */
    > > int max_val; /* Max value of data */
    > > int or, oy; /* Origin */
    > > unsigned char **data; /* Pixel values */
    > > };
    > >
    > > int main ( int argc, char *argv[])
    > > {
    > >
    > > FILE *fp;
    > > char line_in[3000];
    > > int count = 0;
    > > int row, col;
    > > struct pgmimage *pgm_in;
    > >
    > > if ( argc < 2 )
    > > {
    > > printf ("You need to enter a file name\n");
    > > printf ("\n");
    > > printf ("Usage: %s <file_name>\n", argv[0]);
    > > return 1;
    > > }
    > >
    > > fp = fopen( argv[1], "r");
    > > if (fp == NULL)
    > > {
    > > printf (" unable to open file: %s\n", argv[1]);
    > > return 1;
    > > }
    > >
    > > pgm_in = malloc(sizeof *pgm_in);

    >
    > You have to check the result of malloc. malloc() may return a null
    > pointer flagging that it can't enough memory to fullify your request.
    >
    > > pgm_in->magic_str = malloc(10 * sizeof *pgm_in->magic_str);

    >
    > Who says that malloc can>'t fail to give you enough memory?
    >
    > > while( fgets( line_in, 2999, fp) != NULL)

    >
    > This will eats up the whole file. The while as such seems to be
    > superflous even as you needs to check for EOF and error.
    >
    > > {
    > > if (line_in[0] != '#')
    > > {
    > > count = count + 1;
    > > if (count == 1)
    > > {sscanf(line_in, "%s", &pgm_in->magic_str);}

    >
    > Who says that you'll never can read more than 9 chars here?
    >
    > > if (count == 2)
    > > {sscanf(line_in, "%i %i", &pgm_in->nr, &pgm_in->nc);}

    >
    > Who says that you got exactly 2 int?
    >
    > > if (count == 3)
    > > {sscanf(line_in, "%i", &pgm_in->max_val); break;}

    >
    > Your programming style is horrible! Use at least 1 space to separate
    > the brackets from anything else. Use separate lines for separate
    > statements.
    >
    > > }
    > > }
    > >
    > > printf("PGM Header: Magic # %s\n", &pgm_in->magic_str);
    > > printf("PGM Header: Rows # %i\n", pgm_in->nr);
    > > printf("PGM Header: Columns # %i\n", pgm_in->nc);
    > > printf("PGM Header: Max Value # %i\n", pgm_in->max_val);
    > >
    > > pgm_in->data = malloc( pgm_in->nc * pgm_in->nr * sizeof
    > > *pgm_in->data);
    > > if(!pgm_in->data)
    > > { printf("Memory allocation failed\n"); }
    > >
    > > for(row = 0; row <= pgm_in->nr-1; row++)

    >
    > Ends up in the lands of undefined behavior when pgm_in-> is 0.
    > row < pgm_in->nr; is better
    >
    > > for(col = 0; col <= pgm_in->nc-1;col++)

    >
    > Ends up in the lands of undefined behavior when pgm_in->nc is 0.
    > col < pgm_in_nc; is better.
    >
    > > {
    > > if(fscanf(fp, "%i", &pgm_in->data[row][col]) == EOF)
    > > {
    > > printf("reached EOF early at: Row:%i\n Col:%i",row,
    > > col);

    >
    > Reading further after printing an error seems meaningless.
    >
    > > }
    > > }
    > >
    > > fclose (fp);
    > > return 0;
    > >
    > > }
    > >

    >
    >
    > Use a C compiler to compile C, Don't use the C++ compiler.
    >
    > --
    > Tschau/Bye
    > Herbert
    >
    > Visit http://www.ecomstation.de the home of german eComStation
    > eComStation 1.2 Deutsch ist da!


    Herbert,

    I dont quite get your point, if had one to make.
    I was going to reply to each of your comments, but that would be a
    waste of time.
    Other then nitpick you did not help to answer the original question.
    But then again you already know that.
    SP, Aug 9, 2006
    #4
  5. "SP" <> writes:
    > Herbert Rosenau wrote:
    >> On Tue, 8 Aug 2006 03:21:42 UTC, "SP" <> wrote:
    >> > The following code crashes after I add the two nested FOR loops at the
    >> > end, I am starting to learn about pointers and would like to understand
    >> > what I'm doing wrong. I think the problem is the way I access the
    >> > array elements.
    >> >
    >> > Thanks for your help.
    >> >

    >> The problem is in the bugs you've programmed in. See below.

    [code and critique snipped]
    >
    > Herbert,
    >
    > I dont quite get your point, if had one to make.
    > I was going to reply to each of your comments, but that would be a
    > waste of time.
    > Other then nitpick you did not help to answer the original question.
    > But then again you already know that.


    Your program is dying with a segmentation fault, and you don't know
    why. I haven't examined either your code or Herbert's critique in
    detail, but it seems to me that nitpicking is exactly what you need.

    Fix the bugs in your code and try again. If it still blows up, and
    you still don't know why, post again.

    --
    Keith Thompson (The_Other_Keith) <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.
    Keith Thompson, Aug 9, 2006
    #5
  6. SP wrote:
    > The following code crashes after I add the two nested FOR loops at the
    > end, I am starting to learn about pointers and would like to understand
    > what I'm doing wrong. I think the problem is the way I access the
    > array elements.




    No, the problem is you're too optimistic.

    Any time you read in a value from the outside world, you should
    check it for reasonableness.

    Anytime you multiply two numbers together, you should pre-flight the
    numbers for reasonableness. Ariane lost $400 million because they
    thought they could save a nanosecond by omitting one such if statement.


    Anytime you ask for memory, you should make sure you're asking for a
    reasonable amount.

    Even after you request it, you should check that you got it.
    Ancient_Hacker, Aug 9, 2006
    #6
  7. SP

    manmohan Guest

    :
    > SP wrote:
    > >


    pgm_in = malloc(sizeof *pgm_in);
    pgm_in->magic_str = malloc(10 * sizeof *pgm_in->magic_str);

    what i feel is the way you collect a dynamic address from malloc is a
    bit crumpy!!

    try this way..hope it helps..

    make an array of structure pointers..initialize all items like
    arr = (struct pgimage*)malloc(sizeof (structpgm_in);
    then u can basically do strcpy for magic_str
    strcpy((arr)->magic_str,filename);
    manmohan, Aug 9, 2006
    #7
  8. manmohan said:

    >
    > :
    >> SP wrote:
    >> >

    >
    > pgm_in = malloc(sizeof *pgm_in);
    > pgm_in->magic_str = malloc(10 * sizeof *pgm_in->magic_str);
    >
    > what i feel is the way you collect a dynamic address from malloc is a
    > bit crumpy!!


    What's wrong with his way? (Apart from the lack of error checking.)

    >
    > try this way..hope it helps..
    >
    > make an array of structure pointers..initialize all items like
    > arr = (struct pgimage*)malloc(sizeof (structpgm_in);


    What value does this array add? What value does the cast add? Why have you
    nailed the type name in there?

    > then u can basically do strcpy for magic_str
    > strcpy((arr)->magic_str,filename);


    Not until you've checked that the malloc for magic_str succeeded, and
    ensured that the allocated area is sufficient to store the filename.

    --
    Richard Heathfield
    "Usenet is a strange place" - dmr 29/7/1999
    http://www.cpax.org.uk
    email: rjh at above domain (but drop the www, obviously)
    Richard Heathfield, Aug 9, 2006
    #8
  9. On 7 Aug 2006 20:21:42 -0700, "SP" <> wrote:

    > The following code crashes after I add the two nested FOR loops at the


    i.e. if you start making any use of your 'data' element

    > end, I am starting to learn about pointers and would like to understand
    > what I'm doing wrong. I think the problem is the way I access the
    > array elements.


    Indeed it is.

    > struct pgmimage {


    > unsigned char **data; /* Pixel values */


    > char line_in[3000];


    > while( fgets( line_in, 2999, fp) != NULL)


    Minor point: the limit to fgets includes the null terminating byte, so
    you can use 3000 or just sizeof line_in here. OTOH, if your lines
    aren't longer than 2997 chars, it doesn't make any real difference.

    > {
    > if (line_in[0] != '#')
    > {
    > count = count + 1;
    > if (count == 1)
    > {sscanf(line_in, "%s", &pgm_in->magic_str);}
    > if (count == 2)
    > {sscanf(line_in, "%i %i", &pgm_in->nr, &pgm_in->nc);}
    > if (count == 3)
    > {sscanf(line_in, "%i", &pgm_in->max_val); break;}


    You should check the return value of sscanf() to make sure you don't
    go nuts if someone feeds you garbage. %i allows octal and hexadecimal
    input as well as decimal; I don't know if that's what you want for
    PGM; if not, use %d.

    > }
    > }
    >

    And this (first) loop has already read through your whole file, so
    there won't actually be any data available to read below (although
    your code fails before encountering that problem). You probably want
    to break out of this loop after reading the last header element/line.

    > printf("PGM Header: Magic # %s\n", &pgm_in->magic_str);
    > printf("PGM Header: Rows # %i\n", pgm_in->nr);
    > printf("PGM Header: Columns # %i\n", pgm_in->nc);
    > printf("PGM Header: Max Value # %i\n", pgm_in->max_val);
    >
    > pgm_in->data = malloc( pgm_in->nc * pgm_in->nr * sizeof
    > *pgm_in->data);
    > if(!pgm_in->data)
    > { printf("Memory allocation failed\n"); }
    >

    Here you allocate a single large block of NC * NR bytes ...

    > for(row = 0; row <= pgm_in->nr-1; row++)
    > for(col = 0; col <= pgm_in->nc-1;col++)
    > {
    > if(fscanf(fp, "%i", &pgm_in->data[row][col]) == EOF)


    And here you try to access it as a '2-level array', that is, a pointer
    to a series of pointers. It isn't. See 6.16 et seq of the FAQ at
    www.c-faq.com and (other) usual places. You need to either:

    - allocate space for NR pointers (to pointer to element, although on
    mainstream systems all pointers or at least all data pointers are the
    same size); allocate space for NR different rows of NC elements and
    store each of those in data[row]; throughout checking for failures

    - allocate space for NR pointers (to pointer to element); allocate
    space for NR * NC elements and compute and set each data[row] as
    &X[NC*0], &X[NC*1], &X[NC*2] and so on.

    - (in C89) allocate space for NR * NC elements and do the 2-dim
    subscripting yourself: data [ row * NR + col ]

    - in C99 or 'ganuck' (GNU C), use a local VLA, or a local VM type
    pointing to a dynamically-allocated variable-strided array.

    Moreover even if this correctly accessed an unsigned char, you try to
    parse an 'int' (again allowing nondecimal) and store it there, which
    (on nearly all platforms) doesn't fit. I've heard, but not checked,
    that PGM pixels can be more than a byte, in which case you should
    declare, allocate and store into a big enough type, 'int' or 'long'.
    If the values really are bytes, in C89 you must fscanf into a
    temporary int or short (the latter with %hd or %hi or unsigned %hu)
    and then copy into the byte; in C99 you can fscanf directly into a
    char with %hhd or %hhi signed or %%hu unsigned.

    And finally a style point: it is more common in C, and more quickly
    recognized by most C programmers, to run your loops 'halfopen':
    for( row = 0; row < pgm_in -> nr; row ++) instead of
    for( row = 0; row <= pgm_in -> nr - 1; row ++);

    > {
    > printf("reached EOF early at: Row:%i\n Col:%i",row,
    > col);
    > }
    > }
    >
    > fclose (fp);
    > return 0;
    >
    > }


    - David.Thompson1 at worldnet.att.net
    Dave Thompson, Aug 14, 2006
    #9
    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. utab
    Replies:
    7
    Views:
    3,373
    Dietmar Kuehl
    Mar 14, 2006
  2. Wes
    Replies:
    6
    Views:
    502
    Old Wolf
    Oct 24, 2006
  3. Replies:
    4
    Views:
    343
    Ian Collins
    May 1, 2007
  4. jbholman
    Replies:
    19
    Views:
    898
    CBFalconer
    Sep 19, 2008
  5. Replies:
    0
    Views:
    406
Loading...

Share This Page