Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[Andy Zhou]

On Mon, Aug 1, 2016 at 10:08 AM, William Tu  wrote:

> Thanks, I've submitted v2 which removes the 'const'.
>

LGTM.  Pushed to master.  Thanks for working on this.


>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[William Tu]

Thanks, I've submitted v2 which removes the 'const'.


On Wed, Jul 27, 2016 at 1:39 PM, Ben Pfaff  wrote:
> On Tue, Jul 26, 2016 at 06:28:30PM -0700, William Tu wrote:
>> Valgrind testcase 1804 ovsdb-server.at:1023 insert rows, update rows by value
>> reports the following leak.
>> json_from_string (json.c:1025)
>> execute_update (replication.c:614), similarily at execute_delete()
>> process_table_update (replication.c:502)
>> process_notification.part.5 (replication.c:445)
>> process_notification (replication.c:402)
>> check_for_notifications (replication.c:418)
>> replication_run (replication.c:110)
>>
>> Signed-off-by: William Tu 
>> ---
>>  ovsdb/replication.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
>> index af7ae5c..fe89d39 100644
>> --- a/ovsdb/replication.c
>> +++ b/ovsdb/replication.c
>> @@ -573,6 +573,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
>>  }
>>
>>  ovsdb_condition_destroy();
>> +json_destroy(CONST_CAST(struct json *, where));
>> +
>>  return error;
>>  }
>>
>> @@ -630,6 +632,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
>>  ovsdb_row_destroy(row);
>>  ovsdb_column_set_destroy();
>>  ovsdb_condition_destroy();
>> +json_destroy(CONST_CAST(struct json *, where));
>>
>>  return error;
>>  }
>
> Thanks, good catch.
>
> I think that we should just remove "const" from the variable
> declarations in each case; the cast is not worth it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[nickcooper-zhangtonghao]

Good, it is unnecessary to check again.

> On Jul 28, 2016, at 2:13 AM, William Tu <u9012...@gmail.com> wrote:
> 
> Hi zhangtonghao,
> 
> Thanks for the patch!
> I think it's safe to call free(ptr) when ptr == NULL. The free() will
> be no operation.
> 
> Regards,
> William
> 
> On Tue, Jul 26, 2016 at 9:18 PM, nickcooper-zhangtonghao
> <nickcooper-zhangtong...@opencloud.tech> wrote:
>> Hi William,
>> I reviewed your patch codes and found other bugs.
>> 
>> If the ‘ovsdb_condition_from_json’ return the error, cnd->clauses will be
>> set NULL, so ‘ovsdb_condition_destroy’ should check the 'cnd->clauses’ before
>> free it.
>> 
>> It is applied to ‘ovsdb_row_from_json’.
>> 
>> 
>> Signed-off-by: nickcooper-zhangtonghao 
>> <nickcooper-zhangtong...@opencloud.tech>
>> ---
>> ovsdb/column.c  | 5 -
>> ovsdb/condition.c   | 6 +-
>> ovsdb/replication.c | 3 +++
>> 3 files changed, 12 insertions(+), 2 deletions(-)
>> 
>> diff --git a/ovsdb/column.c b/ovsdb/column.c
>> index 8838df3..f01301f 100644
>> --- a/ovsdb/column.c
>> +++ b/ovsdb/column.c
>> @@ -129,7 +129,10 @@ ovsdb_column_set_init(struct ovsdb_column_set *set)
>> void
>> ovsdb_column_set_destroy(struct ovsdb_column_set *set)
>> {
>> -free(set->columns);
>> +if (set->columns) {
>> +free(set->columns);
>> +set->columns = NULL;
>> +}
>> }
>> 
>> void
>> diff --git a/ovsdb/condition.c b/ovsdb/condition.c
>> index 6da3b08..f337a37 100644
>> --- a/ovsdb/condition.c
>> +++ b/ovsdb/condition.c
>> @@ -485,7 +485,11 @@ ovsdb_condition_destroy(struct ovsdb_condition *cnd)
>> for (i = 0; i < cnd->n_clauses; i++) {
>> ovsdb_clause_free(>clauses[i]);
>> }
>> -free(cnd->clauses);
>> +
>> +if (cnd->clauses) {
>> +free(cnd->clauses);
>> +cnd->clauses = NULL;
>> +}
>> cnd->n_clauses = 0;
>> 
>> ovsdb_condition_optimize_destroy(cnd);
>> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
>> index 52b7085..bfd2ca1 100644
>> --- a/ovsdb/replication.c
>> +++ b/ovsdb/replication.c
>> @@ -568,6 +568,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
>> }
>> 
>> ovsdb_condition_destroy();
>> +    json_destroy(CONST_CAST(struct json *, where));
>> +
>> return error;
>> }
>> 
>> @@ -625,6 +627,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
>> ovsdb_row_destroy(row);
>> ovsdb_column_set_destroy();
>> ovsdb_condition_destroy();
>> +json_destroy(CONST_CAST(struct json *, where));
>> 
>> return error;
>> }
>> --
>> 1.8.3.1
>> 
>> 
>>> From: William Tu <u9012...@gmail.com>
>>> To: dev@openvswitch.org
>>> Subject: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.
>>> Message-ID: <1469582910-64371-1-git-send-email-u9012...@gmail.com>
>>> 
>>> Valgrind testcase 1804 ovsdb-server.at:1023 insert rows, update rows by 
>>> value
>>> reports the following leak.
>>>   json_from_string (json.c:1025)
>>>   execute_update (replication.c:614), similarily at execute_delete()
>>>   process_table_update (replication.c:502)
>>>   process_notification.part.5 (replication.c:445)
>>>   process_notification (replication.c:402)
>>>   check_for_notifications (replication.c:418)
>>>   replication_run (replication.c:110)
>>> 
>>> Signed-off-by: William Tu <u9012...@gmail.com>
>>> ---
>>> ovsdb/replication.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
>>> index af7ae5c..fe89d39 100644
>>> --- a/ovsdb/replication.c
>>> +++ b/ovsdb/replication.c
>>> @@ -573,6 +573,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
>>>}
>>> 
>>>ovsdb_condition_destroy();
>>> +json_destroy(CONST_CAST(struct json *, where));
>>> +
>>>return error;
>>> }
>>> 
>>> @@ -630,6 +632,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
>>>ovsdb_row_destroy(row);
>>>ovsdb_column_set_destroy();
>>>ovsdb_condition_destroy();
>>> +json_destroy(CONST_CAST(struct json *, where));
>>> 
>>>return error;
>>> }
>>> --
>>> 2.5.0
>> 



