diff mbox

[for-1.4,04/12] qmp: Plug memory leaks in memchar-write, memchar-read

Message ID 1360081335-6594-5-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Feb. 5, 2013, 4:22 p.m. UTC 
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-char.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Eric Blake Feb. 5, 2013, 8:48 p.m. UTC | #1 
On 02/05/2013 09:22 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-char.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>
Luiz Capitulino Feb. 6, 2013, 3:21 p.m. UTC | #2 
On Tue,  5 Feb 2013 17:22:07 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-char.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 892d5bb..9bf53e0 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2782,6 +2782,10 @@ void qmp_memchar_write(const char *device, const char *data,
>  
>      ret = cirmem_chr_write(chr, write_data, write_count);
>  
> +    if (write_data != (uint8_t *)data) {

Minor, but wouldn't it be clearer to check against has_format and format?

> +        g_free((void *)write_data);
> +    }
> +
>      if (ret < 0) {
>          error_setg(errp, "Failed to write to device %s", device);
>          return;
> @@ -2825,6 +2829,7 @@ char *qmp_memchar_read(const char *device, int64_t size,
>  
>      if (has_format && (format == DATA_FORMAT_BASE64)) {
>          data = g_base64_encode(read_data, count);
> +        g_free(read_data);
>      } else {
>          data = (char *)read_data;
>      }
Markus Armbruster Feb. 6, 2013, 4:11 p.m. UTC | #3 
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue,  5 Feb 2013 17:22:07 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qemu-char.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 892d5bb..9bf53e0 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2782,6 +2782,10 @@ void qmp_memchar_write(const char *device, const char *data,
>>  
>>      ret = cirmem_chr_write(chr, write_data, write_count);
>>  
>> +    if (write_data != (uint8_t *)data) {
>
> Minor, but wouldn't it be clearer to check against has_format and format?

I actually like it this way, because it makes it blatantly obvious that
write_data is either freed or owned by the caller.

>> +        g_free((void *)write_data);
>> +    }
>> +
>>      if (ret < 0) {
>>          error_setg(errp, "Failed to write to device %s", device);
>>          return;
>> @@ -2825,6 +2829,7 @@ char *qmp_memchar_read(const char *device, int64_t size,
>>  
>>      if (has_format && (format == DATA_FORMAT_BASE64)) {
>>          data = g_base64_encode(read_data, count);
>> +        g_free(read_data);
>>      } else {
>>          data = (char *)read_data;
>>      }
diff mbox

Patch

diff --git a/qemu-char.c b/qemu-char.c
index 892d5bb..9bf53e0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2782,6 +2782,10 @@  void qmp_memchar_write(const char *device, const char *data,
 
     ret = cirmem_chr_write(chr, write_data, write_count);
 
+    if (write_data != (uint8_t *)data) {
+        g_free((void *)write_data);
+    }
+
     if (ret < 0) {
         error_setg(errp, "Failed to write to device %s", device);
         return;
@@ -2825,6 +2829,7 @@  char *qmp_memchar_read(const char *device, int64_t size,
 
     if (has_format && (format == DATA_FORMAT_BASE64)) {
         data = g_base64_encode(read_data, count);
+        g_free(read_data);
     } else {
         data = (char *)read_data;
     }