Memory leak?!

nyk · 01-19-2005, 08:09 AM

I wrote a small program to take photos with a canon camera at certain intervals. Now, I want it to run for about a year, but after it runs for a day or two, it occupies about 250MB of memory!
But I don't know how it's possible, the program frees all the memory it used when it waiting to take the next picture... Does anyone see the memory leak in this code?

Code:

/*********************************************************
 *
 * Captures images from a CANON IXUS in remote control mode.
 * Written January 2004 by Nick Fankhauser.
 *
 * Based on CARMEN Canon module by Mike Montemerlo
 *
 ********************************************************/
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include "canon.h"
#define MAXDIG 6
 
int main(int argc, char *argv[]) {
        usb_dev_handle *ixus;FILE *ifile;
        unsigned char **image;int *image_length;
        unsigned char **thumb;int *thumb_length;
        FILE *ifp;int exist=1;
        int run=1,err,cnt=0,ch=0,interval,k;
        char fn[30],zahl[10],zahlo[10];
        if (argc<3) {printf("Syntax: ixus_capture <prefix> <interval [s]>\n");exit(1);}
        interval=atoi(argv[2]);printf("Interval: %d\n",interval);
        strcpy(fn,"test/test1.jpg");
// find resume number
        while (exist) {
                cnt++;strcpy(fn,argv[1]);strcat(fn,"/");strcat(fn,argv[1]);
                sprintf(zahl,"%d",cnt);strcpy(zahlo,"");
                for (k=0;k<MAXDIG-strlen(zahl);k++) {strcat(zahlo,"0");}
                strcat(zahlo,zahl);strcat(fn,zahlo);strcat(fn,".jpg");
                if ((ifp=fopen(fn,"r"))==NULL) {exist=0;} else {fclose(ifp);}
        }
        printf("Resuming at: %s\n",fn);cnt--;
// end of find resume number
        if (mkdir(argv[1],0777)) {printf("Directory %s exists...\n",argv[1]);}
        ixus=canon_open_camera();if (ixus==NULL) {exit(1);}
        canon_initialize_camera(ixus);canon_rcc_init(ixus);
        canon_initialize_capture(ixus,FULL_TO_PC,FLASH_OFF);
        while (run) {
                image=malloc(1000000);thumb=malloc(100000);
                image_length=malloc(sizeof(int));thumb_length=malloc(sizeof(int));
                err=canon_capture_image(ixus,thumb,thumb_length,image,image_length);
                if (!err==0) {run=0;interval=0;}
                cnt++;strcpy(fn,argv[1]);strcat(fn,"/");strcat(fn,argv[1]);
                sprintf(zahl,"%d",cnt);strcpy(zahlo,"");
                for (k=0;k<MAXDIG-strlen(zahl);k++) {strcat(zahlo,"0");}
                strcat(zahlo,zahl);strcat(fn,zahlo);strcat(fn,".jpg");
                ifile=fopen(fn,"w");
                if (ifile==NULL) {printf("Failed to create %s!\n",fn);exit(1);}
                printf("Writing %d bytes to %s (E%d)...\n",*image_length,fn,err);
                fwrite(*image,*image_length,1,ifile);fclose(ifile);
                free(image);free(image_length);free(thumb);free(thumb_length);
                for (k=0;k<interval;k++) {
                        sleep(1);
// exit if /tmp/ixusterm exists
                        if ((ifp=fopen("/tmp/ixusterm","r"))!=NULL) {run=0;k=interval;}
                }
        }
        canon_stop_capture(ixus);canon_rcc_exit(ixus);
        canon_close_camera(ixus);return 0;
}

jailbait · 01-19-2005, 11:20 AM

"if (ifile==NULL) {printf("Failed to create %s!\n",fn);exit(1);}"

If you take this exit then you have a memory leak.

----------------------------
Steve Stites

jlliagre · 01-19-2005, 11:35 AM

Doesn't look there's an obvious leak in your code, what about the canon library you're using ?

Hko · 01-19-2005, 11:42 AM

Quote:

Originally posted by jailbait
"if (ifile==NULL) {printf("Failed to create %s!\n",fn);exit(1);}"

If you take this exit then you have a memory leak.

While not un- true what you say here, it does not matter because you're exiting anyway.

Hko · 01-19-2005, 12:26 PM

You seem to use some canon-API I don't know about, but I think you should not declare image and thumb as char **. Also, image_length and thumb_length should not be pointers, but regular int's. Then pass the adresses (with prefix '&') to the canon-API functions.

Like this: (changed lines in red)

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include "canon.h"
#define MAXDIG 6
 
