[PATCH 0/6] Fix some memory leaks in Ceph

[PATCH 0/6] Fix some memory leaks in Ceph

[Danny Al-Gaaf]

Here some patches to fix memory leaks in Ceph found by cppcheck.

Danny Al-Gaaf (6):
  common/fiemap.cc: fix realloc memory leak
  os/FileStore.cc: fix realloc memory leak in error case
  rgw/rgw_xml.cc: fix realloc memory leak in error case
  wireshark: fix some memory leaks
  SyntheticClient.cc: fix some memory leaks in the error handling
  rgw/rgw_rest.cc: fix 4K memory leak

 src/client/SyntheticClient.cc | 15 ++++++++++++---
 src/common/fiemap.cc          |  9 ++++++---
 src/os/FileStore.cc           |  7 +++++--
 src/rgw/rgw_rest.cc           |  4 +++-
 src/rgw/rgw_xml.cc            | 11 +++++++++--
 wireshark/ceph/packet-ceph.c  | 19 ++++++++++++++-----
 6 files changed, 49 insertions(+), 16 deletions(-)

-- 
1.8.1.2

[PATCH 1/6] common/fiemap.cc: fix realloc memory leak

[Danny Al-Gaaf]

Fix error from cppcheck:

[src/common/fiemap.cc:73]: (error) Common realloc mistake: 'fiemap'
  nulled but not freed upon failure

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/common/fiemap.cc | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/common/fiemap.cc b/src/common/fiemap.cc
index 0df12d6..a1d5fbe 100644
--- a/src/common/fiemap.cc
+++ b/src/common/fiemap.cc
@@ -40,6 +40,7 @@
 struct fiemap *read_fiemap(int fd)
 {
   struct fiemap *fiemap;
+  struct fiemap *_realloc_fiemap = NULL;
   int extents_size;
   int r;
 
@@ -62,18 +63,20 @@ struct fiemap *read_fiemap(int fd)
   }
 
   if (!fiemap->fm_mapped_extents) {
-    free(fiemap);
-    return NULL;
+    goto done_err;
   }
 
   /* Read in the extents */
   extents_size = sizeof(struct fiemap_extent) * (fiemap->fm_mapped_extents);
 
   /* Resize fiemap to allow us to read in the extents */
