codegen: Keep arrays alive during async server method calls

When calling a co-routine it is the caller's responsibility to ensure
that arrays stay alive for the duration of the call. The GDBus server
code emitted did not do this, resulting in use-after-free.

https://bugzilla.gnome.org/show_bug.cgi?id=783002

diff --git a/codegen/valagasyncmodule.vala b/codegen/valagasyncmodule.vala
index 54df5239548e565032c015587648bf28a932f700..715c35f56de7c7f8b519a93151cfa30d68bc75c1 100644 (file)
--- a/codegen/valagasyncmodule.vala
+++ b/codegen/valagasyncmodule.vala
@@ -381,7 +381,7 @@ public class Vala.GAsyncModule : GtkModule {
                pop_context ();
        }
 
-       void append_struct (CCodeStruct structure) {
+       public void append_struct (CCodeStruct structure) {
                var typename = new CCodeVariableDeclarator (structure.name.substring (1));
                var typedef = new CCodeTypeDefinition ("struct " + structure.name, typename);
                cfile.add_type_declaration (typedef);

diff --git a/codegen/valagdbusservermodule.vala b/codegen/valagdbusservermodule.vala
index 68909c05f038e29a1a29d48763a88b237a47d101..3506bc655e245cd9a1ec62c53b8c95ce5eb9b35a 100644 (file)
--- a/codegen/valagdbusservermodule.vala
+++ b/codegen/valagdbusservermodule.vala
@@ -23,7 +23,7 @@
 public class Vala.GDBusServerModule : GDBusClientModule {
        string generate_dbus_wrapper (Method m, ObjectTypeSymbol sym, bool ready = false) {
                string wrapper_name = "_dbus_%s".printf (get_ccode_name (m));
-               bool need_goto_label = false;
+               bool need_goto_label = ready;
 
                if (m.base_method != null) {
                        m = m.base_method;
@@ -51,8 +51,12 @@ public class Vala.GDBusServerModule : GDBusClientModule {
 
                push_function (function);
 
+               CCodeIdentifier ready_data_expr = m.coroutine ? new CCodeIdentifier ("_ready_data") : null;
+               string ready_data_struct_name = Symbol.lower_case_to_camel_case (get_ccode_name (m)) + "ReadyData";
+
                if (ready) {
-                       ccode.add_declaration ("GDBusMethodInvocation *", new CCodeVariableDeclarator ("invocation", new CCodeIdentifier ("_user_data_")));
+                       ccode.add_declaration (ready_data_struct_name + "*", new CCodeVariableDeclarator ("_ready_data", new CCodeIdentifier ("_user_data_")));
+                       ccode.add_declaration ("GDBusMethodInvocation*", new CCodeVariableDeclarator ("invocation", new CCodeMemberAccess.pointer (ready_data_expr, "_invocation_")));
                }
 
                var connection = new CCodeFunctionCall (new CCodeIdentifier ("g_dbus_method_invocation_get_connection"));
@@ -97,6 +101,22 @@ public class Vala.GDBusServerModule : GDBusClientModule {
                                ccode.add_declaration ("gint", new CCodeVariableDeclarator ("_fd"));
                        }
 
+                       CCodeStruct? ready_data_struct = null;
+
+                       if (m.coroutine) {
+                               ready_data_struct = new CCodeStruct ("_" + ready_data_struct_name);
+                               ready_data_struct.add_field ("GDBusMethodInvocation*", "_invocation_");
+                               append_struct (ready_data_struct);
+
+                               var ready_data_alloc = new CCodeFunctionCall (new CCodeIdentifier ("g_slice_new0"));
+                               ready_data_alloc.add_argument (new CCodeIdentifier (ready_data_struct_name));
+
+                               ccode.add_declaration (ready_data_struct_name + "*", new CCodeVariableDeclarator ("_ready_data"));
+                               ccode.add_assignment (ready_data_expr, ready_data_alloc);
+
+                               ccode.add_assignment (new CCodeMemberAccess.pointer (ready_data_expr, "_invocation_"), new CCodeIdentifier ("invocation"));
+                       }
+
                        foreach (Parameter param in m.get_parameters ()) {
                                string param_name = get_variable_cname (param.name);
                                if (param.direction != ParameterDirection.IN) {
@@ -112,17 +132,32 @@ public class Vala.GDBusServerModule : GDBusClientModule {
                                        continue;
                                }
 
+                               CCodeExpression param_expr;
+                               if (ready_data_expr != null) {
+                                       param_expr = new CCodeMemberAccess.pointer (ready_data_expr, param_name);
+                               } else {
+                                       param_expr = new CCodeIdentifier (param_name);
+                               }
+
                                var owned_type = param.variable_type.copy ();
                                owned_type.value_owned = true;
 
-                               ccode.add_declaration (get_ccode_name (owned_type), new CCodeVariableDeclarator.zero (param_name, default_value_for_type (param.variable_type, true)));
+                               if (ready_data_struct != null) {
+                                       ready_data_struct.add_field (get_ccode_name (owned_type), param_name);
+                               } else {
+                                       ccode.add_declaration (get_ccode_name (owned_type), new CCodeVariableDeclarator.zero (param_name, default_value_for_type (param.variable_type, true)));
+                               }
 
                                var array_type = param.variable_type as ArrayType;
                                if (array_type != null) {
                                        for (int dim = 1; dim <= array_type.rank; dim++) {
                                                string length_cname = get_parameter_array_length_cname (param, dim);
 
-                                               ccode.add_declaration ("int", new CCodeVariableDeclarator.zero (length_cname, new CCodeConstant ("0")));
+                                               if (ready_data_struct != null) {
+                                                       ready_data_struct.add_field ("int", length_cname);
+                                               } else {
+                                                       ccode.add_declaration ("int", new CCodeVariableDeclarator.zero (length_cname, new CCodeConstant ("0")));
+                                               }
                                        }
                                }
 
@@ -130,7 +165,7 @@ public class Vala.GDBusServerModule : GDBusClientModule {
                                message_expr.add_argument (new CCodeIdentifier ("invocation"));
 
                                bool may_fail;
-                               receive_dbus_value (param.variable_type, message_expr, new CCodeIdentifier ("_arguments_iter"), new CCodeIdentifier (param_name), param, new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, new CCodeIdentifier ("error")), out may_fail);
+                               receive_dbus_value (param.variable_type, message_expr, new CCodeIdentifier ("_arguments_iter"), param_expr, param, new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, new CCodeIdentifier ("error")), out may_fail);
 
                                if (may_fail) {
                                        if (!uses_error) {
@@ -159,6 +194,14 @@ public class Vala.GDBusServerModule : GDBusClientModule {
 
                foreach (Parameter param in m.get_parameters ()) {
                        string param_name = get_variable_cname (param.name);
+
+                       CCodeExpression param_expr;
+                       if (ready_data_expr != null && param.direction == ParameterDirection.IN) {
+                               param_expr = new CCodeMemberAccess.pointer (ready_data_expr, param_name);
+                       } else {
+                               param_expr = new CCodeIdentifier (param_name);
+                       }
+
                        if (param.direction == ParameterDirection.IN && !ready) {
                                if (param.variable_type is ObjectType && param.variable_type.data_type.get_full_name () == "GLib.Cancellable") {
                                        ccall.add_argument (new CCodeConstant ("NULL"));
@@ -175,12 +218,12 @@ public class Vala.GDBusServerModule : GDBusClientModule {
 
                                var st = param.variable_type.data_type as Struct;
                                if (st != null && !st.is_simple_type ()) {
-                                       ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, new CCodeIdentifier (param_name)));
+                                       ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, param_expr));
                                } else {
-                                       ccall.add_argument (new CCodeIdentifier (param_name));
+                                       ccall.add_argument (param_expr);
                                }
                        } else if (param.direction == ParameterDirection.OUT && (!m.coroutine || ready)) {
-                               ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, new CCodeIdentifier (param_name)));
+                               ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, param_expr));
                        }
 
                        var array_type = param.variable_type as ArrayType;
@@ -188,10 +231,16 @@ public class Vala.GDBusServerModule : GDBusClientModule {
                                for (int dim = 1; dim <= array_type.rank; dim++) {
                                        string length_cname = get_parameter_array_length_cname (param, dim);
 
+                                       CCodeExpression length_expr;
+                                       if (ready_data_expr != null && param.direction == ParameterDirection.IN)
+                                               length_expr = new CCodeMemberAccess.pointer (ready_data_expr, length_cname);
+                                       else
+                                               length_expr = new CCodeIdentifier (length_cname);
+
                                        if (param.direction == ParameterDirection.IN && !ready) {
-                                               ccall.add_argument (new CCodeIdentifier (length_cname));
+                                               ccall.add_argument (length_expr);
                                        } else if (param.direction == ParameterDirection.OUT && !no_reply && (!m.coroutine || ready)) {
-                                               ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, new CCodeIdentifier (length_cname)));
+                                               ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, length_expr));
                                        }
                                }
                        }
@@ -216,7 +265,7 @@ public class Vala.GDBusServerModule : GDBusClientModule {
 
                if (m.coroutine && !ready) {
                        ccall.add_argument (new CCodeCastExpression (new CCodeIdentifier (wrapper_name + "_ready"), "GAsyncReadyCallback"));
-                       ccall.add_argument (new CCodeIdentifier ("invocation"));
+                       ccall.add_argument (ready_data_expr);
                }
 
                if (!m.coroutine || ready) {
@@ -373,7 +422,7 @@ public class Vala.GDBusServerModule : GDBusClientModule {
                }
 
                foreach (Parameter param in m.get_parameters ()) {
-                       if ((param.direction == ParameterDirection.IN && !ready) ||
+                       if ((param.direction == ParameterDirection.IN && (ready_data_expr == null || ready)) ||
                            (param.direction == ParameterDirection.OUT && !no_reply && (!m.coroutine || ready))) {
                                if (param.variable_type is ObjectType && param.variable_type.data_type.get_full_name () == "GLib.Cancellable") {
                                        continue;
@@ -388,13 +437,37 @@ public class Vala.GDBusServerModule : GDBusClientModule {
                                owned_type.value_owned = true;
 
                                if (requires_destroy (owned_type)) {
-                                       // keep local alive (symbol_reference is weak)
-                                       var local = new LocalVariable (owned_type, get_variable_cname (param.name));
-                                       ccode.add_expression (destroy_local (local));
+                                       if (ready_data_expr != null && param.direction == ParameterDirection.IN) {
+                                               var target = new GLibValue (owned_type, new CCodeMemberAccess.pointer (ready_data_expr, param.name), true);
+
+                                               var array_type = owned_type as ArrayType;
+                                               if (array_type != null) {
+                                                       for (int dim = 1; dim <= array_type.rank; dim++) {
+                                                               string length_cname = get_parameter_array_length_cname (param, dim);
+
+                                                               target.append_array_length_cvalue (new CCodeMemberAccess.pointer (ready_data_expr, length_cname));
+                                                       }
+                                               }
+
+                                               ccode.add_expression (destroy_value (target));
+                                       } else {
+                                               // keep local alive (symbol_reference is weak)
+                                               var local = new LocalVariable (owned_type, get_variable_cname (param.name));
+                                               ccode.add_expression (destroy_local (local));
+                                       }
                                }
                        }
                }
 
+               if (ready) {
+                       var freecall = new CCodeFunctionCall (new CCodeIdentifier ("g_slice_free"));
+                       freecall.add_argument (new CCodeIdentifier (ready_data_struct_name));
+                       freecall.add_argument (ready_data_expr);
+                       ccode.add_expression (freecall);
+               } else if (need_goto_label) {
+                       ccode.add_statement (new CCodeEmptyStatement ());
+               }
+
                pop_function ();
 
                cfile.add_function_declaration (function);

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0645f2f13b83c87a909a51fc0683645b5367644a..5a7dbebee1a1eaffea2cf562b7a902fe907c61b4 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -375,6 +375,7 @@ TESTS = \
        dbus/bug596862.vala \
        dbus/bug602003.test \
        dbus/bug782719.test \
+       dbus/bug783002.test \
        dbus/bug792277.vala \
        dbus/rawvariants.test \
        gir/bug651773.test \

diff --git a/tests/dbus/bug783002.test b/tests/dbus/bug783002.test
new file mode 100644 (file)
index 0000000..f300c2b
--- /dev/null
+++ b/tests/dbus/bug783002.test
@@ -0,0 +1,70 @@
+Packages: gio-2.0
+D-Bus
+
+Program: client
+
+[DBus (name = "org.example.Test")]
+interface Test : Object {
+       public abstract async string test_array_lifetime (string[] items) throws IOError;
+}
+
+MainLoop main_loop;
+
+void main () {
+       main_loop = new MainLoop ();
+       run.begin ();
+       main_loop.run ();
+}
+
+async void run () {
+       Test test = yield Bus.get_proxy (BusType.SESSION, "org.example.Test", "/org/example/test");
+
+       var result = yield test.test_array_lifetime (new string[] { "Badger", "Snake", "Mushroom" });
+       assert (result == "BadgerSnakeMushroom");
+
+       main_loop.quit ();
+}
+
+Program: server
+
+[DBus (name = "org.example.Test")]
+class Test : Object {
+       public async string test_array_lifetime (string[] items) throws IOError {
+               Idle.add (() => {
+                       test_array_lifetime.callback ();
+                       return false;
+               });
+               yield;
+
+               var result = new StringBuilder ();
+               foreach (var item in items) {
+                       result.append (item);
+               }
+
+               assert (result.str == "BadgerSnakeMushroom");
+               return result.str;
+       }
+}
+
+MainLoop main_loop;
+
+void on_client_exit (Pid pid, int status) {
+       assert (status == 0);
+       main_loop.quit ();
+}
+
+void main () {
+       var conn = Bus.get_sync (BusType.SESSION);
+       conn.register_object ("/org/example/test", new Test ());
+
+       var request_result = conn.call_sync ("org.freedesktop.DBus", "/org/freedesktop/DBus", "org.freedesktop.DBus", "RequestName",
+                                             new Variant ("(su)", "org.example.Test", 0x4), null, 0, -1);
+       assert ((uint) request_result.get_child_value (0) == 1);
+
+       Pid client_pid;
+       Process.spawn_async (null, { "test", "/dbus/bug783002/client" }, null, SpawnFlags.DO_NOT_REAP_CHILD, null, out client_pid);
+       ChildWatch.add (client_pid, on_client_exit);
+
+       main_loop = new MainLoop ();
+       main_loop.run ();
+}