[cairo] [PATCH] Enable links to image files in SVG backend

Chris Wilson chris at chris-wilson.co.uk
Wed Jan 20 03:25:04 PST 2010


Comments inline.

On Wed, 20 Jan 2010 12:56:44 +0200, Alexander Shulgin <alex.shulgin at gmail.com> wrote:
> OK, now I've got this:
> 
> ---
>  doc/public/cairo-sections.txt               |    2 +
>  doc/public/tmpl/cairo-surface.sgml          |   14 +++
>  src/Makefile.am                             |    2 +-
>  src/cairo-surface.c                         |   17 +++-
>  src/cairo-svg-surface.c                     |  136 ++++++++++++++++++++++++++-
>  src/cairo.h                                 |    2 +
>  src/generate-uri-allowed-set-lookup-table.c |   27 ++++++
>  src/uri-allowed-set-lookup-table.inc        |   35 +++++++
>  8 files changed, 229 insertions(+), 6 deletions(-)
>  create mode 100644 src/generate-uri-allowed-set-lookup-table.c
>  create mode 100644 src/uri-allowed-set-lookup-table.inc
> 
> diff --git a/doc/public/cairo-sections.txt b/doc/public/cairo-sections.txt
> index ff89db7..1e45c59 100644
> --- a/doc/public/cairo-sections.txt
> +++ b/doc/public/cairo-sections.txt
> @@ -176,6 +176,8 @@ cairo_svg_version_to_string
>  CAIRO_MIME_TYPE_JP2
>  CAIRO_MIME_TYPE_JPEG
>  CAIRO_MIME_TYPE_PNG
> +CAIRO_MIME_TYPE_XURI
> +CAIRO_MIME_TYPE_XURI_ENCODED

I leave others to debate the merits of accepting a non-rfc3986 encoded
URI, and whether it is wise for Cairo just to mandate use of
g_filename_to_uri (or whatever) for local files.

> +/**
> + * _cairo_svg_surface_uri_encode:
> + *
> + * Percent-encode the URI string. Percent sign itself is not in
> + * allowed set so it gets encoded.
> + *
> + * RFC: http://tools.ietf.org/html/rfc3986
> + **/
This is not svg specific, so if we keep the uri encoding, we should create
a cairo-uri.c.

> +static cairo_status_t
> +_cairo_svg_surface_uri_encode (const unsigned char *uri, unsigned int length,
> +			       unsigned char **enc, unsigned int *enc_len)
> +{
> +    unsigned char *p;
> +    int i;
> +
> +    *enc = malloc (length*3 + 1 /* to fit nul-byte from last snprintf */);
We need to do pessimistic integer overflow checks here. So use
_cairo_malloc_ab_plus_c(length, 3, 1); it's a pain but better than a
security exploit.

> +    if (!*enc)
> +	return CAIRO_STATUS_NO_MEMORY;
Please use return _cairo_error (CAIRO_STATUS_NO_MEMORY);

The _cairo_error() gives us a breakpoint on the origin of an error,
absolutely invaluable when tracking down library bugs.

> +
> +    p = *enc;
> +    for (i = 0; i < length; i++)
> +	if (!uri_allowed_set[uri[i]]) {
> +	    snprintf (p, 4 /* "%XX\0" */, "%%%2X", uri[i]);
> +	    p += 3;
> +	} else {
> +	    *(p++) = uri[i];
> +	}
> +
> +    *enc_len = p - *enc;
> +
> +    return CAIRO_STATUS_SUCCESS;
> +}
> +
>  static cairo_status_t
>  _cairo_svg_surface_emit_surface (cairo_svg_document_t *document,
>  				 cairo_surface_t *surface)
> @@ -1175,6 +1277,8 @@ _cairo_svg_surface_emit_surface
> (cairo_svg_document_t *document,
>      cairo_rectangle_int_t extents;
>      cairo_bool_t is_bounded;
>      cairo_status_t status;
> +    const unsigned char *uri;
> +    unsigned int uri_len;
> 
>      if (_cairo_user_data_array_get_data (&surface->user_data,
>  					 (cairo_user_data_key_t *) document))
> @@ -1192,10 +1296,34 @@ _cairo_svg_surface_emit_surface
> (cairo_svg_document_t *document,
> 
>      _cairo_output_stream_printf (document->xml_node_defs, " xlink:href=\"");
> 
> -    status = _cairo_surface_base64_encode (surface,
> -					   document->xml_node_defs);
> -    if (unlikely (status))
> -	return status;
> +    cairo_surface_get_mime_data (surface, CAIRO_MIME_TYPE_XURI_ENCODED,
> +				 &uri, &uri_len);
> +    if (uri)
> +	_cairo_svg_surface_emit_attr_value (document->xml_node_defs,
> +					    uri, uri_len);
Use if (uri != NULL)

> +    else {
No dangling blocks, so } else {

> +	cairo_surface_get_mime_data (surface, CAIRO_MIME_TYPE_XURI,
> +				     &uri, &uri_len);
> +	if (uri) {
> +	    unsigned char *enc;
> +	    unsigned int enc_len;
> +
> +	    status = _cairo_svg_surface_uri_encode (uri, uri_len,
> +						    &enc, &enc_len);
> +	    if (unlikely (status))
> +		return status;
> +
> +	    _cairo_svg_surface_emit_attr_value (document->xml_node_defs,
> +						enc, enc_len);
> +
> +	    free (enc);
> +	} else {
> +	    status = _cairo_surface_base64_encode (surface,
> +						   document->xml_node_defs);
> +	    if (unlikely (status))
> +		return status;
> +	}
> +    }
> 
>      _cairo_output_stream_printf (document->xml_node_defs, "\"/>\n");
> 
> diff --git a/src/cairo.h b/src/cairo.h
> index 8be8043..16cafaf 100644
> --- a/src/cairo.h
> +++ b/src/cairo.h
> @@ -2042,6 +2042,8 @@ cairo_surface_set_user_data (cairo_surface_t		 *surface,
>  #define CAIRO_MIME_TYPE_JPEG "image/jpeg"
>  #define CAIRO_MIME_TYPE_PNG "image/png"
>  #define CAIRO_MIME_TYPE_JP2 "image/jp2"
> +#define CAIRO_MIME_TYPE_XURI "text/x-uri"
> +#define CAIRO_MIME_TYPE_XURI_ENCODED "text/x-uri-encoded"

If we were to accepted both XURI and XURI_ENCODED, the one we want people
to use most often should have the shorter name, ie XURI and
XURI_NON_ENCODED. What's the justification for the non-encoded XURI
again? ;-)

>  cairo_public void
>  cairo_surface_get_mime_data (cairo_surface_t		*surface,
> diff --git a/src/generate-uri-allowed-set-lookup-table.c
> b/src/generate-uri-allowed-set-lookup-table.c
> new file mode 100644
> index 0000000..702a973
> --- /dev/null
> +++ b/src/generate-uri-allowed-set-lookup-table.c
> @@ -0,0 +1,27 @@
> +/* vim: set sw=4 sts=4: -*- Mode: c; tab-width: 8; c-basic-offset: 4;
> indent-tabs-mode: t; -*- */
> +#include <stdio.h>
> +#include <string.h>
> +#include <ctype.h>
> +
> +static const char *uri_allowed_set =
> "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~:/?#[]@!$&'()*+,;=";
> +
> +int
> +main ()
> +{
> +    int i;
> +
> +    printf ("/* %s */\n", uri_allowed_set);
> +    printf ("static const char uri_allowed_set[256] = {\n 0,");
> +    for (i = 1; i < 256; i++) {
> +	printf (strchr (uri_allowed_set, (char) i) ? " 1" : " 0");
> +
> +	if (isgraph (i))
> +	    printf (" /*%c*/", i);
> +
> +	putchar(',');
> +	if (i % 8 == 7)
> +	    putchar('\n');
> +    }
> +    printf ("};\n");
> +    return 0;
> +}
> diff --git a/src/uri-allowed-set-lookup-table.inc
> b/src/uri-allowed-set-lookup-table.inc
I'm resistant to the first .inc file, it's not a convention in wide usage.
In this case, I would just include it directly in cairo-uri.c with the
generator in a comment (which would be even more useful if was directly
executable, for example a python script ;-).

> new file mode 100644
> index 0000000..a6caa9d
> --- /dev/null
> +++ b/src/uri-allowed-set-lookup-table.inc
> @@ -0,0 +1,35 @@
> +/* ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~:/?#[]@!$&'()*+,;=
> */
> +static const char uri_allowed_set[256] = {
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 1 /*!*/, 0 /*"*/, 1 /*#*/, 1 /*$*/, 0 /*%*/, 1 /*&*/, 1 /*'*/,
> + 1 /*(*/, 1 /*)*/, 1 /***/, 1 /*+*/, 1 /*,*/, 1 /*-*/, 1 /*.*/, 1 /*/*/,
> + 1 /*0*/, 1 /*1*/, 1 /*2*/, 1 /*3*/, 1 /*4*/, 1 /*5*/, 1 /*6*/, 1 /*7*/,
> + 1 /*8*/, 1 /*9*/, 1 /*:*/, 1 /*;*/, 0 /*<*/, 1 /*=*/, 0 /*>*/, 1 /*?*/,
> + 1 /*@*/, 1 /*A*/, 1 /*B*/, 1 /*C*/, 1 /*D*/, 1 /*E*/, 1 /*F*/, 1 /*G*/,
> + 1 /*H*/, 1 /*I*/, 1 /*J*/, 1 /*K*/, 1 /*L*/, 1 /*M*/, 1 /*N*/, 1 /*O*/,
> + 1 /*P*/, 1 /*Q*/, 1 /*R*/, 1 /*S*/, 1 /*T*/, 1 /*U*/, 1 /*V*/, 1 /*W*/,
> + 1 /*X*/, 1 /*Y*/, 1 /*Z*/, 1 /*[*/, 0 /*\*/, 1 /*]*/, 0 /*^*/, 1 /*_*/,
> + 0 /*`*/, 1 /*a*/, 1 /*b*/, 1 /*c*/, 1 /*d*/, 1 /*e*/, 1 /*f*/, 1 /*g*/,
> + 1 /*h*/, 1 /*i*/, 1 /*j*/, 1 /*k*/, 1 /*l*/, 1 /*m*/, 1 /*n*/, 1 /*o*/,
> + 1 /*p*/, 1 /*q*/, 1 /*r*/, 1 /*s*/, 1 /*t*/, 1 /*u*/, 1 /*v*/, 1 /*w*/,
> + 1 /*x*/, 1 /*y*/, 1 /*z*/, 0 /*{*/, 0 /*|*/, 0 /*}*/, 1 /*~*/, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> + 0, 0, 0, 0, 0, 0, 0, 0,
> +};
> -- 
> 1.6.3.3
> 
> Is this looking any good? :)

Yes. :-)
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre


More information about the cairo mailing list