-  if ((fiemap = (struct fiemap*)realloc(fiemap,sizeof(struct fiemap) +
+
+  if ((_realloc_fiemap = (struct fiemap*)realloc(fiemap,sizeof(struct fiemap) +
                                         extents_size)) == NULL) {
     fprintf(stderr, "Out of memory allocating fiemap\n");
     goto done_err;
+  } else {
+    fiemap = _realloc_fiemap;
   }
 
   memset(fiemap->fm_extents, 0, extents_size);
-- 
1.8.1.2

[PATCH 2/6] os/FileStore.cc: fix realloc memory leak in error case

[Danny Al-Gaaf]

Fix error from cppcheck:

[src/os/FileStore.cc:512]: (error) Common realloc mistake: 'fiemap'
  nulled but not freed upon failure

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/os/FileStore.cc | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/os/FileStore.cc b/src/os/FileStore.cc
index 1bab9c3..44f3b57 100644
--- a/src/os/FileStore.cc
+++ b/src/os/FileStore.cc
@@ -490,6 +490,7 @@ bool parse_attrname(char **name)
 static int do_fiemap(int fd, off_t start, size_t len, struct fiemap **pfiemap)
 {
   struct fiemap *fiemap = NULL;
+  struct fiemap *_realloc_fiemap = NULL;
   int size;
   int ret;
 
@@ -509,11 +510,13 @@ static int do_fiemap(int fd, off_t start, size_t len, struct fiemap **pfiemap)
 
   size = sizeof(struct fiemap_extent) * (fiemap->fm_mapped_extents);
 
-  fiemap = (struct fiemap *)realloc(fiemap, sizeof(struct fiemap) +
+  _realloc_fiemap = (struct fiemap *)realloc(fiemap, sizeof(struct fiemap) +
                                     size);
-  if (!fiemap) {
+  if (!_realloc_fiemap) {
     ret = -ENOMEM;
     goto done_err;
+  } else {
+    fiemap = _realloc_fiemap;
   }
 
   memset(fiemap->fm_extents, 0, size);
-- 
1.8.1.2

[PATCH 3/6] rgw/rgw_xml.cc: fix realloc memory leak in error case

[Danny Al-Gaaf]

Fix error from cppcheck:

[src/rgw/rgw_xml.cc:212]: (error) Common realloc mistake: 'buf'
  nulled but not freed upon failure

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/rgw/rgw_xml.cc | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/rgw/rgw_xml.cc b/src/rgw/rgw_xml.cc
index 4347b06..eee69d0 100644
--- a/src/rgw/rgw_xml.cc
+++ b/src/rgw/rgw_xml.cc
@@ -209,9 +209,16 @@ bool RGWXMLParser::init()
 bool RGWXMLParser::parse(const char *_buf, int len, int done)
 {
   int pos = buf_len;
-  buf = (char *)realloc(buf, buf_len + len);
-  if (!buf)
+  char *tmp_buf;
+  tmp_buf = (char *)realloc(buf, buf_len + len);
+  if (tmp_buf == NULL){
+    free(buf);
+    buf = NULL;
     return false;
+  } else {
+    buf = tmp_buf;
+  }
+
   memcpy(&buf[buf_len], _buf, len);
   buf_len += len;
 
-- 
1.8.1.2

[PATCH 4/6] wireshark: fix some memory leaks

[Danny Al-Gaaf]

Fix some memory leaks in packet-ceph.c. Error from cppcheck was:

[wireshark/ceph/packet-ceph.c:215]: (error) Memory leak: plop
[wireshark/ceph/packet-ceph.c:237]: (error) Memory leak: plop
[wireshark/ceph/packet-ceph.c:543]: (error) Memory leak: fsid_dec

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 wireshark/ceph/packet-ceph.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/wireshark/ceph/packet-ceph.c b/wireshark/ceph/packet-ceph.c
index 5d2c702..2a4e31f 100644
--- a/wireshark/ceph/packet-ceph.c
+++ b/wireshark/ceph/packet-ceph.c
@@ -209,8 +209,8 @@ static gint ett_ceph_footer = -1;
 
 const char *ceph_cap_op_name(int op)
 {
-        char* plop = malloc(16*sizeof(char));
-        sprintf(plop,"%i",op);
+        char* plop;
+
         switch (op) {
         case CEPH_CAP_OP_GRANT: return "grant";
         case CEPH_CAP_OP_REVOKE: return "revoke";
@@ -226,13 +226,17 @@ const char *ceph_cap_op_name(int op)
         case CEPH_CAP_OP_RELEASE: return "release";
         case CEPH_CAP_OP_RENEW: return "renew";
         }
+
+        plop = malloc(16*sizeof(char));
+        sprintf(plop,"%i",op);
+
         return plop;
 }
 
 const char *ceph_mds_op_name(int op)
 {
-  char* plop = malloc(16*sizeof(char));
-         sprintf(plop,"%i",op);
+	char* plop;
+
         switch (op) {
         case CEPH_MDS_OP_LOOKUP:  return "lookup";
         case CEPH_MDS_OP_LOOKUPHASH:  return "lookuphash";
@@ -261,6 +265,10 @@ const char *ceph_mds_op_name(int op)
         case CEPH_MDS_OP_SETFILELOCK: return "setfilelock";
         case CEPH_MDS_OP_GETFILELOCK: return "getfilelock";
         }
+
+	plop = malloc(16*sizeof(char));
+        printf(plop,"%i",op);
+
         return plop;
 }
 
@@ -533,13 +541,14 @@ static guint32 dissect_ceph_fsid(tvbuff_t *tvb, proto_tree *tree, guint32 offset
 	fsid_dec = malloc(4*sizeof(guint32));
 	fsid = *(struct ceph_fsid *)tvb_get_ptr(tvb, offset, sizeof(struct ceph_fsid));
 	memcpy(fsid_dec,fsid.fsid,4*sizeof(guint32));
-	proto_tree_add_text(tree, tvb, offset,sizeof(struct ceph_fsid), "fsid: %x-%x-%x-%x",
+	proto_tree_add_text(tree, tvb, offset, sizeof(struct ceph_fsid), "fsid: %x-%x-%x-%x",
 			ntohl(fsid_dec[0]),
 			ntohl(fsid_dec[1]),
 			ntohl(fsid_dec[2]),
 			ntohl(fsid_dec[3])
 			);
 	offset += sizeof(struct ceph_fsid);
+	free (fsid_dec);
 	return offset;
 }
 
-- 
1.8.1.2

[PATCH 5/6] SyntheticClient.cc: fix some memory leaks in the error handling

[Danny Al-Gaaf]

Fix some memory leaks in case of error handling due to failed
client->open() calls.

Error from cppcheck was:
[src/client/SyntheticClient.cc:1980]: (error) Memory leak: buf
[src/client/SyntheticClient.cc:2040]: (error) Memory leak: buf
[src/client/SyntheticClient.cc:2090]: (error) Memory leak: buf

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/client/SyntheticClient.cc | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/client/SyntheticClient.cc b/src/client/SyntheticClient.cc
index b2a936f..0da8bed 100644
--- a/src/client/SyntheticClient.cc
+++ b/src/client/SyntheticClient.cc
@@ -1977,7 +1977,10 @@ int SyntheticClient::write_file(string& fn, int size, loff_t wrsize)   // size i
 
   int fd = client->open(fn.c_str(), O_RDWR|O_CREAT);
   dout(5) << "writing to " << fn << " fd " << fd << dendl;
-  if (fd < 0) return fd;
+  if (fd < 0) {
+    delete[] buf;
+    return fd;
+  }
   
   utime_t from = ceph_clock_now(g_ceph_context);
   utime_t start = from;
@@ -2037,7 +2040,10 @@ int SyntheticClient::write_fd(int fd, int size, int wrsize)   // size is in MB,
   uint64_t chunks = (uint64_t)size * (uint64_t)(1024*1024) / (uint64_t)wrsize;
 
   //dout(5) << "SyntheticClient::write_fd: writing to fd " << fd << dendl;
-  if (fd < 0) return fd;
+  if (fd < 0) {
+    return fd;
+    delete[] buf;
+  }
 
   for (unsigned i=0; i<chunks; i++) {
     if (time_to_stop()) {
@@ -2087,7 +2093,10 @@ int SyntheticClient::read_file(const std::string& fn, int size,
 
   int fd = client->open(fn.c_str(), O_RDONLY);
   dout(5) << "reading from " << fn << " fd " << fd << dendl;
-  if (fd < 0) return fd;
+  if (fd < 0) {
+    return fd;
+    delete[] buf;
+  }
 
   utime_t from = ceph_clock_now(g_ceph_context);
   utime_t start = from;
-- 
1.8.1.2

[PATCH 6/6] rgw/rgw_rest.cc: fix 4K memory leak

[Danny Al-Gaaf]

Fix 4K memory leak in case RGWClientIO::read() fails in
read_all_chunked_input().

Error from cppcheck was:
Checking src/rgw/rgw_rest.cc...
[src/rgw/rgw_rest.cc:688]: (error) Memory leak: data

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/rgw/rgw_rest.cc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/rgw/rgw_rest.cc b/src/rgw/rgw_rest.cc
index 72aab14..ab3927e 100644
--- a/src/rgw/rgw_rest.cc
+++ b/src/rgw/rgw_rest.cc
@@ -684,8 +684,10 @@ static int read_all_chunked_input(req_state *s, char **pdata, int *plen)
   int read_len = 0, len = 0;
   do {
     int r = s->cio->read(data + len, need_to_read, &read_len);
-    if (r < 0)
+    if (r < 0) {
+      free(data);
       return r;
+    }
 
     len += read_len;
 
-- 
1.8.1.2

Re: [PATCH 5/6] SyntheticClient.cc: fix some memory leaks in the error handling

[Danny Al-Gaaf]

Am 08.02.2013 17:25, schrieb Danny Al-Gaaf:
> Fix some memory leaks in case of error handling due to failed
> client->open() calls.
> 
> Error from cppcheck was:
> [src/client/SyntheticClient.cc:1980]: (error) Memory leak: buf
> [src/client/SyntheticClient.cc:2040]: (error) Memory leak: buf
> [src/client/SyntheticClient.cc:2090]: (error) Memory leak: buf
> 
> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Sorry, wrong patch version with a bug. I'll send a new version of this
patch.

Danny

[PATCH v2 5/6] SyntheticClient.cc: fix some memory leaks in the error handling

[Danny Al-Gaaf]

Fix some memory leaks in case of error handling due to failed
client->open() calls.

Error from cppcheck was:
[src/client/SyntheticClient.cc:1980]: (error) Memory leak: buf
[src/client/SyntheticClient.cc:2040]: (error) Memory leak: buf
[src/client/SyntheticClient.cc:2090]: (error) Memory leak: buf

---
v2: - fix order in two cases 

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/client/SyntheticClient.cc | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/client/SyntheticClient.cc b/src/client/SyntheticClient.cc
index b2a936f..0da8bed 100644
--- a/src/client/SyntheticClient.cc
+++ b/src/client/SyntheticClient.cc
@@ -1977,7 +1977,10 @@ int SyntheticClient::write_file(string& fn, int size, loff_t wrsize)   // size i
 
   int fd = client->open(fn.c_str(), O_RDWR|O_CREAT);
   dout(5) << "writing to " << fn << " fd " << fd << dendl;
-  if (fd < 0) return fd;
+  if (fd < 0) {
+    delete[] buf;
+    return fd;
+  }
   
   utime_t from = ceph_clock_now(g_ceph_context);
   utime_t start = from;
@@ -2037,7 +2040,10 @@ int SyntheticClient::write_fd(int fd, int size, int wrsize)   // size is in MB,
   uint64_t chunks = (uint64_t)size * (uint64_t)(1024*1024) / (uint64_t)wrsize;
 
   //dout(5) << "SyntheticClient::write_fd: writing to fd " << fd << dendl;
-  if (fd < 0) return fd;
+  if (fd < 0) {
+    delete[] buf;
+    return fd;
+  }
 
   for (unsigned i=0; i<chunks; i++) {
     if (time_to_stop()) {
@@ -2087,7 +2093,10 @@ int SyntheticClient::read_file(const std::string& fn, int size,
 
   int fd = client->open(fn.c_str(), O_RDONLY);
   dout(5) << "reading from " << fn << " fd " << fd << dendl;
-  if (fd < 0) return fd;
+  if (fd < 0) {
+    delete[] buf;
+    return fd;
+  }
 
   utime_t from = ceph_clock_now(g_ceph_context);
   utime_t start = from;
-- 
1.8.1.2

Re: [PATCH 5/6] SyntheticClient.cc: fix some memory leaks in the error handling

[Sage Weil]

On Fri, 8 Feb 2013, Danny Al-Gaaf wrote:
> Fix some memory leaks in case of error handling due to failed
> client->open() calls.
> 
> Error from cppcheck was:
> [src/client/SyntheticClient.cc:1980]: (error) Memory leak: buf
> [src/client/SyntheticClient.cc:2040]: (error) Memory leak: buf
> [src/client/SyntheticClient.cc:2090]: (error) Memory leak: buf
> 
> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
> ---
>  src/client/SyntheticClient.cc | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/src/client/SyntheticClient.cc b/src/client/SyntheticClient.cc
> index b2a936f..0da8bed 100644
> --- a/src/client/SyntheticClient.cc
> +++ b/src/client/SyntheticClient.cc
> @@ -1977,7 +1977,10 @@ int SyntheticClient::write_file(string& fn, int size, loff_t wrsize)   // size i
>  
>    int fd = client->open(fn.c_str(), O_RDWR|O_CREAT);
>    dout(5) << "writing to " << fn << " fd " << fd << dendl;
> -  if (fd < 0) return fd;
> +  if (fd < 0) {
> +    delete[] buf;
> +    return fd;
> +  }
>    
>    utime_t from = ceph_clock_now(g_ceph_context);
>    utime_t start = from;
> @@ -2037,7 +2040,10 @@ int SyntheticClient::write_fd(int fd, int size, int wrsize)   // size is in MB,
>    uint64_t chunks = (uint64_t)size * (uint64_t)(1024*1024) / (uint64_t)wrsize;
>  
>    //dout(5) << "SyntheticClient::write_fd: writing to fd " << fd << dendl;
> -  if (fd < 0) return fd;
> +  if (fd < 0) {
> +    return fd;
> +    delete[] buf;

oops, wrong order...

> +  }
>  
>    for (unsigned i=0; i<chunks; i++) {
>      if (time_to_stop()) {
> @@ -2087,7 +2093,10 @@ int SyntheticClient::read_file(const std::string& fn, int size,
>  
>    int fd = client->open(fn.c_str(), O_RDONLY);
>    dout(5) << "reading from " << fn << " fd " << fd << dendl;
> -  if (fd < 0) return fd;
> +  if (fd < 0) {
> +    return fd;
> +    delete[] buf;

here too!

> +  }
>  
>    utime_t from = ceph_clock_now(g_ceph_context);
>    utime_t start = from;
> -- 
> 1.8.1.2
> 
> 

Re: [PATCH 5/6] SyntheticClient.cc: fix some memory leaks in the error handling

[Danny Al-Gaaf]

Am 10.02.2013 07:04, schrieb Sage Weil:
> On Fri, 8 Feb 2013, Danny Al-Gaaf wrote:
[...]
>>    //dout(5) << "SyntheticClient::write_fd: writing to fd " << fd << dendl;
>> -  if (fd < 0) return fd;
>> +  if (fd < 0) {
>> +    return fd;
>> +    delete[] buf;
> 
> oops, wrong order...

I know, that's why I've send another fixed version of this patch to the
list the same day:
http://thread.gmane.org/gmane.comp.file-systems.ceph.devel/12969/focus=12985

Danny

Re: [PATCH 5/6] SyntheticClient.cc: fix some memory leaks in the error handling

[Sage Weil]

On Sun, 10 Feb 2013, Danny Al-Gaaf wrote:
> Am 10.02.2013 07:04, schrieb Sage Weil:
> > On Fri, 8 Feb 2013, Danny Al-Gaaf wrote:
> [...]
> >>    //dout(5) << "SyntheticClient::write_fd: writing to fd " << fd << dendl;
> >> -  if (fd < 0) return fd;
> >> +  if (fd < 0) {
> >> +    return fd;
> >> +    delete[] buf;
> > 
> > oops, wrong order...
> 
> I know, that's why I've send another fixed version of this patch to the
> list the same day:
> http://thread.gmane.org/gmane.comp.file-systems.ceph.devel/12969/focus=12985

Oops, I see it now, sorry :)

I merged the memleaks branch.  The cppcheck-clang one is coming up with 
build errors tho:

	http://ceph.com/gitbuilder.cgi

Thanks!
sage