GNOME.org
 

[gnumeric] Names: fix memory corruption.

commit f8ad082d199d245096144b7e5308c1fe8fc0f785
Author: Morten Welinder <terra gnome org>
Date:   Sun Mar 2 21:31:41 2014 -0500

    Names: fix memory corruption.

 ChangeLog                            |   15 +++
 NEWS                                 |    1 +
 plugins/excel/ms-excel-write.c       |    3 +-
 plugins/openoffice/openoffice-read.c |   17 ++--
 src/application.c                    |   18 ++++
 src/application.h                    |    2 +
 src/dependent.c                      |    2 +-
 src/expr-name.c                      |  158 ++++++++++++++++++++++++++++------
 src/expr-name.h                      |   16 +---
 src/ssconvert.c                      |    4 +-
 src/ssindex.c                        |    7 +-
 src/wbc-gtk.c                        |   13 +++-
 12 files changed, 207 insertions(+), 49 deletions(-)
---
diff --git a/ChangeLog b/ChangeLog
index c0178af..8558a64 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,20 @@
 2014-03-02  Morten Welinder  <terra gnome org>
 
+       * src/wbc-gtk.c (cb_workbook_debug_info): New debug flag
+       name-collections.
+
+       * src/expr-name.c (gnm_named_expr_collection_dump)
+       (gnm_named_expr_collection_sanity_check): New function.
+
+       * src/application.c (gnm_app_sanity_check): New function.
+
+       * src/expr-name.c (gnm_named_expr_collection_new): Don't use the
+       string inside the GOString as a hash key.  It can change.
+       (gnm_named_expr_collection_foreach): As a consequence of the
+       above, the first argument to the handler for
+       gnm_named_expr_collection_foreach changes to something
+       unspecified.  All callers changed.
+
        * src/sheet.c (sheet_dup): Don't flip display-outlines for the new
        sheet.
 
