Re: [patch] serious problem in pango_layout_set_text()

[Owen Taylor <otaylor redhat com>]

Sven Neumann <sven gimp org> writes:

> Hi,
> 
> Owen Taylor <otaylor redhat com> writes:
> 
> > So, making the API return a value here is wrong. However, I wouldn't
> > object to a patch that didn't free the old value of layout->text
> > until after validating the new string ... if it's easy, we might
> > as well try to prevent a crash.
> 
> can I commit the following change then?
> 
> 
> Salut, Sven
> 


> Index: docs/tmpl/layout.sgml
> ===================================================================
> RCS file: /cvs/gnome/pango/docs/tmpl/layout.sgml,v
> retrieving revision 1.17
> diff -u -r1.17 layout.sgml
> --- docs/tmpl/layout.sgml	2001/06/08 16:02:58	1.17
> +++ docs/tmpl/layout.sgml	2001/08/22 23:21:59
> @@ -97,6 +97,8 @@
>  @layout: 
>  @text: 
>  @length: 
> +<!-- # Unused Parameters # -->
> + Returns: 

rm docs/tmpl/layout.sgml ; cvs up docs/tmpl/layout.sgml

> Index: pango/pango-layout.c
> ===================================================================
> RCS file: /cvs/gnome/pango/pango/pango-layout.c,v
> retrieving revision 1.68
> diff -u -r1.68 pango-layout.c
> --- pango/pango-layout.c	2001/08/09 07:29:41	1.68
> +++ pango/pango-layout.c	2001/08/22 23:21:59
> @@ -723,11 +723,11 @@
>    g_return_if_fail (layout != NULL);
>    g_return_if_fail (length == 0 || text != NULL);
>    
> -  if (layout->text)
> -    g_free (layout->text);
> -
>    if (length == 0)
>      {
> +      if (layout->text)
> +        g_free (layout->text);
> +
>        layout->text = g_strdup ("");
>        layout->n_chars = 0;
>      }
> @@ -755,6 +755,9 @@
>  
>        length = p - text;
>        
> +      if (layout->text)
> +        g_free (layout->text);
> +
>        /* NULL-terminate the text for convenience.
>         */

I think this would be best rewritten to remove the length == 0 special
case, and at the same time, fix the stupid read-one-past-the-end-of-the-buffer
bug I left in there:

      while (*p && (length < 0 || p < text + length))

In fact, it would be best to simply use g_utf8_validate() ...

====
void
pango_layout_set_text (PangoLayout *layout,
		       const char  *text,
		       int          length)
{
  const gchar *end;
  
  g_return_if_fail (layout != NULL);
  g_return_if_fail (length == 0 || text != NULL);

  if (!g_utf8_validate (text, length, &end))
    g_warning ("Invalid UTF8 string passed to pango_layout_set_text()");
  
  length = end - text;

  if (layout->text)
    g_free (layout->text);

  /* NULL-terminate the text for convenience.
   */
  layout->text = g_malloc (length + 1);
  memcpy (layout->text, text, length);
  layout->text[length] = '\0';

  layout->n_chars = g_utf8_strlen (layout->text, -1);
  layout->length = length;

  pango_layout_clear_lines (layout);
}
====

(This is perhaps a tiny bit slower because it needs to walk over the
text twice ... on the other hand, g_utf8_validate() uses macros
internal to gutf8.c and may be faster than what was being done before.
In any case, the speed hit is small compared to other overhead of
pango layout.)

Hmmm, I seem to have rewritten it myself, I guess I might as well
commit.

Regards,
                                        Owen