Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

Memory leaks in operator=

Ahnfelt
2009-03-06
2013-04-23
  • Ahnfelt
    Ahnfelt
    2009-03-06

    The memory for the existing image is not freed.

    This fix avoids reallocating memory if the dimensions haven't changed.

    I haven't checked pngwriter(pngwriter&), but that might be problematic in the same way.

    Here's a diff:

    545a546,547
    >    int original_width = width_;
    >    int original_height = height_;
    552a555,559
    >    delete filename_;
    >    delete textauthor_;
    >    delete textdescription_;
    >    delete textsoftware_;
    >    delete texttitle_;
    571c578,581
    <    graph_ = (png_bytepp)malloc(height_ * sizeof(png_bytep));
    ---
    >     if(graph_ == NULL || original_height != height_) {
    >         if(graph_ != NULL) free(graph_);
    >         graph_ = (png_bytepp)malloc(height_ * sizeof(png_bytep));
    >     }
    578a589,590
    >     if(graph_[kkkk] == NULL || original_width != width_) {
    >         if(graph_[kkkk] != NULL) free(graph_[kkkk]);
    579a592
    >     }

     
    • Paul Blackburn
      Paul Blackburn
      2009-03-06

      Thank you very much for the report and the code. I will take a look at it.

      Cheers,

      Paul

       
    • Paul Blackburn
      Paul Blackburn
      2009-03-07

      I think there might be a problem with that code, please correct me if I'm wrong.

      If the height changes, graph_ is freed and memory corresponding to the new height is allocated. But the memory associated with each element of graph_ is never freed.

      Hence, that section of the code should probably look like:

      - If height or width changes, free ALL memory associated with that instance.
      - Allocate new memory.

      p.

       
    • Ahnfelt
      Ahnfelt
      2009-03-08

      I think you're right. Fixing a memory leak by introducing a segfault... whoops!

       
    • Paul Blackburn
      Paul Blackburn
      2009-03-08

      Ok, here's the altered code for the overloaded = operator. The copy constructor shouldn't be a problem, since it's a constructor, and memory was not allocated to graph_ before it is called.

      * * *

      // Overloading operator =
      /////////////////////////////////////////////////////////
      pngwriter & pngwriter::operator = (const pngwriter & rhs)
      {
         if( this==&rhs)
           {
              return *this;
           }

          int original_width = width_;  //
          int original_height = height_; //

         width_ = rhs.width_;
         height_ = rhs.height_;
         backgroundcolour_ = rhs.backgroundcolour_;
         compressionlevel_ = rhs.compressionlevel_;
         filegamma_ = rhs.filegamma_;
         transformation_ = rhs.transformation_;

         delete filename_;     //
         delete textauthor_;    //
         delete textdescription_;   //
         delete textsoftware_;    //
         delete texttitle_;   //

         filename_ = new char[strlen(rhs.filename_)+1];
         textauthor_ = new char[strlen(rhs.textauthor_)+1];
         textdescription_ = new char[strlen(rhs.textdescription_)+1];
         textsoftware_ = new char[strlen(rhs.textsoftware_)+1];
         texttitle_ = new char[strlen(rhs.texttitle_)+1];

         strcpy(textauthor_, rhs.textauthor_);
         strcpy(textdescription_, rhs.textdescription_);
         strcpy(textsoftware_,rhs.textsoftware_);
         strcpy(texttitle_, rhs.texttitle_);
         strcpy(filename_, rhs.filename_);

         int kkkk;

         bit_depth_ = rhs.bit_depth_;
         colortype_= rhs.colortype_;
         screengamma_ = rhs.screengamma_;
        
         if(original_height != height_ || original_width != width_)
          {
               // If the height or width have changed, free the memory
              for (kkkk = 0; kkkk < height_; kkkk++)
                  {
                      if(graph_[kkkk] != NULL) free(graph_[kkkk]);         
                     }
              if(graph_ != NULL) free(graph_);   
               
              // And re-allocate it
               graph_ = (png_bytepp)malloc(height_ * sizeof(png_bytep));
                 if(graph_ == NULL)
                   {
                      std::cerr << " PNGwriter::pngwriter - ERROR **:  Not able to allocate memory for image." << std::endl;
                   }
                 for (kkkk = 0; kkkk < height_; kkkk++)
                   {
                        graph_[kkkk] = (png_bytep)malloc(6*width_ * sizeof(png_byte));
                      if(graph_[kkkk] == NULL)
                           {
                                 std::cerr << " PNGwriter::pngwriter - ERROR **:  Not able to allocate memory for image." << std::endl;
                           }
                  }
          }

          // At this point, graph_ is the right size for rhs, either because we re-allocated it, or because the sizes were the same.

         int tempindex;
         for(int hhh = 0; hhh<width_;hhh++)
           {
          for(int vhhh = 0; vhhh<height_;vhhh++)
            {
               tempindex=6*hhh;
               graph_[vhhh][tempindex] = rhs.graph_[vhhh][tempindex];
               graph_[vhhh][tempindex+1] = rhs.graph_[vhhh][tempindex+1];
               graph_[vhhh][tempindex+2] = rhs.graph_[vhhh][tempindex+2];
               graph_[vhhh][tempindex+3] = rhs.graph_[vhhh][tempindex+3];
               graph_[vhhh][tempindex+4] = rhs.graph_[vhhh][tempindex+4];
               graph_[vhhh][tempindex+5] = rhs.graph_[vhhh][tempindex+5];
            }
           }

         return *this;
      }