-1

I'm using an Artifical Neural Network library made by a researcher for which i'm doing an internship. There's a function reading pgm images and loading them into arrays. The function works just fine at first. Here's the interesting part of it:

unsigned char *tmp=(unsigned char *)calloc(width, sizeof(unsigned char));
for(int i=0;i<height;i++)
{
    int r=(int)fread(tmp, sizeof(unsigned char), width, fp);
    if(r!=width)
    {
        fprintf(stderr, "Error while reading PGM file [%s] (%d bytes read instead of %d)\n", filename, r, width);
        Del2D(height, pix);
        pix=NULL;
        free(tmp);
        fclose(fp);
        return(0);
    }
    for(int j=0;j<width;j++)
        pix[i][j]=(unsigned char)tmp[j];
}
name=strdup(filename);
fclose(fp);
free(tmp);

I do get a segmentation fault at the line

int r=(int)fread(tmp,sizeof(unsigned char), width, fp);

However, i only get this error for one pgm image. The only reason i see for that is because it's the largest one in terms of height and width (900x900). I've looked a bit about the usage of fread and it seems to me that this portion of code is fine. I've not seen any limitation about the number of characters readable at once by the function either.

Any idea?


Edit: The width parameter was equal to -1 for this particular image, but there's no reason for that since reading the width works just fine for every other image. I'm gonna guess that something's wrong with the image itself. The program works fine for 1024*1024 pgm images I found afterward so it's not a size issue. Thanks for your answers.

Fatalize
  • 3,513
  • 15
  • 25
  • Any reason for the dynamic allocation? There are stdlib facilities that free the memory automatically. – chris Jul 05 '13 at 14:48
  • I assume you found out which line it is by running in a debugger? If so, did you check the values of `tmp` or `fp` to make sure they weren't `NULL`? – Some programmer dude Jul 05 '13 at 14:48
  • I found out the line getting me the segfault by using good old `printf("TEST\n");`. fp is not `NULL`, it is checked in a part of the code above this, which is not shown – Fatalize Jul 05 '13 at 14:53
  • Is tmp NULL? What address is the SegFault saying that its trying to access? Have you tried running this in GDB and browsing the stacktrace? – George Mitchell Jul 05 '13 at 15:03
  • tmp is indeed 0 for that image. So the problem comes from calloc then I assume – Fatalize Jul 05 '13 at 15:07
  • `width` must be an insane value for the problem image. – RichieHindle Jul 05 '13 at 15:18
  • 1
    @RichieHindle is right, I can't see calloc flipping out over only 900 bytes... Do you have width as an unsigned value and force it to go "negative" somewhere else in the code? (I made this mistake a few minutes ago lol) – George Mitchell Jul 05 '13 at 16:43
  • Try to printf width before the calloc, and also within the loop before the offending fread (don't forget '\n' at the end of the printf string so it is flushed before the segfault would strike in). Also add printf checkpoints around your copy loop. Maybe this will uncover what evil could be happening in there. – Jubatian Jul 05 '13 at 20:28
  • [Don't cast the return value of memory allocators](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/605858#605858). –  Jul 08 '13 at 13:29

2 Answers2

1

Are you doing this under DOS / Windows? If so, did you remember to open the file in binary mode (ie. "rb")?

Joe Z
  • 17,413
  • 3
  • 28
  • 39
  • That's a good question, but I doubt it's related to this error. – Barmar Jul 05 '13 at 14:52
  • @Barmar It sure is important, as if it's open in text mode a newline `'\n'` may get converted to a CR-NL sequence, so suddenly the `fread` call tries to put `width + x` bytes into a buffer of only `width` bytes. – Some programmer dude Jul 05 '13 at 14:54
  • I'm under Ubuntu, so that's not it. the code is Windows-capable too and it does open the file in binary anyways – Fatalize Jul 05 '13 at 14:54
0

a) Did you actually check the return of calloc? b) Did you check that pix was allocated succesfully?

Tom Tanner
  • 9,244
  • 3
  • 33
  • 61