segmentation fault writing to array elements in structure

[SP]

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;

}

 

[Dann Corbit]

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:r (line 9, file blam.c) not
referenced
Info 754: local structure member pgmimage: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:\>

 

[Herbert Rosenau]

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.

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!

 

[SP]

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.

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.

 

[Keith Thompson]

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.

 

[Ancient_Hacker]

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.

 

[manmohan]

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

 

[Richard Heathfield]

manmohan said:

:

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.

 

[Dave Thompson]

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

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

fclose (fp);
return 0;

}

- David.Thompson1 at worldnet.att.net