diff mbox

[ovs-dev,1/3] ovsdb-monitor: Fix segfault during replication.

Message ID 20160909204853.27839-2-joe@ovn.org
State Superseded
Headers show

Commit Message

Joe Stringer Sept. 9, 2016, 8:48 p.m. UTC
Prior to commit 60e0cd041958 ("ovsdb: Replication usability
improvements"), a changes would never have 'change->new' == NULL.
Apparently this is now possible, which leads to the crash below.

Valgrind reports:

==18725== Process terminating with default action of signal 11
(SIGSEGV): dumping core
==18725==  Access not within mapped region at address 0x0
==18725==    at 0x43937E: ovsdb_datum_compare_3way (ovsdb-data.c:1626)
==18725==    by 0x439344: ovsdb_datum_equals (ovsdb-data.c:1616)
==18725==    by 0x4166CC: update_monitor_row_data (monitor.c:310)
==18725==    by 0x414A90: ovsdb_monitor_changes_update (monitor.c:1255)
==18725==    by 0x417009: ovsdb_monitor_change_cb (monitor.c:1339)
==18725==    by 0x41DB52: ovsdb_txn_for_each_change (transaction.c:906)
==18725==    by 0x416CC9: ovsdb_monitor_commit (monitor.c:1553)
==18725==    by 0x41D993: ovsdb_txn_commit_ (transaction.c:868)
==18725==    by 0x41D6F5: ovsdb_txn_commit (transaction.c:893)
==18725==    by 0x418185: process_notification (replication.c:576)
==18725==    by 0x417705: replication_run (replication.c:185)
==18725==    by 0x408240: main_loop (ovsdb-server.c:198)
==18725==    by 0x406432: main (ovsdb-server.c:429)

Fixes: 60e0cd041958 ("ovsdb: Replication usability improvements")
Signed-off-by: Joe Stringer <joe@ovn.org>
---
This addresses the crash, but I don't understand the deeper reasoning
why we get into this state in the first place so I welcome any counter
proposals.

This also affects branch-2.6.
---
 ovsdb/monitor.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Ben Pfaff Sept. 11, 2016, 4:23 a.m. UTC | #1
On Fri, Sep 09, 2016 at 01:48:51PM -0700, Joe Stringer wrote:
> Prior to commit 60e0cd041958 ("ovsdb: Replication usability
> improvements"), a changes would never have 'change->new' == NULL.
> Apparently this is now possible, which leads to the crash below.
> 
> Valgrind reports:
> 
> ==18725== Process terminating with default action of signal 11
> (SIGSEGV): dumping core
> ==18725==  Access not within mapped region at address 0x0
> ==18725==    at 0x43937E: ovsdb_datum_compare_3way (ovsdb-data.c:1626)
> ==18725==    by 0x439344: ovsdb_datum_equals (ovsdb-data.c:1616)
> ==18725==    by 0x4166CC: update_monitor_row_data (monitor.c:310)
> ==18725==    by 0x414A90: ovsdb_monitor_changes_update (monitor.c:1255)
> ==18725==    by 0x417009: ovsdb_monitor_change_cb (monitor.c:1339)
> ==18725==    by 0x41DB52: ovsdb_txn_for_each_change (transaction.c:906)
> ==18725==    by 0x416CC9: ovsdb_monitor_commit (monitor.c:1553)
> ==18725==    by 0x41D993: ovsdb_txn_commit_ (transaction.c:868)
> ==18725==    by 0x41D6F5: ovsdb_txn_commit (transaction.c:893)
> ==18725==    by 0x418185: process_notification (replication.c:576)
> ==18725==    by 0x417705: replication_run (replication.c:185)
> ==18725==    by 0x408240: main_loop (ovsdb-server.c:198)
> ==18725==    by 0x406432: main (ovsdb-server.c:429)
> 
> Fixes: 60e0cd041958 ("ovsdb: Replication usability improvements")
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
> This addresses the crash, but I don't understand the deeper reasoning
> why we get into this state in the first place so I welcome any counter
> proposals.
> 
> This also affects branch-2.6.

Please try this as an alternative:
        https://patchwork.ozlabs.org/patch/668413/
diff mbox

Patch

diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index 9a6fbf5a5a30..5320c7db0514 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -1252,7 +1252,11 @@  ovsdb_monitor_changes_update(const struct ovsdb_row *old,
         change->new = clone_monitor_row_data(mt, new);
     } else {
         if (new) {
-            update_monitor_row_data(mt, new, change->new);
+            if (change->new) {
+                update_monitor_row_data(mt, new, change->new);
+            } else {
+                change->new = clone_monitor_row_data(mt, new);
+            }
         } else {
             free_monitor_row_data(mt, change->new);
             change->new = NULL;