___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[Ben Pfaff]

On Tue, Jul 26, 2016 at 06:28:30PM -0700, William Tu wrote:
> Valgrind testcase 1804 ovsdb-server.at:1023 insert rows, update rows by value
> reports the following leak.
> json_from_string (json.c:1025)
> execute_update (replication.c:614), similarily at execute_delete()
> process_table_update (replication.c:502)
> process_notification.part.5 (replication.c:445)
> process_notification (replication.c:402)
> check_for_notifications (replication.c:418)
> replication_run (replication.c:110)
> 
> Signed-off-by: William Tu 
> ---
>  ovsdb/replication.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
> index af7ae5c..fe89d39 100644
> --- a/ovsdb/replication.c
> +++ b/ovsdb/replication.c
> @@ -573,6 +573,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
>  }
>  
>  ovsdb_condition_destroy();
> +json_destroy(CONST_CAST(struct json *, where));
> +
>  return error;
>  }
>  
> @@ -630,6 +632,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
>  ovsdb_row_destroy(row);
>  ovsdb_column_set_destroy();
>  ovsdb_condition_destroy();
> +json_destroy(CONST_CAST(struct json *, where));
>  
>  return error;
>  }

Thanks, good catch.

I think that we should just remove "const" from the variable
declarations in each case; the cast is not worth it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[Andy Zhou]

On Tue, Jul 26, 2016 at 6:28 PM, William Tu  wrote:

> Valgrind testcase 1804 ovsdb-server.at:1023 insert rows, update rows by
> value
> reports the following leak.
> json_from_string (json.c:1025)
> execute_update (replication.c:614), similarily at execute_delete()
> process_table_update (replication.c:502)
> process_notification.part.5 (replication.c:445)
> process_notification (replication.c:402)
> check_for_notifications (replication.c:418)
> replication_run (replication.c:110)
>
> Signed-off-by: William Tu 
>

Tested, it removed the memory leak reported.
I think declare 'where' as 'struct json' instead of 'const struct json'
will make the code easier to read.

 Acked-by: Andy Zhou 


>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[Ben Pfaff]

On Wed, Jul 27, 2016 at 12:18:37PM +0800, nickcooper-zhangtonghao wrote:
> If the ‘ovsdb_condition_from_json’ return the error, cnd->clauses will be
> set NULL, so ‘ovsdb_condition_destroy’ should check the 'cnd->clauses’ before
> free it.

Please read CodingStyle.md:

