zeitgeist team mailing list archive

[Siegfried Gevatter <rainct@xxxxxxxxxx>]

Siegfried Gevatter has proposed merging lp:~zeitgeist/zeitgeist/bluebird-sig-crash2 into lp:zeitgeist.

Requested reviews:
  Zeitgeist Framework Team (zeitgeist)

For more details, see:
https://code.launchpad.net/~zeitgeist/zeitgeist/bluebird-sig-crash2/+merge/90166
-- 
https://code.launchpad.net/~zeitgeist/zeitgeist/bluebird-sig-crash2/+merge/90166
Your team Zeitgeist Framework Team is requested to review the proposed merge of lp:~zeitgeist/zeitgeist/bluebird-sig-crash2 into lp:zeitgeist.

=== modified file 'src/datamodel.vala'
--- src/datamodel.vala	2012-01-16 12:25:29 +0000
+++ src/datamodel.vala	2012-01-25 17:41:35 +0000
@@ -22,6 +22,13 @@
 
 namespace Zeitgeist
 {
+    private void assert_sig (bool condition, string error_message)
+        throws EngineError
+    {
+        if (unlikely (!condition))
+            throw new EngineError.INVALID_SIGNATURE (error_message);
+    }
+
     namespace Timestamp
     {
         public static int64 now ()
@@ -67,8 +74,10 @@
         }
 
         public TimeRange.from_variant (Variant variant)
+            throws EngineError.INVALID_SIGNATURE
         {
-            assert (variant.get_type_string () == "(xx)");
+            assert_sig (variant.get_type_string () == "(xx)",
+                "Invalid D-Bus signature.");
 
             int64 start_msec = 0;
             int64 end_msec = 0;
@@ -252,7 +261,7 @@
                              // must be immediately available to the user
         ANY             = 2  // The event subjects may or may not be available
     }
-    
+
     private bool check_field_match (string property,
             string template_property, bool is_symbol = false,
             bool can_wildcard = false)
@@ -334,21 +343,21 @@
             subjects.add (subject);
         }
 
-        public Event.from_variant (Variant event_variant) {
-            assert (event_variant.get_type_string () == "(" +
-                Utils.SIG_EVENT + ")");
+        public Event.from_variant (Variant event_variant) throws EngineError {
+            assert_sig (event_variant.get_type_string () == "(" +
+                Utils.SIG_EVENT + ")", "Invalid D-Bus signature.");
 
             VariantIter iter = event_variant.iterator ();
 
-            assert (iter.n_children () >= 3);
+            assert_sig (iter.n_children () >= 3, "Incomplete event struct.");
             VariantIter event_array = iter.next_value ().iterator ();
             VariantIter subjects_array = iter.next_value ().iterator ();
             Variant payload_variant = iter.next_value ();
 
             var event_props = event_array.n_children ();
-            assert (event_props >= 5);
-            id = (uint32) uint64.parse (event_array.next_value ().get_string ());
-            var str_timestamp = event_array.next_value ().get_string ();
+            assert_sig (event_props >= 5, "Missing event information.");
+            id = (uint32) uint64.parse (event_array.next_value().get_string ());
+            var str_timestamp = event_array.next_value().get_string ();
             if (str_timestamp == "")
                 timestamp = Timestamp.now ();
             else
@@ -421,7 +430,7 @@
             uchar[] data = new uchar[event_variant.get_size ()];
             event_variant.store (data);
             unowned uchar[] data_copy = data;
- 
+
             Variant ret = Variant.new_from_data (
                 new VariantType ("("+Utils.SIG_EVENT+")"),
                 data_copy, true, (owned) data);
@@ -455,9 +464,8 @@
                                s.mimetype, s.origin, s.text, s.current_uri,
                                s.storage);
             }
-       }
+        }
 
-        
 
         public bool matches_template (Event template_event)
         {
@@ -588,11 +596,12 @@
         }
 
         public Subject.from_variant (Variant subject_variant)
+            throws EngineError
         {
             VariantIter iter = subject_variant.iterator();
 
             var subject_props = iter.n_children ();
-            assert (subject_props >= 7);
+            assert_sig (subject_props >= 7, "Missing subject information");
             uri = iter.next_value().get_string ();
             interpretation = iter.next_value().get_string ();
             manifestation = iter.next_value().get_string ();

=== modified file 'src/errors.vala'
--- src/errors.vala	2012-01-25 10:36:27 +0000
+++ src/errors.vala	2012-01-25 17:41:35 +0000
@@ -32,6 +32,7 @@
         INVALID_ARGUMENT,
         INVALID_KEY,
         EXISTING_INSTANCE,
+        INVALID_SIGNATURE, // FIXME: change from EngineError to sth. + public
     }
 
     // vala doesn't include proper headers, this fixes it

=== modified file 'src/sql.vala'
--- src/sql.vala	2012-01-25 13:24:34 +0000
+++ src/sql.vala	2012-01-25 17:41:35 +0000
@@ -287,7 +287,7 @@
         public void assert_query_success (int rc, string msg,
             int success_code=Sqlite.OK) throws EngineError
         {
-            if (rc != success_code)
+            if (unlikely (rc != success_code))
             {
                 string error_message = "%s: %d, %s".printf(
                     msg, rc, database.errmsg ());

=== modified file 'test/direct/marshalling-test.vala'
--- test/direct/marshalling-test.vala	2011-12-31 00:31:17 +0000
+++ test/direct/marshalling-test.vala	2012-01-25 17:41:35 +0000
@@ -1,6 +1,8 @@
 /* marshalling-test.vala
  *
  * Copyright © 2011 Michal Hruby <michal.mhr@xxxxxxxxx>
+ * Copyright © 2011 Canonical Ltd.
+ *             By Siegfried-A. Gevatter <siegfried.gevatter@xxxxxxxxxxxxxxx>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Lesser General Public License as published by
@@ -21,75 +23,166 @@
 
 int main (string[] argv)
 {
-  Test.init (ref argv);
-
-  Test.add_func ("/marshalling/subjects", subject_test);
-  Test.add_func ("/marshalling/event", event_test);
-  Test.add_func ("/marshalling/events", events_test);
-
-  return Test.run ();
+    Test.init (ref argv);
+
+    Test.add_func ("/Marshalling/subjects", subject_test);
+    Test.add_func ("/Marshalling/event", event_test);
+    Test.add_func ("/Marshalling/events", events_test);
+    Test.add_func ("/Marshalling/timerange", timerange_test);
+    Test.add_func ("/Marshalling/corrupt_events", corrupt_events_test);
+    Test.add_func ("/Marshalling/corrupt_subjects", corrupt_subjects_test);
+    Test.add_func ("/Marshalling/corrupt_timerange", corrupt_timerange_test);
+
+    return Test.run ();
 }
 
 Subject create_subject ()
 {
-  var s = new Subject ();
-  s.uri = "scheme:///uri";
-  s.interpretation = "subject_interpretation_uri";
-  s.manifestation = "subject_manifestation_uri";
-  s.mimetype = "text/plain";
-  s.origin = "scheme:///";
-  s.text = "Human readable text";
-  s.storage = "";
-  s.current_uri = "scheme:///uri";
+    var s = new Subject ();
+    s.uri = "scheme:///uri";
+    s.interpretation = "subject_interpretation_uri";
+    s.manifestation = "subject_manifestation_uri";
+    s.mimetype = "text/plain";
+    s.origin = "scheme:///";
+    s.text = "Human readable text";
+    s.storage = "";
+    s.current_uri = "scheme:///uri";
 
-  return s;
+    return s;
 }
 
 Event create_event ()
 {
-  var e = new Event ();
-  e.id = 1234;
-  e.timestamp = 1234567890L;
-  e.interpretation = "interpretation_uri";
-  e.manifestation = "manifestation_uri";
-  e.actor = "test.desktop";
-  e.origin = "source";
+    var e = new Event ();
+    e.id = 1234;
+    e.timestamp = 1234567890L;
+    e.interpretation = "interpretation_uri";
+    e.manifestation = "manifestation_uri";
+    e.actor = "test.desktop";
+    e.origin = "source";
 
-  return e;
+    return e;
 }
 
 void subject_test ()
 {
-  for (int i=0; i<1000; i++)
-  {
-    Variant vsubject = create_subject ().to_variant ();
-    var subject = new Subject.from_variant (vsubject);
-    warn_if_fail (subject != null);
-  }
+    for (int i = 0; i < 1000; i++)
+    {
+        Variant vsubject = create_subject ().to_variant ();
+        var subject = new Subject.from_variant (vsubject);
+        warn_if_fail (subject != null);
+    }
 }
 
 void event_test ()
 {
-  for (int i=0; i<1000; i++)
-  {
-    Variant vevent = create_event ().to_variant ();
-    var event = new Event.from_variant (vevent);
-    warn_if_fail (event != null);
-  }
+    for (int i = 0; i < 1000; i++)
+    {
+        Variant vevent = create_event ().to_variant ();
+        var event = new Event.from_variant (vevent);
+        warn_if_fail (event != null);
+    }
 }
 
 void events_test ()
 {
-  GenericArray<Event> events = new GenericArray<Event> ();
-  for (int i=0; i<1000; i++)
-  {
-    var e = create_event ();
-    e.add_subject (create_subject ());
-    events.add (e);
-  }
-
-  Variant vevents = Events.to_variant (events);
-
-  var demarshalled = Events.from_variant (vevents);
-  assert (demarshalled.length == 1000);
+    GenericArray<Event> events = new GenericArray<Event> ();
+    for (int i = 0; i < 1000; i++)
+    {
+        var e = create_event ();
+        e.add_subject (create_subject ());
+        events.add (e);
+    }
+
+    Variant vevents = Events.to_variant (events);
+
+    var demarshalled = Events.from_variant (vevents);
+    assert (demarshalled.length == 1000);
+}
+
+void timerange_test ()
+{
+    for (int i = 0; i < 1000; i++)
+    {
+        Variant v = new Variant("(xx)", i, i+42);
+        TimeRange timerange = new TimeRange.from_variant (v);
+        assert (timerange.start == i);
+        assert (timerange.end == i+42);
+    }
+}
+
+void corrupt_events_test ()
+{
+    // Let's just try to parse some crap and see that it does not crash :)
+    Variant v = new Variant ("(s)", "Zeitgeist is so awesome");
+    bool error_thrown = false;
+    try
+    {
+        new Event.from_variant (v);
+    }
+    catch (EngineError.INVALID_SIGNATURE err) {
+        error_thrown = true;
+    }
+    assert (error_thrown);
+}
+
+void corrupt_subjects_test ()
+{
+    Variant v;
+    string[] arr;
+    bool error_thrown;
+
+    // Parse a valid subject variant
+    arr = { "uri", "interpretation", "manifestation", "origin",
+        "mimetype", "text", "storage", "current_uri" };
+    v = new Variant.strv (arr);
+    new Subject.from_variant (v);
+
+    // Another valid variant, but this time without current_uri
+    arr = { "uri", "interpretation", "manifestation", "origin",
+        "mimetype", "text", "storage" };
+    v = new Variant.strv (arr);
+    new Subject.from_variant (v);
+
+    // And this one is not valid :(
+    arr = { "uri", "interpretation", "manifestation", "origin",
+        "mimetype", "text" };
+    v = new Variant.strv (arr);
+    error_thrown = false;
+    try
+    {
+        new Subject.from_variant (v);
+    }
+    catch (EngineError.INVALID_SIGNATURE err)
+    {
+        error_thrown = true;
+    }
+    assert (error_thrown);
+
+    // Those one is just insane :)
+    v = new Variant ("(x)", 42);
+    error_thrown = false;
+    try
+    {
+        new Subject.from_variant (v);
+    }
+    catch (EngineError.INVALID_SIGNATURE err)
+    {
+        error_thrown = true;
+    }
+    assert (error_thrown);
+}
+
+void corrupt_timerange_test ()
+{
+    Variant v = new Variant ("(s)", "oh noes, what is this?");
+    bool error_thrown = false;
+    try
+    {
+        new TimeRange.from_variant (v);
+    }
+    catch (EngineError.INVALID_SIGNATURE err)
+    {
+        error_thrown = true;
+    }
 }