Patch for bug #103174

Nigel Titley nigel at titley.com
Thu Sep 4 18:50:01 CDT 2003


Hi,

Nigel Titley <nigel at titley.com> writes:

> > Please can someone check and apply this
> First, if you could supply unified diffs instead of context diffs
> that would be appreciated.  Granted, context diffs are OK, but
> I find unified diffs easier to read (and shorter)...

OK, will do.

> Having said that, I've got a question about the last hunk of
> the patch:

> > ***************> *** 240,250 ****>   void>   gnc_basic_cell_set_value_internal (BasicCell *cell, const char *value)>   {>     if (value == NULL)>       value = "";> ! >     g_free (cell->value);> !   cell->value = g_strdup (value);>   >     g_free (cell->value_w);>     cell->value_len = gnc_mbstowcs (&cell->value_w, cell->value);> --- 244,258 ---->   void>   gnc_basic_cell_set_value_internal (BasicCell *cell, const char *value)>   {> +   char *lcl_value; /* Used to store a temporary version of value */> + >     if (value == NULL)>       value = "";> !   /* Save value, in case it is the same location as cell->value */> !   lcl_value = g_strdup (value);>     g_free (cell->value);> !   /* Finally set cell->value from the local copy */> !   cell->value = lcl_value;>   >     g_free (cell->value_w);>     cell->value_len = gnc_mbstowcs (&cell->value_w, cell->value);
> By my reading of this the only change you're making is that you're
> storing the g_strdup(value) into a local variable before you
> g_free(cell->value) and then set cell->value to the local var.

> I don't understand why this makes a difference.  Functionally the
> setting should be the same:
>
>        g_free(foo);
>        foo = g_strdup(value);
>
> should be the same as:
>        
>        temp = g_strdup(value);
>        g_free(foo);
>        foo = temp;

> Am I missing something in your patch?

Yes you are. The problem is that the SX formula_cell stuff calls (via an
intermediate step) 

gnc_basic_cell_set_value_internal (BasicCell *cell, const char *value)

with *value pointing at cell->value. The overall effect of this as far
as gnc_basic_cell_set_value_internal is concerned is to trash the value
stored in cell->value *before* re-writing back to the same place (the
g_free() writes memory management stuff in the first few bytes of the
value). This is what trashes the debit/credit value in the SX register
entry. Now there are at least three ways to fix this:

1. Modify the formula cell code to save cell->value in a temporary value

2. Add a test in gnc_basic_cell_set_value_internal() to not do the
assignment in the case where the source and destination are the same

3. Store the value in a temporary variable to avoid the overwrite.

I decided on 2 or 3 because it makes gnc_basic_cell_set_value_internal()
safer. Whether you prefer a solution based on strategy 2 or 3 depends on
your mindset. I happen to prefer 3, hence the patch.

Sorry it has taken me so long to reply to this. I wasn't subscribed to
gnucash-patches. I've been sitting here getting slightly upset that I've
had no response..... my fault. I eventually decided to check the archive
and saw your question. 

I've now subscribed.... if someone could either check in the patch or
let me know what you would like me to do. This patch fixes 4 extant bugs
in the SX part of bugzilla.

All the best

Nigel
-- 
Nigel Titley <nigel at titley.com>



More information about the gnucash-patches mailing list