Functions that destroy an instance of a dynamically-allocated type
should accept and ignore a null pointer argument.  Code that calls
such a function (including the C standard library function free())
should omit a null-pointer check.  We find that this usually makes
code easier to read.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[nickcooper-zhangtonghao]

Hi William,
I reviewed your patch codes and found other bugs.

If the ‘ovsdb_condition_from_json’ return the error, cnd->clauses will be
set NULL, so ‘ovsdb_condition_destroy’ should check the 'cnd->clauses’ before
free it.

It is applied to ‘ovsdb_row_from_json’.


Signed-off-by: nickcooper-zhangtonghao <nickcooper-zhangtong...@opencloud.tech>
---
 ovsdb/column.c  | 5 -
 ovsdb/condition.c   | 6 +-
 ovsdb/replication.c | 3 +++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/ovsdb/column.c b/ovsdb/column.c
index 8838df3..f01301f 100644
--- a/ovsdb/column.c
+++ b/ovsdb/column.c
@@ -129,7 +129,10 @@ ovsdb_column_set_init(struct ovsdb_column_set *set)
 void
 ovsdb_column_set_destroy(struct ovsdb_column_set *set)
 {
-free(set->columns);
+if (set->columns) {
+free(set->columns);
+set->columns = NULL;
+}
 }

 void
diff --git a/ovsdb/condition.c b/ovsdb/condition.c
index 6da3b08..f337a37 100644
--- a/ovsdb/condition.c
+++ b/ovsdb/condition.c
@@ -485,7 +485,11 @@ ovsdb_condition_destroy(struct ovsdb_condition *cnd)
 for (i = 0; i < cnd->n_clauses; i++) {
 ovsdb_clause_free(>clauses[i]);
 }
-free(cnd->clauses);
+
+if (cnd->clauses) {
+free(cnd->clauses);
+cnd->clauses = NULL;
+}
 cnd->n_clauses = 0;

 ovsdb_condition_optimize_destroy(cnd);
diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index 52b7085..bfd2ca1 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -568,6 +568,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
 }

 ovsdb_condition_destroy();
+json_destroy(CONST_CAST(struct json *, where));
+
 return error;
 }

@@ -625,6 +627,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
 ovsdb_row_destroy(row);
 ovsdb_column_set_destroy();
 ovsdb_condition_destroy();
+json_destroy(CONST_CAST(struct json *, where));

 return error;
 }
--
1.8.3.1


> From: William Tu <u9012...@gmail.com>
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.
> Message-ID: <1469582910-64371-1-git-send-email-u9012...@gmail.com>
> 
> Valgrind testcase 1804 ovsdb-server.at:1023 insert rows, update rows by value
> reports the following leak.
>json_from_string (json.c:1025)
>execute_update (replication.c:614), similarily at execute_delete()
>process_table_update (replication.c:502)
>process_notification.part.5 (replication.c:445)
>process_notification (replication.c:402)
>check_for_notifications (replication.c:418)
>replication_run (replication.c:110)
> 
> Signed-off-by: William Tu <u9012...@gmail.com>
> ---
> ovsdb/replication.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
> index af7ae5c..fe89d39 100644
> --- a/ovsdb/replication.c
> +++ b/ovsdb/replication.c
> @@ -573,6 +573,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
> }
> 
> ovsdb_condition_destroy();
> +json_destroy(CONST_CAST(struct json *, where));
> +
> return error;
> }
> 
> @@ -630,6 +632,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
> ovsdb_row_destroy(row);
> ovsdb_column_set_destroy();
> ovsdb_condition_destroy();
> +json_destroy(CONST_CAST(struct json *, where));
> 
> return error;
> }
> -- 
> 2.5.0



___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[William Tu]

Valgrind testcase 1804 ovsdb-server.at:1023 insert rows, update rows by value
reports the following leak.
json_from_string (json.c:1025)
execute_update (replication.c:614), similarily at execute_delete()
process_table_update (replication.c:502)
process_notification.part.5 (replication.c:445)
process_notification (replication.c:402)
check_for_notifications (replication.c:418)
replication_run (replication.c:110)

Signed-off-by: William Tu 
---
 ovsdb/replication.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index af7ae5c..fe89d39 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -573,6 +573,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
 }
 
 ovsdb_condition_destroy();
+json_destroy(CONST_CAST(struct json *, where));
+
 return error;
 }
 
@@ -630,6 +632,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
 ovsdb_row_destroy(row);
 ovsdb_column_set_destroy();
 ovsdb_condition_destroy();
+json_destroy(CONST_CAST(struct json *, where));
 
 return error;
 }
-- 
2.5.0

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev