From 0572dfd787364f027ee41f3e1d9fa1293769cdca Mon Sep 17 00:00:00 2001 From: Dan Breslau Date: Mon, 8 Aug 2016 21:34:09 -0400 Subject: [PATCH] Refactored the IdCard services list to fix new bugs and (hopefully) prevent even newer ones. --- src/moonshot-id.vala | 73 +++++++++++++++++++++++++----- src/moonshot-idcard-widget.vala | 11 +---- src/moonshot-identities-manager.vala | 12 ++++- src/moonshot-identity-dialog.vala | 21 ++------- src/moonshot-identity-management-view.vala | 9 ++-- src/moonshot-identity-manager-app.vala | 49 ++++---------------- src/moonshot-identity-request.vala | 12 ++--- src/moonshot-keyring-store.vala | 14 ++++-- src/moonshot-local-flat-file-store.vala | 13 ++++-- src/moonshot-provisioning-common.vala | 2 +- src/moonshot-server.vala | 19 +++++++- src/moonshot-webp-parser.vala | 8 +++- 12 files changed, 143 insertions(+), 100 deletions(-) diff --git a/src/moonshot-id.vala b/src/moonshot-id.vala index 45f3a83..ec0866c 100644 --- a/src/moonshot-id.vala +++ b/src/moonshot-id.vala @@ -29,6 +29,9 @@ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. */ + +using Gee; + public class TrustAnchor : Object { public string ca_cert {get; set; default = "";} @@ -64,6 +67,8 @@ public struct Rule public class IdCard : Object { + static MoonshotLogger logger = get_logger("IdCard"); + public const string NO_IDENTITY = "No Identity"; private string _nai; @@ -98,12 +103,60 @@ public class IdCard : Object internal set {_rules = value ?? new Rule[0] ;} } - private string[] _services = new string[0]; - public string[] services { - get {return _services;} - internal set {_services = value ?? new string[0] ;} + private ArrayList _services = new ArrayList(); + + internal ArrayList services { + get {return _services;} } + // Returns the list of services as a string, using the given separator. + internal string get_services_string(string sep) { + if (_services.is_empty) { + return ""; + } + + // ArrayList.to_array() seems to be unreliable -- it causes segfaults + // semi-randomly. (Possibly because it returns an unowned ref?) + // return string.joinv(sep, _services.to_array()); + // + // This problem may be related to the one noted elsewhere as the + // "Centos vala array property bug". + + string[] svcs = new string[_services.size]; + for (int i = 0; i < _services.size; i++) { + svcs[i] = _services[i]; + } + + return string.joinv(sep, svcs); + } + + internal void update_services(string[] services) { + _services.clear(); + + // Doesn't exist in older versions of libgee: + // _services.add_all_array(services); + + if (services != null) { + foreach (string s in services) { + _services.add(s); + } + } + } + + internal void update_services_from_list(ArrayList services) { + if (services == this._services) { + // Don't try to update from self. + return; + } + + _services.clear(); + + if (services != null) { + _services.add_all(services); + } + } + + public bool temporary {get; set; default = false; } public TrustAnchor trust_anchor { get; set; default = new TrustAnchor (); } @@ -145,7 +198,7 @@ public class IdCard : Object if (CompareRules(this.rules, other.rules)!=0) diff |= 1 << DiffFlags.RULES; - if (CompareStringArray(this.services, other.services)!=0) + if (CompareStringArrayList(this._services, other._services)!=0) diff |= 1 << DiffFlags.SERVICES; if (this.trust_anchor.Compare(other.trust_anchor)!=0) @@ -169,10 +222,6 @@ public class IdCard : Object internal void add_rule(Rule rule) { _rules += rule; } - - internal void add_service(string service) { - _services += service; - } } public int CompareRules(Rule[] a, Rule[] b) @@ -189,13 +238,13 @@ public int CompareRules(Rule[] a, Rule[] b) return 0; } -public int CompareStringArray(string[] a, string [] b) +public int CompareStringArrayList(ArrayList a, ArrayList b) { - if (a.length != b.length) { + if (a.size != b.size) { return 1; } - for (int i = 0; i < a.length; i++) { + for (int i = 0; i < a.size; i++) { if (a[i] != b[i]) { return 1; } diff --git a/src/moonshot-idcard-widget.vala b/src/moonshot-idcard-widget.vala index 887dbf6..7a34cf2 100644 --- a/src/moonshot-idcard-widget.vala +++ b/src/moonshot-idcard-widget.vala @@ -123,7 +123,7 @@ class IdCardWidget : Box { // !!TODO: Use a table to format the labels and values string services_text = "Services: "; - string service_spacer = " "; + string service_spacer = "\n "; var label_text = Markup.printf_escaped("%s", this.id_card.display_name); @@ -132,14 +132,7 @@ class IdCardWidget : Box label_text += "\nUsername: " + id_card.username; label_text += "\nRealm: " + id_card.issuer; - var sep = ""; - for (int i = 0; i < id_card.services.length; i++) - { - services_text += sep; - services_text += id_card.services[i]; - - sep = "\n" + service_spacer; - } + services_text += this.id_card.get_services_string(service_spacer); label_text += "\n" + services_text; } diff --git a/src/moonshot-identities-manager.vala b/src/moonshot-identities-manager.vala index 1f390cc..8154052 100644 --- a/src/moonshot-identities-manager.vala +++ b/src/moonshot-identities-manager.vala @@ -182,8 +182,10 @@ public class IdentityManagerModel : Object { } public void add_card(IdCard card, bool force_flat_file_store) { - if (card.temporary) + if (card.temporary) { + logger.trace("add_card: card is temporary; returning."); return; + } string candidate; IIdentityCardStore.StoreType saved_store_type = get_store_type(); @@ -200,12 +202,18 @@ public class IdentityManagerModel : Object { if (!card.store_password) password_table.CachePassword(card, store); + + logger.trace("add_card: Storing card '%s' with services: '%s'" + .printf(card.display_name, card.get_services_string("; "))); + store.add_card(card); set_store_type(saved_store_type); card_list_changed(); } public IdCard update_card(IdCard card) { + logger.trace("update_card"); + IdCard retval; if (card.temporary) { retval = card; @@ -261,7 +269,7 @@ public class IdentityManagerModel : Object { // The 'NoIdentity' card is non-trivial if it has services or rules. // All other cards are automatically non-trivial. if ((!card.is_no_identity()) || - (card.services.length > 0) || + (card.services.size > 0) || (card.rules.length > 0)) { return true; } diff --git a/src/moonshot-identity-dialog.vala b/src/moonshot-identity-dialog.vala index be4914e..fab8f3d 100644 --- a/src/moonshot-identity-dialog.vala +++ b/src/moonshot-identity-dialog.vala @@ -29,6 +29,8 @@ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. */ + +using Gee; using Gtk; @@ -87,7 +89,7 @@ class IdentityDialog : Dialog get { return remember_checkbutton.active; } } - internal string[] get_services() + internal ArrayList get_services() { return card.services; } @@ -300,7 +302,7 @@ class IdentityDialog : Dialog remove_button.set_sensitive(false); - var services_table = new Table(card.services.length, 1, false); + var services_table = new Table(card.services.size, 1, false); services_table.set_row_spacings(1); services_table.set_col_spacings(0); services_table.modify_bg(StateType.NORMAL, white); @@ -383,20 +385,7 @@ class IdentityDialog : Dialog if (result) { if (card != null) { - SList services = new SList(); - - foreach (string srv in card.services) - { - if (srv != selected_item.label) - services.append(srv); - } - - card.services = new string[services.length()]; - for (int j = 0; j < card.services.length; j++) - { - card.services[j] = services.nth_data(j); - } - + card.services.remove(selected_item.label); services_table.remove(selected_item.parent); selected_item = null; remove_button.set_sensitive(false); diff --git a/src/moonshot-identity-management-view.vala b/src/moonshot-identity-management-view.vala index 35dcd5f..cea963b 100644 --- a/src/moonshot-identity-management-view.vala +++ b/src/moonshot-identity-management-view.vala @@ -151,7 +151,7 @@ public class IdentityManagerView : Window { return true; } - if (id_card.services.length > 0) + if (id_card.services.size > 0) { foreach (string service in id_card.services) { @@ -226,6 +226,7 @@ public class IdentityManagerView : Window { } foreach (IdCard id_card in card_list) { + logger.trace(@"load_id_cards: Adding card with display name '$(id_card.display_name)'"); add_id_card_data(id_card); IdCardWidget id_card_widget = add_id_card_widget(id_card); if (id_card_widget.id_card.nai == current_idcard_nai) { @@ -242,7 +243,8 @@ public class IdentityManagerView : Window { id_card.username = dialog.username; id_card.password = dialog.password; id_card.store_password = dialog.store_password; - id_card.services = dialog.get_services(); + + id_card.update_services_from_list(dialog.get_services()); return id_card; } @@ -322,7 +324,8 @@ public class IdentityManagerView : Window { #else Gtk.MessageDialog dialog; IdCard? prev_id = identities_manager.find_id_card(id_card.nai, force_flat_file_store); - logger.trace("add_identity: find_id_card returned " + (prev_id != null ? "non-null" : "null")); + logger.trace("add_identity(flat=%s, card='%s'): find_id_card returned %s" + .printf(force_flat_file_store.to_string(), id_card.display_name, (prev_id != null ? "non-null" : "null"))); if (prev_id!=null) { int flags = prev_id.Compare(id_card); logger.trace("add_identity: compare returned " + flags.to_string()); diff --git a/src/moonshot-identity-manager-app.vala b/src/moonshot-identity-manager-app.vala index 4ac6354..39d84f5 100644 --- a/src/moonshot-identity-manager-app.vala +++ b/src/moonshot-identity-manager-app.vala @@ -125,7 +125,13 @@ public class IdentityManagerApp { } public bool add_identity(IdCard id, bool force_flat_file_store) { - if (view != null) return view.add_identity(id, force_flat_file_store); + if (view != null) + { + logger.trace("add_identity: calling view.add_identity"); + return view.add_identity(id, force_flat_file_store); + } + + logger.trace("add_identity: calling model.add_card"); model.add_card(id, force_flat_file_store); return true; } @@ -158,13 +164,8 @@ public class IdentityManagerApp { /* If any service matches we add id card to the candidate list */ if (has_srv) { - foreach (string srv in id.services) - { - if (request.service == srv) - { - request.candidates.append(id); - continue; - } + if (id.services.contains(request.service)) { + request.candidates.append(id); } } } @@ -174,37 +175,7 @@ public class IdentityManagerApp { { foreach (IdCard id in request.candidates) { - int i = 0; - SList services_list = null; - bool has_service = false; - - foreach (string srv in id.services) - { - if (srv == request.service) - { - has_service = true; - continue; - } - services_list.append(srv); - } - - if (!has_service) - continue; - - if (services_list.length() == 0) - { - id.services = {}; - continue; - } - - string[] services = new string[services_list.length()]; - foreach (string srv in services_list) - { - services[i] = srv; - i++; - } - - id.services = services; + id.services.remove(request.service); } } diff --git a/src/moonshot-identity-request.vala b/src/moonshot-identity-request.vala index 2e1c132..2e7ceed 100644 --- a/src/moonshot-identity-request.vala +++ b/src/moonshot-identity-request.vala @@ -88,17 +88,13 @@ public class IdentityRequest : Object { /* update id_card service list */ if (update_card && id_card != null && this.service != null && this.service != "") { - bool duplicate_service = false; - - foreach (string service in id_card.services) - { - if (service == this.service) - duplicate_service = true; - } + bool duplicate_service = id_card.services.contains(this.service); logger.trace("return_identity: duplicate_service=" + duplicate_service.to_string()); if (duplicate_service == false) { - id_card.add_service(this.service); + logger.trace("return_identity: calling add_service"); + id_card.services.add(this.service); + logger.trace("return_identity: back from add_service"); this.id_card = this.parent_app.model.update_card(id_card); } diff --git a/src/moonshot-keyring-store.vala b/src/moonshot-keyring-store.vala index 6b36022..9045c95 100644 --- a/src/moonshot-keyring-store.vala +++ b/src/moonshot-keyring-store.vala @@ -33,19 +33,27 @@ using Gee; #if GNOME_KEYRING public class KeyringStore : Object, IIdentityCardStore { + static MoonshotLogger logger = get_logger("KeyringStore"); + private LinkedList id_card_list; private const string keyring_store_attribute = "Moonshot"; private const string keyring_store_version = "1.0"; private const GnomeKeyring.ItemType item_type = GnomeKeyring.ItemType.GENERIC_SECRET; public void add_card(IdCard card) { + logger.trace("add_card: Adding card '%s' with services: '%s'" + .printf(card.display_name, card.get_services_string("; "))); + id_card_list.add(card); store_id_cards(); } public IdCard? update_card(IdCard card) { + logger.trace("update_card"); + id_card_list.remove(card); id_card_list.add(card); + store_id_cards(); foreach (IdCard idcard in id_card_list) { if (idcard.display_name == card.display_name) { @@ -107,7 +115,7 @@ public class KeyringStore : Object, IIdentityCardStore { } else if (attribute.name == "DisplayName") { id_card.display_name = value; } else if (attribute.name == "Services") { - id_card.services = value.split(";"); + id_card.update_services(value.split(";")); } else if (attribute.name == "Rules-Pattern") { rules_patterns_index = i; } else if (attribute.name == "Rules-AlwaysConfirm") { @@ -153,11 +161,11 @@ public class KeyringStore : Object, IIdentityCardStore { } public void store_id_cards() { + logger.trace("store_id_cards"); clear_keyring(); foreach (IdCard id_card in this.id_card_list) { /* workaround for Centos vala array property bug: use temp array */ var rules = id_card.rules; - var services_array = id_card.services; string[] rules_patterns = new string[rules.length]; string[] rules_always_conf = new string[rules.length]; @@ -167,7 +175,7 @@ public class KeyringStore : Object, IIdentityCardStore { } string patterns = string.joinv(";", rules_patterns); string always_conf = string.joinv(";", rules_always_conf); - string services = string.joinv(";", services_array); + string services = id_card.get_services_string(";"); GnomeKeyring.AttributeList attributes = new GnomeKeyring.AttributeList(); uint32 item_id; attributes.append_string(keyring_store_attribute, keyring_store_version); diff --git a/src/moonshot-local-flat-file-store.vala b/src/moonshot-local-flat-file-store.vala index 8281d9d..c129072 100644 --- a/src/moonshot-local-flat-file-store.vala +++ b/src/moonshot-local-flat-file-store.vala @@ -88,7 +88,7 @@ public class LocalFlatFileStore : Object, IIdentityCardStore { id_card.issuer = key_file.get_string(identity, "Issuer"); id_card.username = key_file.get_string(identity, "Username"); id_card.password = key_file.get_string(identity, "Password"); - id_card.services = key_file.get_string_list(identity, "Services"); + id_card.update_services(key_file.get_string_list(identity, "Services")); id_card.display_name = key_file.get_string(identity, "DisplayName"); if (key_file.has_key(identity, "StorePassword")) { id_card.store_password = (key_file.get_string(identity, "StorePassword") == "yes"); @@ -140,7 +140,6 @@ public class LocalFlatFileStore : Object, IIdentityCardStore { foreach (IdCard id_card in this.id_card_list) { /* workaround for Centos vala array property bug: use temp arrays */ var rules = id_card.rules; - var services = id_card.services; string[] empty = {}; string[] rules_patterns = new string[rules.length]; string[] rules_always_conf = new string[rules.length]; @@ -157,7 +156,15 @@ public class LocalFlatFileStore : Object, IIdentityCardStore { key_file.set_string(id_card.display_name, "Password", id_card.password); else key_file.set_string(id_card.display_name, "Password", ""); - key_file.set_string_list(id_card.display_name, "Services", services ?? empty); + + // Using id_card.services.to_array() seems to cause a crash, possibly due to + // an unowned reference to the array. + string[] svcs = new string[id_card.services.size]; + for (int i = 0; i < id_card.services.size; i++) { + svcs[i] = id_card.services[i]; + } + + key_file.set_string_list(id_card.display_name, "Services", svcs); if (rules.length > 0) { key_file.set_string_list(id_card.display_name, "Rules-Patterns", rules_patterns); diff --git a/src/moonshot-provisioning-common.vala b/src/moonshot-provisioning-common.vala index 10e2366..87d7630 100644 --- a/src/moonshot-provisioning-common.vala +++ b/src/moonshot-provisioning-common.vala @@ -169,7 +169,7 @@ namespace WebProvisioning } else if (stack.nth_data(0) == "service") { - card.add_service(text); + card.services.add(text); } /* Rules */ diff --git a/src/moonshot-server.vala b/src/moonshot-server.vala index b1cc9ba..fb68864 100644 --- a/src/moonshot-server.vala +++ b/src/moonshot-server.vala @@ -197,12 +197,15 @@ public class MoonshotServer : Object { if ((password != null) && (password != "")) idcard.store_password = true; idcard.issuer = realm; - idcard.services = services; + idcard.update_services(services); idcard.trust_anchor.ca_cert = ca_cert; idcard.trust_anchor.subject = subject; idcard.trust_anchor.subject_alt = subject_alt; idcard.trust_anchor.server_cert = server_cert; + logger.trace("install_id_card: Card '%s' has services: '%s'" + .printf(idcard.display_name, idcard.get_services_string("; "))); + if (rules_patterns.length == rules_always_confirm.length) { /* workaround Centos vala array property bug: use temp array */ @@ -245,13 +248,25 @@ public class MoonshotServer : Object { } } + + // prevent a crash by holding the reference to otherwise + // unowned array(?) + + // string[] svcs = card.services.to_array(); + // string[] svcs = card.services.to_array()[:]; + string[] svcs = new string[card.services.size]; + for (int i = 0; i < card.services.size; i++) { + svcs[i] = card.services[i]; + } + + logger.trace(@"install_from_file: Adding card with display name '$(card.display_name)'"); result = install_id_card(card.display_name, card.username, card.password, card.issuer, rules_patterns, rules_always_confirm, - card.services, + svcs, card.trust_anchor.ca_cert, card.trust_anchor.subject, card.trust_anchor.subject_alt, diff --git a/src/moonshot-webp-parser.vala b/src/moonshot-webp-parser.vala index 90b5452..46ba9c6 100644 --- a/src/moonshot-webp-parser.vala +++ b/src/moonshot-webp-parser.vala @@ -90,7 +90,11 @@ namespace WebProvisioning /* use temp arrays to workaround centos array property bug */ var rules = card.rules; - var services = card.services; + string[] svcs = new string[card.services.size]; + for (int i = 0; i < card.services.size; i++) { + svcs[i] = card.services[i]; + } + if (rules.length > 0) { int i = 0; @@ -111,7 +115,7 @@ namespace WebProvisioning card.issuer, rules_patterns, rules_always_confirm, - services, + svcs, card.trust_anchor.ca_cert, card.trust_anchor.subject, card.trust_anchor.subject_alt, -- 2.1.4