int main(int argc, char *argv[]) {
        usb_dev_handle *ixus;FILE *ifile;
        unsigned char *image;int image_length;
        unsigned char *thumb;int thumb_length;
        FILE *ifp;int exist=1;
        int run=1,err,cnt=0,ch=0,interval,k;
        char fn[30],zahl[10],zahlo[10];
        if (argc<3) {printf("Syntax: ixus_capture <prefix> <interval [s]>\n");exit(1);}
        interval=atoi(argv[2]);printf("Interval: %d\n",interval);
        strcpy(fn,"test/test1.jpg");
// find resume number
        while (exist) {
                cnt++;strcpy(fn,argv[1]);strcat(fn,"/");strcat(fn,argv[1]);
                sprintf(zahl,"%d",cnt);strcpy(zahlo,"");
                for (k=0;k<MAXDIG-strlen(zahl);k++) {strcat(zahlo,"0");}
                strcat(zahlo,zahl);strcat(fn,zahlo);strcat(fn,".jpg");
                if ((ifp=fopen(fn,"r"))==NULL) {exist=0;} else {fclose(ifp);}
        }
        printf("Resuming at: %s\n",fn);cnt--;
// end of find resume number
        if (mkdir(argv[1],0777)) {printf("Directory %s exists...\n",argv[1]);}
        ixus=canon_open_camera();if (ixus==NULL) {exit(1);}
        canon_initialize_camera(ixus);canon_rcc_init(ixus);
        canon_initialize_capture(ixus,FULL_TO_PC,FLASH_OFF);
        while (run) {
                image=malloc(1000000);thumb=malloc(100000);
                /* REMOVE image_length=malloc(sizeof(int));thumb_length=malloc(sizeof(int)); */
                err=canon_capture_image(ixus, &thumb, &thumb_length, &image, &image_length);
                if (!err==0) {run=0;interval=0;}
                cnt++;strcpy(fn,argv[1]);strcat(fn,"/");strcat(fn,argv[1]);
                sprintf(zahl,"%d",cnt);strcpy(zahlo,"");
                for (k=0;k<MAXDIG-strlen(zahl);k++) {strcat(zahlo,"0");}
                strcat(zahlo,zahl);strcat(fn,zahlo);strcat(fn,".jpg");
                ifile=fopen(fn,"w");
                if (ifile==NULL) {printf("Failed to create %s!\n",fn);exit(1);}
                printf("Writing %d bytes to %s (E%d)...\n", image_length, fn,err);
                fwrite(image, image_length, 1, ifile);fclose(ifile);
                free(image); free(thumb); /* removed free(int's) */
                for (k=0;k<interval;k++) {
                        sleep(1);
// exit if /tmp/ixusterm exists
                        if ((ifp=fopen("/tmp/ixusterm","r"))!=NULL) {run=0;k=interval;}
                }
        }
        canon_stop_capture(ixus);canon_rcc_exit(ixus);
        canon_close_camera(ixus);return 0;
}

What (I think) happens with the canon-functions, is that their prototypes have char ** and int *. But the idea is not to declare the variables you want to use for that as char ** and int *.

Why?

These canon-functions need a chunk of memory from your program to store the image in. But in case the memory-chunk you malloc()-ed is not big enough, those functions provide a mechanism to re-allocate memory to get more space (there actually is a standard realloc() library function).

When this reallocation of your memory chunk happens, the address (the pointer itself) changes (may depend on the operating system). Also, obviously the lenght changes. And from that point your program needs to work with the new pointer(s) and length's. So the canon function needs to be able to change the pointer itself, and the changed length's of the re-allocated buffers. In order to be able to change those, it needs the addresses of them: i.e. the address of your actual pointers, and the adress of the int's. That's why the prototype specifies pointer-to-pointer's and pointer-to-int's.

So, with your program, for as long as your pictures fit inside the 1 million bytes you allocated ( "image=malloc(1000000)" ), everything may run fine. But when 1000000 bytes is not enough, the canon-function will reallocate the memory and change the pointer and length accordingly. But the new pointer and int will be lost when the function returns: the pointer inside your program will still be the old one. And when you free() it, you will free the wrong memory-chunk (which might have caused segfaults as well), leaving the new memory-chunk dangling in memory, not to be recovered until your program exits....

hope this helps.

nyk · 01-19-2005, 06:20 PM

Thanks a lot for your detailed reply! I will try all this tomorrow, it very late night here...

just in case you want to have a look at the canon.c from Mike Monemerlo, it's in that package:
http://www.nyk.ch/ixus_capture.tar.bz2

For a start, I'll try make the buffer twice as big and look how it affects the memory leak till tomorrow. Although the outputted file is never bigger that 1meg, it reaches 950kb, so with some overhead it could get bigger than the buffer.

Ok, tried your changed version now, it works perfectly!! Thanks a lot for explaining this to me!
I had problems understanding which function needs &, * or nothing before the ints...

Then the easiest thing was just to allocate what I thought to be enough memory, but maybe it wasn't.
So I hope now it won't memory leak and it shouldn't unless there's something wrong with the canon module.

Hko · 01-20-2005, 09:29 AM

I had a look at "canon.c", and if it looks like it does not do realloc(), as I guessed, but it does the entire allocation by itself (with calloc()) ! This means you should not do any allocation yourself in your program, otherwise you'll still have a leak, as the canon-capture function will always allocate memory for you. Thus changing your pointer, and leaving the memory that you allocated hanging around.

You'll only have to free() the image and thumbnail pointers yourself, but only when there was no error. (hen there's an error the canon function will free it.)

So here the next version (

)
(note: modified from the "ixus_capture.c" on your web-site)

Code:

