Message ID | 78D7904E-3C90-4ABD-9C20-EE17E610F166@opencloud.tech |
---|---|
State | Not Applicable |
Headers | show |
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-zhangtonghao@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-zhangtonghao@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(&cnd->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(&condition); > + 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(&columns); > ovsdb_condition_destroy(&condition); > + json_destroy(CONST_CAST(struct json *, where)); > > return error; > } > -- > 1.8.3.1 > > >> From: William Tu <u9012063@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-u9012063@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 <u9012063@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(&condition); >> + 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(&columns); >> ovsdb_condition_destroy(&condition); >> + json_destroy(CONST_CAST(struct json *, where)); >> >> return error; >> } >> -- >> 2.5.0 >
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.
Good, it is unnecessary to check again. > On Jul 28, 2016, at 2:13 AM, William Tu <u9012063@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-zhangtonghao@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-zhangtonghao@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(&cnd->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(&condition); >> + 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(&columns); >> ovsdb_condition_destroy(&condition); >> + json_destroy(CONST_CAST(struct json *, where)); >> >> return error; >> } >> -- >> 1.8.3.1 >> >> >>> From: William Tu <u9012063@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-u9012063@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 <u9012063@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(&condition); >>> + 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(&columns); >>> ovsdb_condition_destroy(&condition); >>> + json_destroy(CONST_CAST(struct json *, where)); >>> >>> return error; >>> } >>> -- >>> 2.5.0 >>
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(&cnd->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(&condition); + 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(&columns); ovsdb_condition_destroy(&condition); + json_destroy(CONST_CAST(struct json *, where)); return error; }