diff mbox series

[ovs-dev,2/3] ovsdb-idl: Fix memleak when deleting orphan rows.

Message ID 20201124121630.20453.20417.stgit@dceara.remote.csb
State Superseded
Headers show
Series ovsdb-idl: Fix IDL use-after-free and memory leak issues. | expand

Commit Message

Dumitru Ceara Nov. 24, 2020, 12:16 p.m. UTC
Pure IDL orphan rows, i.e., for which no "insert" operation was seen,
which are part of tables with change tracking enabled should also be
freed when the table track_list is flushed.

Reported-by: Ilya Maximets <i.maximets@ovn.org>
Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 lib/ovsdb-idl.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Han Zhou Nov. 30, 2020, 4:52 a.m. UTC | #1
On Tue, Nov 24, 2020 at 4:16 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> Pure IDL orphan rows, i.e., for which no "insert" operation was seen,
> which are part of tables with change tracking enabled should also be
> freed when the table track_list is flushed.

Hi Dumitru, does this change suggest that there are situations that a
deleted row is tracked but there is no tracked_old_datum? Could you give an
example of how it could happen? If a client didn't see a row "inserted"
then how could the server send a notification about the "deletion"? If a
row is never seen by a client IDL then I think it shouldn't be included in
the update notification.

>
> Reported-by: Ilya Maximets <i.maximets@ovn.org>
> Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted
rows.")
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
>  lib/ovsdb-idl.c |   18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index bbc22e4..b282ce4 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -1969,16 +1969,18 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db)
>
>                  ovs_list_remove(&row->track_node);
>                  ovs_list_init(&row->track_node);
> -                if (ovsdb_idl_row_is_orphan(row) &&
row->tracked_old_datum) {
> +                if (ovsdb_idl_row_is_orphan(row)) {
>                      ovsdb_idl_row_unparse(row);
> -                    const struct ovsdb_idl_table_class *class =
> -
 row->table->class_;
> -                    for (size_t c = 0; c < class->n_columns; c++) {
> -                        ovsdb_datum_destroy(&row->tracked_old_datum[c],
> -                                            &class->columns[c].type);
> +                    if (row->tracked_old_datum) {
> +                        const struct ovsdb_idl_table_class *class =
> +                            row->table->class_;
> +                        for (size_t c = 0; c < class->n_columns; c++) {
> +
 ovsdb_datum_destroy(&row->tracked_old_datum[c],
> +                                                &class->columns[c].type);
> +                        }
> +                        free(row->tracked_old_datum);
> +                        row->tracked_old_datum = NULL;
>                      }
> -                    free(row->tracked_old_datum);
> -                    row->tracked_old_datum = NULL;
>                      free(row);
>                  }
>              }
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Dumitru Ceara Nov. 30, 2020, 12:30 p.m. UTC | #2
On 11/30/20 5:52 AM, Han Zhou wrote:
> 
> 
> On Tue, Nov 24, 2020 at 4:16 AM Dumitru Ceara <dceara@redhat.com
> <mailto:dceara@redhat.com>> wrote:
>>
>> Pure IDL orphan rows, i.e., for which no "insert" operation was seen,
>> which are part of tables with change tracking enabled should also be
>> freed when the table track_list is flushed.
> 
> Hi Dumitru, does this change suggest that there are situations that a
> deleted row is tracked but there is no tracked_old_datum? Could you give
> an example of how it could happen? If a client didn't see a row
> "inserted" then how could the server send a notification about the
> "deletion"? If a row is never seen by a client IDL then I think it
> shouldn't be included in the update notification.
> 

Hi Han,

Ilya recently added a test case that exercises this exact scenario:

https://github.com/openvswitch/ovs/blob/fa57e9e45257f32b80c135c18ae821ac3c43a738/tests/ovsdb-idl.at#L1178

The "simple6" record in the test has weak references to "simple" records
"weak_row0", "weak_row1", "weak_row2".  But the monitor condition for
table "simple" is "s==row1_s".

That means that the IDL will create "pure" orphan records for
"weak_row0" and "weak_row1".

When the "simple6" record is deleted, the two orphan rows above should
also be deleted.  For that they're added to the track_list.

Without the fix, they'll just be removed from the list without ever
being freed.

Regards,
Dumitru
diff mbox series

Patch

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index bbc22e4..b282ce4 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1969,16 +1969,18 @@  ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db)
 
                 ovs_list_remove(&row->track_node);
                 ovs_list_init(&row->track_node);
-                if (ovsdb_idl_row_is_orphan(row) && row->tracked_old_datum) {
+                if (ovsdb_idl_row_is_orphan(row)) {
                     ovsdb_idl_row_unparse(row);
-                    const struct ovsdb_idl_table_class *class =
-                                                        row->table->class_;
-                    for (size_t c = 0; c < class->n_columns; c++) {
-                        ovsdb_datum_destroy(&row->tracked_old_datum[c],
-                                            &class->columns[c].type);
+                    if (row->tracked_old_datum) {
+                        const struct ovsdb_idl_table_class *class =
+                            row->table->class_;
+                        for (size_t c = 0; c < class->n_columns; c++) {
+                            ovsdb_datum_destroy(&row->tracked_old_datum[c],
+                                                &class->columns[c].type);
+                        }
+                        free(row->tracked_old_datum);
+                        row->tracked_old_datum = NULL;
                     }
-                    free(row->tracked_old_datum);
-                    row->tracked_old_datum = NULL;
                     free(row);
                 }
             }