diff --git a/NEWS b/NEWS
index 0f0999f..94bd3b5 100644
--- a/NEWS
+++ b/NEWS
@@ -51,6 +51,7 @@ Morten:
        * Don't generate value format during xls read.  [#725453]
        * Fix name criticals.
        * Fix sheet duplication issue.  [#725504]
+       * Fix named expressions crasher.  [Part of #725459]
 
 --------------------------------------------------------------------------
 Gnumeric 1.12.11
diff --git a/plugins/excel/ms-excel-write.c b/plugins/excel/ms-excel-write.c
index 2e07ad0..1b8ac12 100644
--- a/plugins/excel/ms-excel-write.c
+++ b/plugins/excel/ms-excel-write.c
@@ -6413,7 +6413,8 @@ excel_write_v8 (ExcelWriteState *ewb, GsfOutfile *outfile)
 /****************************************************************************/
 
 static void
-cb_check_names (gpointer key, GnmNamedExpr *nexpr, ExcelWriteState *ewb)
+cb_check_names (G_GNUC_UNUSED gconstpointer key,
+               GnmNamedExpr *nexpr, ExcelWriteState *ewb)
 {
        if (expr_name_is_active (nexpr))
                excel_write_prep_expr (ewb, nexpr->texpr);
diff --git a/plugins/openoffice/openoffice-read.c b/plugins/openoffice/openoffice-read.c
index 3fa881a..732bf91 100644
--- a/plugins/openoffice/openoffice-read.c
+++ b/plugins/openoffice/openoffice-read.c
@@ -1824,16 +1824,18 @@ odf_fix_en_validate (char const *name, odf_fix_expr_names_t *fen)
 }
 
 static void
-odf_fix_en_collect (gchar const *key, G_GNUC_UNUSED GnmNamedExpr *nexpr, odf_fix_expr_names_t *fen)
+odf_fix_en_collect (G_GNUC_UNUSED gconstpointer key_,
+                   GnmNamedExpr *nexpr, odf_fix_expr_names_t *fen)
 {
        GString *str;
        gchar *here;
+       const char *name = expr_name_name (nexpr);
 
-       if (expr_name_validate (key))
+       if (expr_name_validate (name))
                return;
-       if (NULL != g_hash_table_lookup (fen->orig2fixed, key))
+       if (NULL != g_hash_table_lookup (fen->orig2fixed, name))
                return;
-       str = g_string_new (key);
+       str = g_string_new (name);
 
        for (here = str->str; *here; here = g_utf8_next_char (here)) {
                if (!g_unichar_isalnum (g_utf8_get_char (here)) &&
@@ -1845,13 +1847,14 @@ odf_fix_en_collect (gchar const *key, G_GNUC_UNUSED GnmNamedExpr *nexpr, odf_fix
        }
        while (!odf_fix_en_validate (str->str, fen))
                g_string_append_c (str, '_');
-       odf_fix_expr_names_t_add (fen, key, g_string_free (str, FALSE));
+       odf_fix_expr_names_t_add (fen, name, g_string_free (str, FALSE));
 }
 
 static void
-odf_fix_en_find (gchar const *key, GnmNamedExpr *nexpr, odf_fix_expr_names_t *fen)
+odf_fix_en_find (G_GNUC_UNUSED gconstpointer key,
+                GnmNamedExpr *nexpr, odf_fix_expr_names_t *fen)
 {
-       if (strcmp (key, fen->nexpr_name) == 0)
+       if (strcmp (expr_name_name (nexpr), fen->nexpr_name) == 0)
                fen->nexpr = nexpr;
 }
 
diff --git a/src/application.c b/src/application.c
index 33ed7eb..db6ba67 100644
--- a/src/application.c
+++ b/src/application.c
@@ -25,6 +25,8 @@
 #include "sheet-object.h"
 #include "commands.h"
 #include "gui-clipboard.h"
+#include "expr-name.h"
+#include "workbook-priv.h"
 
 #include <gnumeric-conf.h>
 #include <goffice/goffice.h>
@@ -154,6 +156,22 @@ gnm_app_workbook_list (void)
        return app->workbook_list;
 }
 
+void
+gnm_app_sanity_check (void)
+{
+       GList *l;
+       gboolean err = FALSE;
+       for (l = gnm_app_workbook_list (); l; l = l->next) {
+               Workbook *wb = l->data;
+               if (gnm_named_expr_collection_sanity_check (wb->names, "workbook"))
+                       err = TRUE;
+       }
+       if (err)
+               g_error ("Sanity check failed\n");
+}
+       
+
+
 /**
  * gnm_app_clipboard_clear:
  *
diff --git a/src/application.h b/src/application.h
index f88b4e8..af1d16b 100644
--- a/src/application.h
+++ b/src/application.h
@@ -24,6 +24,8 @@ Workbook    *gnm_app_workbook_get_by_index (int i);
 GSList      *gnm_app_history_get_list     (int max_elements);
 void        gnm_app_history_add           (char const *filename, const char *mimetype);
 
+void         gnm_app_sanity_check (void);
+
 void         gnm_app_recalc                (void);
 void         gnm_app_recalc_start          (void);
 void         gnm_app_recalc_finish         (void);
diff --git a/src/dependent.c b/src/dependent.c
index 174aaeb..ab9b974 100644
--- a/src/dependent.c
+++ b/src/dependent.c
@@ -2114,7 +2114,7 @@ struct cb_remote_names {
 };
 
 static void
-cb_remote_names1 (G_GNUC_UNUSED const char *name,
+cb_remote_names1 (G_GNUC_UNUSED gconstpointer key,
                  GnmNamedExpr *nexpr,
                  struct cb_remote_names *data)
 {
diff --git a/src/expr-name.c b/src/expr-name.c
index 1056367..c4e0185 100644
--- a/src/expr-name.c
+++ b/src/expr-name.c
@@ -174,6 +174,33 @@ expr_name_relink_deps (GnmNamedExpr *nexpr)
        g_slist_free (deps);
 }
 
+static guint
+fake_go_string_hash (gconstpointer s_)
+{
+       const GOString *s = s_;
+       return g_str_hash (s->str);
+}
+
+static gboolean
+fake_go_string_equal (gconstpointer a_, gconstpointer b_)
+{
+       const GOString *a = a_;
+       const GOString *b = b_;
+       return g_str_equal (a->str, b->str);
+}
+
+
+struct _GnmNamedExprCollection {
+       /* all the defined names */
+       GHashTable *names;
+
+       /* placeholders for references to undefined names */
+       GHashTable *placeholders;
+
+       /* <private> */
+       unsigned ref_count;     /* boxed type */
+};
+
 /**
  * gnm_named_expr_collection_new:
  *
@@ -184,9 +211,11 @@ gnm_named_expr_collection_new (void)
 {
        GnmNamedExprCollection *res = g_new (GnmNamedExprCollection, 1);
 
-       res->names = g_hash_table_new_full (g_str_hash, g_str_equal,
-               NULL, (GDestroyNotify) cb_nexpr_remove);
-       res->placeholders = g_hash_table_new_full (g_str_hash, g_str_equal,
+       res->names = g_hash_table_new_full
+               (fake_go_string_hash, fake_go_string_equal,
+                NULL, (GDestroyNotify) cb_nexpr_remove);
+       res->placeholders = g_hash_table_new_full
+               (fake_go_string_hash, fake_go_string_equal,
                NULL, (GDestroyNotify) cb_nexpr_remove);
        res->ref_count = 1;
 
@@ -221,6 +250,76 @@ gnm_named_expr_collection_ref (GnmNamedExprCollection *names)
        return names;
 }
 
+void
+gnm_named_expr_collection_dump (GnmNamedExprCollection *names, const char *id)
+{
+       g_printerr ("Named collection %s\n", id);
+       if (!names) {
+               g_printerr ("  Empty\n");
+               return;
+       }
+
+       if (names->names && g_hash_table_size (names->names)) {
+               GHashTableIter hiter;
+               gpointer key, value;
+
+               g_printerr ("  Defined names:\n");
+               g_hash_table_iter_init (&hiter, names->names);
+               while (g_hash_table_iter_next (&hiter, &key, &value)) {
+                       const GOString *name = key;
+                       GnmNamedExpr const *nexpr = value;
+                       g_printerr ("    [%s] =>\n", name->str);
+                       if (name != nexpr->name)
+                               g_printerr ("      Weird keys: %p vs %p\n",
+                                           name, nexpr->name);
+               }
+       }
+
+       if (names->placeholders && g_hash_table_size (names->placeholders)) {
+               GHashTableIter hiter;
+               gpointer key, value;
+
+               g_printerr ("  Defined placeholders:\n");
+               g_hash_table_iter_init (&hiter, names->placeholders);
+               while (g_hash_table_iter_next (&hiter, &key, &value)) {
+                       const GOString *name = key;
+                       GnmNamedExpr const *nexpr = value;
+                       g_printerr ("    [%s] =>\n", name->str);
+                       if (name != nexpr->name)
+                               g_printerr ("      Weird keys: %p vs %p\n",
+                                           name, nexpr->name);
+               }
+       }
+}
+
+gboolean
+gnm_named_expr_collection_sanity_check (GnmNamedExprCollection *names,
+                                       const char *id)
+{
+       gboolean err = FALSE;
+       g_printerr ("Checking sanity for container %s\n", id);
+       if (names->names) {
+               GHashTableIter hiter;
+               gpointer key, value;
+
+               g_hash_table_iter_init (&hiter, names->names);
+               while (g_hash_table_iter_next (&hiter, &key, &value)) {
+                       const GOString *name = key;
+                       GnmNamedExpr const *nexpr = value;
+                       if (name != nexpr->name) {
+                               err = TRUE;
+                               g_printerr ("Container %s has strange defined name\n",
+                                           id);
+                               g_printerr ("  key is %p [%s]\n",
+                                           name, name->str);
+                               g_printerr ("  target's name is %p [%s]\n",
+                                           nexpr->name, nexpr->name->str);
+                       }
+               }
+       }
+       return err;
+}
+
 GType
 gnm_named_expr_collection_get_type (void)
 {
@@ -280,9 +379,14 @@ gnm_named_expr_collection_lookup (GnmNamedExprCollection const *scope,
                                  char const *name)
 {
        if (scope != NULL) {
-               GnmNamedExpr *nexpr = g_hash_table_lookup (scope->names, name);
+               GOString fake_name;
+               GnmNamedExpr *nexpr;
+
+               fake_name.str = name;
+               nexpr = g_hash_table_lookup (scope->names, &fake_name);
                if (nexpr == NULL)
-                       nexpr = g_hash_table_lookup (scope->placeholders, name);
+                       nexpr = g_hash_table_lookup (scope->placeholders,
+                                                    &fake_name);
                return nexpr;
        } else
                return NULL;
@@ -333,8 +437,9 @@ gnm_named_expr_collection_insert (GnmNamedExprCollection *scope,
        /* name can be active at this point, eg we are converting a
         * placeholder, or changing a scope */
        nexpr->scope = scope;
-       g_hash_table_replace (nexpr->is_placeholder
-             ? scope->placeholders : scope->names, (gpointer)nexpr->name->str, nexpr);
+       g_hash_table_replace
+               (nexpr->is_placeholder ? scope->placeholders : scope->names,
+                (gpointer)nexpr->name, nexpr);
 }
 
 typedef struct {
@@ -620,12 +725,15 @@ expr_name_add (GnmParsePos const *pp, char const *name,
 {
        GnmNamedExpr *nexpr = NULL;
        GnmNamedExprCollection *scope = NULL;
+       GOString fake_name;
 
        g_return_val_if_fail (pp != NULL, NULL);
        g_return_val_if_fail (pp->sheet != NULL || pp->wb != NULL, NULL);
        g_return_val_if_fail (name != NULL, NULL);
        g_return_val_if_fail (stub == NULL || stub->is_placeholder, NULL);
 
+       fake_name.str = name;
+
        if (texpr != NULL && expr_name_check_for_loop (name, texpr)) {
                gnm_expr_top_unref (texpr);
                if (error_msg)
@@ -635,7 +743,7 @@ expr_name_add (GnmParsePos const *pp, char const *name,
 
        scope = (pp->sheet != NULL) ? pp->sheet->names : pp->wb->names;
        /* see if there was a place holder */
-       nexpr = g_hash_table_lookup (scope->placeholders, name);
+       nexpr = g_hash_table_lookup (scope->placeholders, &fake_name);
        if (nexpr != NULL) {
                if (texpr == NULL) {
                        /* there was already a placeholder for this */
@@ -644,10 +752,10 @@ expr_name_add (GnmParsePos const *pp, char const *name,
                }
 
                /* convert the placeholder into a real name */
-               g_hash_table_steal (scope->placeholders, name);
+               g_hash_table_steal (scope->placeholders, &fake_name);
                nexpr->is_placeholder = FALSE;
        } else {
-               nexpr = g_hash_table_lookup (scope->names, name);
+               nexpr = g_hash_table_lookup (scope->names, &fake_name);
                /* If this is a permanent name, we may be adding it */
                /* on opening of a file, although */
                /* the name is already in place. */
@@ -767,7 +875,7 @@ expr_name_remove (GnmNamedExpr *nexpr)
 
        g_hash_table_remove (
                nexpr->is_placeholder ? nexpr->scope->placeholders : nexpr->scope->names,
-               nexpr->name->str);
+               nexpr->name);
 }
 
 const char *
@@ -790,6 +898,7 @@ expr_name_set_name (GnmNamedExpr *nexpr,
 {
        const char *old_name;
        GHashTable *h;
+       GOString fake_new_name;
 
        g_return_val_if_fail (nexpr != NULL, TRUE);
        g_return_val_if_fail (nexpr->scope == NULL || new_name, TRUE);
@@ -798,6 +907,7 @@ expr_name_set_name (GnmNamedExpr *nexpr,
        if (go_str_compare (new_name, old_name) == 0)
                return FALSE;
 
+       fake_new_name.str = new_name;
 #if 0
        g_printerr ("Renaming %s to %s\n", old_name, new_name);
 #endif
@@ -808,21 +918,23 @@ expr_name_set_name (GnmNamedExpr *nexpr,
                : NULL;
        if (h) {
                if (new_name &&
-                   (g_hash_table_lookup (nexpr->scope->placeholders, new_name) ||
-                    g_hash_table_lookup (nexpr->scope->names, new_name))) {
+                   (g_hash_table_lookup (nexpr->scope->placeholders,
+                                         &fake_new_name) ||
+                    g_hash_table_lookup (nexpr->scope->names,
+                                         &fake_new_name))) {
                        /* The only error not to be blamed on the programmer is
                           already-in-use.  */
                        return TRUE;
                }
 
-               g_hash_table_steal (h, old_name);
+               g_hash_table_steal (h, nexpr->name);
        }
 
        go_string_unref (nexpr->name);
        nexpr->name = go_string_new (new_name);
 
        if (h)
-               g_hash_table_insert (h, (gpointer)nexpr->name->str, nexpr);
+               g_hash_table_insert (h, (gpointer)nexpr->name, nexpr);
 
        return FALSE;
 }
@@ -889,7 +1001,6 @@ char *
 expr_name_set_pos (GnmNamedExpr *nexpr, GnmParsePos const *pp)
 {
        GnmNamedExprCollection *old_scope, *new_scope;
-       const char *name;
 
        g_return_val_if_fail (nexpr != NULL, NULL);
        g_return_val_if_fail (pp != NULL, NULL);
@@ -897,20 +1008,19 @@ expr_name_set_pos (GnmNamedExpr *nexpr, GnmParsePos const *pp)
        old_scope = nexpr->scope;
        new_scope = pp->sheet ? pp->sheet->names : pp->wb->names;
 
-       name = nexpr->name->str;
        if (old_scope != new_scope &&
-           (g_hash_table_lookup (new_scope->placeholders, name) ||
-            g_hash_table_lookup (new_scope->names, name))) {
+           (g_hash_table_lookup (new_scope->placeholders, nexpr->name) ||
+            g_hash_table_lookup (new_scope->names, nexpr->name))) {
                const char *fmt = pp->sheet
                        ? _("'%s' is already defined in sheet")
                        : _("'%s' is already defined in workbook");
-               return g_strdup_printf (fmt, name);
+               return g_strdup_printf (fmt, nexpr->name);
        }
 
        if (old_scope)
                g_hash_table_steal
                        (nexpr->is_placeholder ? old_scope->placeholders : old_scope->names,
-                        name);
+                        nexpr->name);
 
        nexpr->pos = *pp;
        gnm_named_expr_collection_insert (new_scope, nexpr);
@@ -1013,12 +1123,10 @@ expr_name_set_is_placeholder (GnmNamedExpr *nexpr, gboolean is_placeholder)
        nexpr->is_placeholder = is_placeholder;
 
        if (nexpr->scope) {
-               const char *name = expr_name_name (nexpr);
-
                g_hash_table_steal (is_placeholder
                                    ? nexpr->scope->names
                                    : nexpr->scope->placeholders,
-                                   name);
+                                   nexpr->name);
                gnm_named_expr_collection_insert (nexpr->scope, nexpr);
        }
 }
@@ -1036,7 +1144,7 @@ struct cb_expr_name_in_use {
 };
 
 static void
-cb_expr_name_in_use (G_GNUC_UNUSED const char *name,
+cb_expr_name_in_use (G_GNUC_UNUSED gconstpointer key,
                     GnmNamedExpr *nexpr,
                     struct cb_expr_name_in_use *pdata)
 {
diff --git a/src/expr-name.h b/src/expr-name.h
index 6e5a0be..7bd2eab 100644
--- a/src/expr-name.h
+++ b/src/expr-name.h
@@ -65,17 +65,6 @@ GOUndo *expr_name_set_expr_undo_new (GnmNamedExpr *nexpr);
 
 /******************************************************************************/
 
-struct _GnmNamedExprCollection {
-       /* all the defined names */
-       GHashTable *names;
-
-       /* placeholders for references to undefined names */
-       GHashTable *placeholders;
-
-       /* <private> */
-       unsigned ref_count;     /* boxed type */
-};
-
 GType gnm_named_expr_collection_get_type (void);
 GnmNamedExprCollection *gnm_named_expr_collection_new (void);
 void gnm_named_expr_collection_free (GnmNamedExprCollection *names);
@@ -88,6 +77,11 @@ GSList  *gnm_named_expr_collection_list (GnmNamedExprCollection const *scope);
 
 GnmNamedExpr *gnm_named_expr_collection_lookup (GnmNamedExprCollection const *scope,
                                                char const *name);
+void gnm_named_expr_collection_dump (GnmNamedExprCollection *names,
+                                    const char *id);
+gboolean gnm_named_expr_collection_sanity_check (GnmNamedExprCollection *names,
+                                                const char *id);
+
 G_END_DECLS
 
 #endif /* _GNM_EXPR_NAME_H_ */
diff --git a/src/ssconvert.c b/src/ssconvert.c
index 0a0435a..75f4006 100644
--- a/src/ssconvert.c
+++ b/src/ssconvert.c
@@ -335,7 +335,9 @@ suggest_size (GSList *wbs, int *csuggest, int *rsuggest)
 }
 
 static void
-cb_fixup_name_wb (const char *name, GnmNamedExpr *nexpr, Workbook *wb)
+cb_fixup_name_wb (G_GNUC_UNUSED gconstpointer key,
+                 GnmNamedExpr *nexpr,
+                 Workbook *wb)
 {
        GnmParsePos newpos = nexpr->pos;
 
diff --git a/src/ssindex.c b/src/ssindex.c
index d255734..b23c778 100644
--- a/src/ssindex.c
+++ b/src/ssindex.c
@@ -20,6 +20,7 @@
 #include "workbook.h"
 #include "sheet.h"
 #include "cell.h"
+#include "expr-name.h"
 #include "value.h"
 #include "mstyle.h"
 #include "sheet-style.h"
@@ -150,9 +151,11 @@ ssindex_chart (IndexerState *state, GogObject *obj)
 }
 
 static void
-cb_index_name (const char *name, GnmExprName const *nexpr, IndexerState *state)
+cb_index_name (G_GNUC_UNUSED gconstpointer key,
+              GnmNamedExpr const *nexpr, IndexerState *state)
 {
-       gsf_xml_out_simple_element (state->output, "data", name);
+       gsf_xml_out_simple_element (state->output,
+                                   "data", expr_name_name (nexpr));
 }
 
 
diff --git a/src/wbc-gtk.c b/src/wbc-gtk.c
index 84c4846..8d8d318 100644
--- a/src/wbc-gtk.c
+++ b/src/wbc-gtk.c
@@ -60,6 +60,7 @@
 #include "tools/analysis-auto-expression.h"
 #include "sheet-object-cell-comment.h"
 #include "print-info.h"
+#include "expr-name.h"
 
 #include <goffice/goffice.h>
 #include <gsf/gsf-impl-utils.h>
@@ -2202,6 +2203,7 @@ cb_statusbox_focus (GtkEntry *entry, GdkEventFocus *event,
 }
 
 /******************************************************************************/
+
 static void
 cb_workbook_debug_info (WBCGtk *wbcg)
 {
@@ -2223,6 +2225,14 @@ cb_workbook_debug_info (WBCGtk *wbcg)
        if (gnm_debug_flag ("style-optimize")) {
                workbook_optimize_style (wb);
        }
+
+       if (gnm_debug_flag ("name-collections")) {
+               gnm_named_expr_collection_dump (wb->names, "workbook");
+               WORKBOOK_FOREACH_SHEET(wb, sheet, {
+                       gnm_named_expr_collection_dump (sheet->names,
+                                                       sheet->name_unquoted);
+               });
+       }
 }
 
 static void
@@ -2821,7 +2831,8 @@ wbc_gtk_create_edit_area (WBCGtk *wbcg)
        debug_button = GET_GUI_ITEM ("debug_button");
        if (gnm_debug_flag ("deps") ||
            gnm_debug_flag ("expr-sharer") ||
-           gnm_debug_flag ("style-optimize")) {
+           gnm_debug_flag ("style-optimize") ||
+           gnm_debug_flag ("name-collections")) {
                g_signal_connect_swapped (debug_button,
                                          "clicked", G_CALLBACK (cb_workbook_debug_info),
                                          wbcg);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]