Like Tree1Likes
  • 1 Post By Subsonics

Better way to free resources on error

This is a discussion on Better way to free resources on error within the C Programming forums, part of the General Programming Boards category; Is there a better way to handle this situation? Everytime I open a file and allocate memory I check if ...

  1. #1
    Registered User
    Join Date
    Nov 2011
    Posts
    52

    Better way to free resources on error

    Is there a better way to handle this situation? Everytime I open a file and allocate memory I check if it is successful. If not then free all files and memory before I return an error.

    Code:
    int edb_initBlob(edb_blobStore *store, char *path)
    {
       store->writer.fp = fopen(path, "rb+");
       if(!store->writer.fp)
          return EDB_BLOB_IOERR;
    
       store->reader.fp = fopen(path, "rb");
       if(!store->reader.fp) {
          fclose(store->writer.fp);
          return EDB_BLOB_IOERR;
       }
    
       u32 *blobInf = malloc(sizeof(u32) * 4);
       if(!blobInf)  {
          fclose(store->writer.fp);
          fclose(store->reader.fp);
          return EDB_BLOB_NOMEM;
        }
       size_t ret = fread(blobInf, sizeof(u32), 4, store->reader.fp);
       if(ret != 4)  {
          free(blobInf);
          fclose(store->writer.fp);
          fclose(store->reader.fp);
          return EDB_BLOB_IOERR;
        }
    }
    I am thinking of using GOTO and put all my free code in one place? Tell me what you think.
    The price of reliability is the pursuit of the utmost simplicity. - Sir Charles Antony Richard Hoare,

  2. #2
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,302
    In all honesty, the best way is to use RAII, i.e. putting the cleanup in a destructor, but you'll find that you need e.g. C++ for that.

    The fact that there just is no "great" solution to the problem in C is one of the reasons to look to other languages for solutions.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  3. #3
    Registered User
    Join Date
    Mar 2009
    Posts
    344
    Ignoring the memory leak and the fact you're not returning anything in the non-error case plus the potential issues with opening the same file twice, something like this might work :

    Code:
    int edb_initBlob(edb_blobStore *store, char *path)
    {
       int rc = EDB_OK; /* whatever a good rc is */   
       size_t ret = 0;
       u32 *blobInf = NULL;
       store->writer.fp = store->reader.fp = NULL;
    
       store->writer.fp = fopen(path, "rb+");
       if(store->writer.fp)
       {
          store->reader.fp = fopen(path, "rb");
          if(store->reader.fp) 
          {
             blobInf = malloc(sizeof(u32) * 4);
             if(blobInf)  
             {
    	    ret = fread(blobInf, sizeof(u32), 4, store->reader.fp);
    	 }
          }
       }
    
       if (!store->writer.fp || !store->reader.fp)
          rc = EDB_BLOB_IOERR;
       else if (!blobInf)
          rc = EDB_BLOB_NOMEM;
       else if (ret != 4)
          rc = EDB_BLOB_IOERR;
    
       if (rc != EDB_OK)
       {
          if (store->writer.fp)
    	 fclose(store->writer.fp);
          if (store->reader.fp)
    	 fclose(store->reader.fp);
          if (blobInf)
    	 free (blobInf); /* should be freed regardless of RC to avoid mem leak, but this matches the original code */
       }
       return rc;
    }
    Not saying it's better or worse, just a different approach that's a bit closer to a C++ RAII feel. Single entry, single exit, clean up on error before leaving.
    Last edited by KCfromNC; 04-04-2012 at 08:31 AM.

  4. #4
    Registered User
    Join Date
    Nov 2011
    Posts
    52
    Quote Originally Posted by iMalc View Post
    In all honesty, the best way is to use RAII, i.e. putting the cleanup in a destructor, but you'll find that you need e.g. C++ for that.

    The fact that there just is no "great" solution to the problem in C is one of the reasons to look to other languages for solutions.
    Still want to use C on this project just checking if there is a better way to handle this. Since there's no better solution I will just leave it like that then.

    Thanks for the info.
    The price of reliability is the pursuit of the utmost simplicity. - Sir Charles Antony Richard Hoare,

  5. #5
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,647
    Quote Originally Posted by mnd22
    Still want to use C on this project just checking if there is a better way to handle this. Since there's no better solution I will just leave it like that then.
    Well, if you really do want to go that route, then the goto method is an acceptable way of dealing with the problem.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  6. #6
    Registered User
    Join Date
    Nov 2011
    Posts
    52
    @KCfromNC
    Thank you for the tip. I will check how can I use that approach. Sorry I forgot to put the success return code I just pasted a small part of the code.

    Another question what are the potential issues on opening the same file twice? I am trying to create two FILE pointers, one for reading and writing for the same file.

    @laserlight
    Cool I will see how can I use goto on this.
    The price of reliability is the pursuit of the utmost simplicity. - Sir Charles Antony Richard Hoare,

  7. #7
    Captain Crash brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,239
    Use an integer to keep track of your "initialization state". For each resource (file, memory, etc) you initialize, increment this counter. Write a cleanup function that uses a switch statement and fall-through to correctly release the resources if something fails. You will need to package up all the resources into a struct so you can pass it into the cleanup function.

    This particular issue is THE BIGGEST reason I don't write C code anymore.
    Code:
    //try
    //{
    	if (a) do { f( b); } while(1);
    	else   do { f(!b); } while(1);
    //}

  8. #8
    Registered User
    Join Date
    Jan 2009
    Posts
    1,485
    I use goto for situations like this. I have used a macro (not by me) that prints an error message and goto the clean up section like this:

    Code:
    #define OUT_ON_NULL(p, msg) if(!p) { fprintf( stderr, "%s\n", msg); goto out; }
    
    // then at the clean up
    
    if(pointer) free(pointer);
    Lately I have switched to something like the check macro in this link: 21 Exercise 20: Zed's Awesome Debug Macros that also prints error messages if errno is set and also let me test other conditions like:

    Code:
    check( argc == 2, "Missing argument" );
    It's very important to initialize all variables at the top of the function, otherwise you might free() resources that was never allocated.
    MK27 likes this.

  9. #9
    Registered User
    Join Date
    Nov 2011
    Posts
    52
    Quote Originally Posted by Subsonics View Post
    I use goto for situations like this. I have used a macro (not by me) that prints an error message and goto the clean up section like this:

    Code:
    #define OUT_ON_NULL(p, msg) if(!p) { fprintf( stderr, "%s\n", msg); goto out; }
    
    // then at the clean up
    
    if(pointer) free(pointer);
    Lately I have switched to something like the check macro in this link: 21 Exercise 20: Zed's Awesome Debug Macros that also prints error messages if errno is set and also let me test other conditions like:

    Code:
    check( argc == 2, "Missing argument" );
    It's very important to initialize all variables at the top of the function, otherwise you might free() resources that was never allocated.
    Wow! Thank you for the link Subsonics. This will help me a lot.
    The price of reliability is the pursuit of the utmost simplicity. - Sir Charles Antony Richard Hoare,

  10. #10
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by Subsonics View Post
    I use goto for situations like this.
    Me too:

    Code:
    struct whatever *example (args) {
    
    	// validate args
    	if (!somecondition) return null;
    
    	struct whatever *rv = malloc(sizeof(struct whatever));
    
    	// do more stuff
    	if (!somecondition) goto problem1;
    
    	rv->p = malloc(xxxx);
    
    	// more stuff
    	if (!somecondition) goto problem2;
    
    	return rv; // happens if there were no problems
    
    problem2:
    	free(rv->p);
    
    problem1:
    	free(rv);
    
    	return null;
    }
    Hopefully it's clear how that works.
    Last edited by MK27; 04-05-2012 at 09:13 AM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. DEBUG ERROR WHEN USING free() ??
    By mugiwara528 in forum C Programming
    Replies: 14
    Last Post: 07-13-2011, 11:33 AM
  2. Malloc - Free giving double free or corruption error
    By andrew.bolster in forum C Programming
    Replies: 2
    Last Post: 11-02-2007, 06:22 AM
  3. Free chess openings resources
    By Zewu in forum A Brief History of Cprogramming.com
    Replies: 23
    Last Post: 02-23-2005, 10:04 AM
  4. GlobalMemoryStatus to get "system resources free&
    By hanhao in forum Windows Programming
    Replies: 0
    Last Post: 03-20-2004, 01:30 AM
  5. error when using free
    By rotis23 in forum C Programming
    Replies: 11
    Last Post: 08-21-2002, 10:27 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21