Properly handle extensions without id or name

We used to fail silently, crashing later-on.

3 files changed, 113 insertions, 101 deletions

diff --git a/src/extension/extension.cpp b/src/extension/extension.cpp
index cf1ccf2de..85ea99ca8 100644
--- a/src/extension/extension.cpp
+++ b/src/extension/extension.cpp
@@ -54,12 +54,9 @@ Extension::Extension (Inkscape::XML::Node *in_repr, Implementation::Implementati
     : _gui(true)
     , execution_env(nullptr)
 {
+    g_return_if_fail(in_repr); // should be ensured in system.cpp
     repr = in_repr;
-    Inkscape::GC::anchor(in_repr);
-
-    id = nullptr;
-    name = nullptr;
-    _state = STATE_UNLOADED;
+    Inkscape::GC::anchor(repr);
 
     if (in_imp == nullptr) {
         imp = new Implementation::Implementation();
@@ -68,47 +65,60 @@ Extension::Extension (Inkscape::XML::Node *in_repr, Implementation::Implementati
     }
 
     // Read XML tree and parse extension
-    if (repr) {
-        Inkscape::XML::Node *child_repr = repr->firstChild();
-        while (child_repr) {
-            const char *chname = child_repr->name();
-			if (!strncmp(chname, INKSCAPE_EXTENSION_NS_NC, strlen(INKSCAPE_EXTENSION_NS_NC))) {
-				chname += strlen(INKSCAPE_EXTENSION_NS);
-			}
-            if (chname[0] == '_') { // allow leading underscore in tag names for backwards-compatibility
-                chname++;
-            }
+    Inkscape::XML::Node *child_repr = repr->firstChild();
+    while (child_repr) {
+        const char *chname = child_repr->name();
+        if (!strncmp(chname, INKSCAPE_EXTENSION_NS_NC, strlen(INKSCAPE_EXTENSION_NS_NC))) {
+            chname += strlen(INKSCAPE_EXTENSION_NS);
+        }
+        if (chname[0] == '_') { // allow leading underscore in tag names for backwards-compatibility
+            chname++;
+        }
 
-            /* TODO: Handle what happens if we don't have name and id */
-            if (!strcmp(chname, "id")) {
-                const char *value = child_repr->firstChild()->content();
-                id = g_strdup(value);
-            } else if (!strcmp(chname, "name")) {
-                name = g_strdup(child_repr->firstChild()->content());
-            } else if (!strcmp(chname, "param")) {
-                Parameter *param = Parameter::make(child_repr, this);
-                if (param) {
-                    parameters.push_back(param);
-                }
-            } else if (!strcmp(chname, "dependency")) {
-                _deps.push_back(new Dependency(child_repr));
-            } else if (!strcmp(chname, "script")) { // check command as a dependency (see LP #505920)
-                for (Inkscape::XML::Node *child = child_repr->firstChild(); child != nullptr; child = child->next()) {
-                    if (child->type() == Inkscape::XML::ELEMENT_NODE) { // skip non-element nodes (see LP #1372200)
-                        _deps.push_back(new Dependency(child));
-                        break;
-                    }
-                }
+        if (!strcmp(chname, "id")) {
+            const char *id = child_repr->firstChild() ? child_repr->firstChild()->content() : nullptr;
+            if (id) {
+                _id = g_strdup(id);
             } else {
-                // We could do some sanity checking here.
-                // However, we don't really know which additional elements Extension subclasses might need...
+                throw extension_no_id();
             }
-
-            child_repr = child_repr->next();
+        } else if (!strcmp(chname, "name")) {
+            const char *name = child_repr->firstChild() ? child_repr->firstChild()->content() : nullptr;
+            if (name) {
+                _name = g_strdup(name);
+            } else {
+                throw extension_no_name();
+            }
+        } else if (!strcmp(chname, "param")) {
+            Parameter *param = Parameter::make(child_repr, this);
+            if (param) {
+                parameters.push_back(param);
+            }
+        } else if (!strcmp(chname, "dependency")) {
+            _deps.push_back(new Dependency(child_repr));
+        } else if (!strcmp(chname, "script")) { // check command as a dependency (see LP #505920)
+            for (Inkscape::XML::Node *child = child_repr->firstChild(); child != nullptr; child = child->next()) {
+                if (child->type() == Inkscape::XML::ELEMENT_NODE) { // skip non-element nodes (see LP #1372200)
+                    _deps.push_back(new Dependency(child));
+                    break;
+                }
+            }
+        } else {
+            // We could do some sanity checking here.
+            // However, we don't really know which additional elements Extension subclasses might need...
         }
 
-        db.register_ext (this);
+        child_repr = child_repr->next();
+    }
+
+    // register extension if we have an id and a name
+    if (!_id) {
+        throw extension_no_id();
+    }
+    if (!_name) {
+        throw extension_no_name();
     }
+    db.register_ext (this);
 
     timer = nullptr;
 }
@@ -130,8 +140,8 @@ Extension::~Extension ()
 
     Inkscape::GC::release(repr);
 
-    g_free(id);
-    g_free(name);
+    g_free(_id);
+    g_free(_name);
 
     delete timer;
     timer = nullptr;
@@ -242,9 +252,6 @@ Extension::check ()
 {
     bool retval = true;
 
-    // static int i = 0;
-    // std::cout << "Checking module[" << i++ << "]: " << name << std::endl;
-
     const char * inx_failure = _("  This is caused by an improper .inx file for this extension."
                                  "  An improper .inx file could have been caused by a faulty installation of Inkscape.");
 
@@ -253,20 +260,12 @@ Extension::check ()
 #ifndef _WIN32
     const char* win_ext[] = {"com.vaxxine.print.win32"};
     std::vector<std::string> v (win_ext, win_ext + sizeof(win_ext)/sizeof(win_ext[0]));
-    std::string ext_id(id);
+    std::string ext_id(_id);
     if (std::find(v.begin(), v.end(), ext_id) != v.end()) {
         printFailure(Glib::ustring(_("the extension is designed for Windows only.")) + inx_failure);
         retval = false;
     }
 #endif
-    if (id == nullptr) {
-        printFailure(Glib::ustring(_("an ID was not defined for it.")) + inx_failure);
-        retval = false;
-    }
-    if (name == nullptr) {
-        printFailure(Glib::ustring(_("there was no name defined for it.")) + inx_failure);
-        retval = false;
-    }
     if (repr == nullptr) {
         printFailure(Glib::ustring(_("the XML description of it got lost.")) + inx_failure);
         retval = false;
@@ -299,7 +298,7 @@ Extension::check ()
 void
 Extension::printFailure (Glib::ustring reason)
 {
-    error_file << _("Extension \"") << name << _("\" failed to load because ");
+    error_file << _("Extension \"") << _name << _("\" failed to load because ");
     error_file << reason.raw();
     error_file << std::endl;
     return;
@@ -322,7 +321,7 @@ Extension::get_repr ()
 gchar *
 Extension::get_id ()
 {
-    return id;
+    return _id;
 }
 
 /**
@@ -332,7 +331,7 @@ Extension::get_id ()
 gchar *
 Extension::get_name ()
 {
-    return name;
+    return _name;
 }
 
 /**
@@ -747,8 +746,8 @@ Extension::get_info_widget()
     info->add(*table);
 
     int row = 0;
-    add_val(_("Name:"), _(name), table, &row);
-    add_val(_("ID:"), id, table, &row);
+    add_val(_("Name:"), _(_name), table, &row);
+    add_val(_("ID:"), _id, table, &row);
     add_val(_("State:"), _state == STATE_LOADED ? _("Loaded") : _state == STATE_UNLOADED ? _("Unloaded") : _("Deactivated"), table, &row);
 
     retval->show_all();
diff --git a/src/extension/extension.h b/src/extension/extension.h
index e544dcccd..23396fc27 100644
--- a/src/extension/extension.h
+++ b/src/extension/extension.h
@@ -99,9 +99,9 @@ public:
     };
 
 private:
-    gchar     *id;                        /**< The unique identifier for the Extension */
-    gchar     *name;                      /**< A user friendly name for the Extension */
-    state_t    _state;                    /**< Which state the Extension is currently in */
+    gchar     *_id = nullptr;             /**< The unique identifier for the Extension */
+    gchar     *_name = nullptr;           /**< A user friendly name for the Extension */
+    state_t    _state = STATE_UNLOADED;   /**< Which state the Extension is currently in */
     std::vector<Dependency *>  _deps;     /**< Dependencies for this extension */
     static std::ofstream error_file;      /**< This is the place where errors get reported */
     bool _gui;
@@ -158,6 +158,12 @@ public:
      * simply doesn't exist */
     class param_not_exist {};
 
+    /** no valid ID found while parsing XML representation */
+    class extension_no_id{};
+
+    /** no valid name found while parsing XML representation */
+    class extension_no_name{};
+
     /** An error class for when a filename already exists, but the user
      * doesn't want to overwrite it */
     class no_overwrite {};
@@ -192,19 +198,19 @@ public:
     bool        get_param_bool          (const gchar *name,
                                          const SPDocument *doc = nullptr,
                                          const Inkscape::XML::Node *node = nullptr) const;
-                                        
+
     int         get_param_int           (const gchar *name,
                                          const SPDocument *doc = nullptr,
                                          const Inkscape::XML::Node *node = nullptr) const;
-                                        
+
     float       get_param_float         (const gchar *name,
                                          const SPDocument *doc = nullptr,
                                          const Inkscape::XML::Node *node = nullptr) const;
-                                        
+
     const char *get_param_string        (const gchar *name,
                                          const SPDocument *doc = nullptr,
                                          const Inkscape::XML::Node *node = nullptr) const;
-                                        
+
     const char *get_param_optiongroup   (const gchar *name,
                                          const SPDocument *doc = nullptr,
                                          const Inkscape::XML::Node *node = nullptr) const;
@@ -212,36 +218,36 @@ public:
                                          const char *value,
                                          const SPDocument *doc = nullptr,
                                          const Inkscape::XML::Node * node = nullptr) const;
-                                        
+
     guint32     get_param_color         (const gchar *name,
                                          const SPDocument *doc = nullptr,
                                          const Inkscape::XML::Node *node = nullptr) const;
-                                        
+
     bool        set_param_bool          (const gchar *name,
                                          const bool value,
                                          SPDocument *doc = nullptr,
                                          Inkscape::XML::Node *node = nullptr);
-                                        
+
     int         set_param_int           (const gchar *name,
                                          const int value,
                                          SPDocument *doc = nullptr,
                                          Inkscape::XML::Node *node = nullptr);
-                                        
+
     float       set_param_float         (const gchar *name,
                                          const float value,
                                          SPDocument *doc = nullptr,
                                          Inkscape::XML::Node *node = nullptr);
-                                        
+
     const char *set_param_string        (const gchar *name,
                                          const char *value,
                                          SPDocument *doc = nullptr,
                                          Inkscape::XML::Node *node = nullptr);
-                                        
+
     const char *set_param_optiongroup   (const gchar *name,
                                          const char *value,
                                          SPDocument *doc = nullptr,
                                          Inkscape::XML::Node *node = nullptr);
-                                        
+
     guint32     set_param_color         (const gchar *name,
                                          const guint32 color,
                                          SPDocument *doc = nullptr,
diff --git a/src/extension/system.cpp b/src/extension/system.cpp
index 05a62884c..370e30f00 100644
--- a/src/extension/system.cpp
+++ b/src/extension/system.cpp
@@ -508,31 +508,38 @@ build_from_reprdoc(Inkscape::XML::Document *doc, Implementation::Implementation
     }
 
     Extension *module = nullptr;
-    switch (module_functional_type) {
-        case MODULE_INPUT: {
-            module = new Input(repr, imp);
-            break;
-        }
-        case MODULE_OUTPUT: {
-            module = new Output(repr, imp);
-            break;
-        }
-        case MODULE_FILTER: {
-            module = new Effect(repr, imp);
-            break;
-        }
-        case MODULE_PRINT: {
-            module = new Print(repr, imp);
-            break;
-        }
-        case MODULE_PATH_EFFECT: {
-            module = new PathEffect(repr, imp);
-            break;
-        }
-        default: {
-            module = new Extension(repr, imp);
-            break;
+    try {
+        switch (module_functional_type) {
+            case MODULE_INPUT: {
+                module = new Input(repr, imp);
+                break;
+            }
+            case MODULE_OUTPUT: {
+                module = new Output(repr, imp);
+                break;
+            }
+            case MODULE_FILTER: {
+                module = new Effect(repr, imp);
+                break;
+            }
+            case MODULE_PRINT: {
+                module = new Print(repr, imp);
+                break;
+            }
+            case MODULE_PATH_EFFECT: {
+                module = new PathEffect(repr, imp);
+                break;
+            }
+            default: {
+                g_warning("Extension of unkonw type!"); // TODO: Should not happen! Is this even useful?
+                module = new Extension(repr, imp);
+                break;
+            }
         }
+    } catch (const Extension::extension_no_id& e) {
+        g_warning("Building extension failed. Extension does not have a valid ID");
+    } catch (const Extension::extension_no_name& e) {
+        g_warning("Building extension failed. Extension does not have a valid name");
     }
 
     return module;
@@ -552,10 +559,10 @@ build_from_file(gchar const *filename)
     Inkscape::XML::Document *doc = sp_repr_read_file(filename, INKSCAPE_EXTENSION_URI);
     std::string dir = Glib::path_get_dirname(filename);
     Extension *ext = build_from_reprdoc(doc, nullptr, &dir);
-    if (ext != nullptr)
-        Inkscape::GC::release(doc);
-    else
+    Inkscape::GC::release(doc);
+    if (!ext) {
         g_warning("Unable to create extension from definition file %s.\n", filename);
+    }
     return ext;
 }
 
@@ -646,7 +653,7 @@ get_file_save_path (SPDocument *doc, FileSaveMethod method) {
             }
             break;
         case FILE_SAVE_METHOD_EXPORT:
-            /// \todo no default path set for Export? 
+            /// \todo no default path set for Export?
             // defaults to g_get_home_dir()
             break;
     }