int main(int argc, char *argv[]) {
    usb_dev_handle *ixus;FILE *ifile;
    unsigned char *image;int image_length;
    unsigned char *thumb;int thumb_length;
    int run=1,err,cnt=0,ch=0,interval,k;
    char fn[30],zahl[10],comm[50],zahlo[10];

    if (argc<3) {printf("Syntax: ixus_capture <prefix> <interval [s]>\n");exit(1);}
    interval=atoi(argv[2]);printf("Interval: %d\n",interval);
    printf("-> Press q to quit!\n");
    ixus=canon_open_camera();if (ixus==NULL) {exit(1);}
    canon_initialize_camera(ixus);canon_rcc_init(ixus);
    canon_initialize_capture(ixus,FULL_TO_PC,FLASH_OFF);
    tcgetattr(0,&orig);new=orig;new.c_lflag&=~ICANON;
    new.c_lflag&=~ECHO;new.c_lflag&=~ISIG;
    new.c_cc[VMIN]=1;new.c_cc[VTIME]=0;
    tcsetattr(0, TCSANOW, &new);
    mkdir(argv[1],0777);
    while (run) {
        err=canon_capture_image(ixus,&thumb,&thumb_length,&image,&image_length);
        if (err < 0) {  /* Error */
            run=0;      /* Make loop stop */
        } else {
            /* Only do this part when there was no error.
             * Otherwise the free() calls may segfault.
             */
            cnt++;strcpy(fn,argv[1]);strcat(fn,"/");strcat(fn,argv[1]);
            sprintf(zahl,"%d",cnt);strcpy(zahlo,"");
            for (k=0;k<MAXDIG-strlen(zahl);k++) {strcat(zahlo,"0");}
            strcat(zahlo,zahl);strcat(fn,zahlo);strcat(fn,".jpg");
            ifile=fopen(fn,"w");
            if (ifile==NULL) {printf("Failed to create %s!\n",fn);exit(1);}
            printf("Writing %d bytes to %s (E%d)...\n",image_length,fn,err);
            fwrite(image,image_length,1,ifile);fclose(ifile);
            free(image);free(image_length);
            for (k=0;k<interval;k++) {
                sleep(1);
                if(kbhit()) {
                    ch=readch();
                    if (ch=='q') {k=interval;run=0;}
                }
            }
        }
    }
    tcsetattr(0,TCSANOW, &orig);
    canon_stop_capture(ixus);canon_rcc_exit(ixus);
    canon_close_camera(ixus);return 0;
}

nyk · 01-22-2005, 01:30 PM

Thanks again for your help!

You ment to write free
free(image);free(thumb);
and not
free(image);free(image_length);
in the modifies version, I guess?

Like this it's running very well and doesn't permanently use memory.

BTW: it takes pictures here: http://balkon.mine.nu

Dark_Helmet · 01-22-2005, 03:12 PM

Interesting project, but HOLY SHNIKEYS!

I just did some basic number crunching, and you'll be chewing through your storage space in a hurry.

Assuming one picture is 640x480 @ 16 bits per pixel = 600 KiB per picture
Note: I realize the camera will take the pictures and compress them with jpeg. So this is off... unless maybe the increased size/colors offset the compression

To cover the span of a year, that would be: 1 picture per 5 minutes * 60 minutes per hour * 24 hours per day * 365 days per year = 105,120 pictures

Total storage space consumed: 600 KiB per picture * 105,120 pictures = ~60 GiB

Granted, you could probably reduce that by 1/3 if you can intelligently filter out all the night shots. I just saw that the computer had an 80 GiB hard drive, and was curious if it would be able to handle it.

BTW, in the picture, is the computer suspended in that black bag (over the rail)? That's probably not good for the fans/termperature especially if it gets hot during the summer. Just a "heads-up" in case you hadn't considered it.

Still, like I said, it's a very interesting project; homemade time-lapse photography.

Hko · 01-23-2005, 10:14 AM

Quote:

Originally posted by nyk
Thanks again for your help!

You're welcome.

Quote:

You ment to write free
free(image);free(thumb);
and not
free(image);free(image_length);
in the modifies version, I guess?

Yes. That was my mistake.

Quote:

BTW: it takes pictures here: http://balkon.mine.nu

Nice idea.
But like Dark_Helmet already said, there are going to be a very large number of files on your server. I checked the directory http://balkon.mine.nu/pics/ and stopped loading it because it's quite large already...

nyk · 01-24-2005, 07:08 PM

I hope I'll be able to handle the storage of all these images... The 80GB should strore about 500 days I guess.. I use 115megs on average each day. If not I can strore some of them on an other server at work. Or delete the night photos, but jpg compresses them to 10% of day size anyway.
And for the problems with heat I hope that there is enough ventillation for the PC in the bag. There's a quite big hole in the back where all the cable go in and out. Its right on the height of the vent, so the hot air can get out. I think it will survive there, it doesn't get much sun at where its located (the dark backjard) anyway...
And the cold is not bad for it, as far as I know, red an article about scientist installing a iridium-phone pc webcam on southpole. Humidity could be fatal, but as the hardware is constantly warm it shouldn't condensate on it too much....