[2/4] scaled fonts: Use wide enough type for pointer arithmetic

Submitted by Simon Richter on Feb. 10, 2016, 8:49 p.m.

Details

Message ID 1455137348-22597-3-git-send-email-Simon.Richter@hogyros.de
State New
Headers show
Series "Series without cover letter" ( rev: 2 ) in Cairo

Not browsing as part of any series.

Commit Message

Simon Richter Feb. 10, 2016, 8:49 p.m.
The "unsigned long" type on Windows is only 32 bit wide, so conversion from
and to pointers is unsafe.

Replace with intptr_t, which is guaranteed to be large enough.
---
 src/cairo-cache-private.h       | 2 +-
 src/cairo-hash.c                | 2 +-
 src/cairo-scaled-font-subsets.c | 4 ++--
 src/cairo-scaled-font.c         | 4 ++--
 src/cairo-types-private.h       | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/cairo-cache-private.h b/src/cairo-cache-private.h
index 24b6d0b..275552c 100644
--- a/src/cairo-cache-private.h
+++ b/src/cairo-cache-private.h
@@ -84,7 +84,7 @@ 
  * not be initialized if so desired.
  **/
 typedef struct _cairo_cache_entry {
-    unsigned long hash;
+    intptr_t hash;
     unsigned long size;
 } cairo_cache_entry_t;
 
diff --git a/src/cairo-hash.c b/src/cairo-hash.c
index 928c74b..8709918 100644
--- a/src/cairo-hash.c
+++ b/src/cairo-hash.c
@@ -340,7 +340,7 @@  _cairo_hash_table_lookup (cairo_hash_table_t *hash_table,
 {
     cairo_hash_entry_t *entry;
     unsigned long table_size, i, idx, step;
-    unsigned long hash = key->hash;
+    intptr_t hash = key->hash;
 
     entry = hash_table->cache[hash & 31];
     if (entry && entry->hash == hash && hash_table->keys_equal (key, entry))
diff --git a/src/cairo-scaled-font-subsets.c b/src/cairo-scaled-font-subsets.c
index 74bfb9e..6080a41 100644
--- a/src/cairo-scaled-font-subsets.c
+++ b/src/cairo-scaled-font-subsets.c
@@ -255,12 +255,12 @@  _cairo_sub_font_init_key (cairo_sub_font_t	*sub_font,
 {
     if (sub_font->is_scaled)
     {
-        sub_font->base.hash = (unsigned long) scaled_font;
+        sub_font->base.hash = (intptr_t) scaled_font;
         sub_font->scaled_font = scaled_font;
     }
     else
     {
-        sub_font->base.hash = (unsigned long) scaled_font->font_face;
+        sub_font->base.hash = (intptr_t) scaled_font->font_face;
         sub_font->scaled_font = scaled_font;
     }
 }
diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index ac80c97..1a3483a 100644
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -639,7 +639,7 @@  _cairo_scaled_font_compute_hash (cairo_scaled_font_t *scaled_font)
     hash = _hash_matrix_fnv (&scaled_font->ctm, hash);
     hash = _hash_mix_bits (hash);
 
-    hash ^= (unsigned long) scaled_font->original_font_face;
+    hash ^= (intptr_t) scaled_font->original_font_face;
     hash ^= cairo_font_options_hash (&scaled_font->options);
 
     /* final mixing of bits */
@@ -2852,7 +2852,7 @@  _cairo_scaled_font_allocate_glyph (cairo_scaled_font_t *scaled_font,
     if (unlikely (page == NULL))
 	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
 
-    page->cache_entry.hash = (unsigned long) scaled_font;
+    page->cache_entry.hash = (intptr_t) scaled_font;
     page->cache_entry.size = 1; /* XXX occupancy weighting? */
     page->num_glyphs = 0;
 
diff --git a/src/cairo-types-private.h b/src/cairo-types-private.h
index 3d15d96..8a540cc 100644
--- a/src/cairo-types-private.h
+++ b/src/cairo-types-private.h
@@ -147,7 +147,7 @@  struct _cairo_observer {
  * the entry need not be initialized if so desired.
  **/
 struct _cairo_hash_entry {
-    unsigned long hash;
+    intptr_t hash;
 };
 
 struct _cairo_array {

Comments

On 11/02/16 07:19, Simon Richter wrote:
>  struct _cairo_hash_entry {
> -    unsigned long hash;
> +    intptr_t hash;
>  };

The hash value is used for looking up the hash table (after performing a
hash % table_size). size_t would be a better type since the hash table
object can't exceed size_t. Scaled fonts uses a pointer as a convenient
means of generating a hash value. But the hash value in general is not
intended to contain a pointer.
Am 10.02.2016 um 21:49 schrieb Simon Richter:
> The "unsigned long" type on Windows is only 32 bit wide, so conversion from
> and to pointers is unsafe.
> 
> Replace with intptr_t, which is guaranteed to be large enough.
> ---
>  src/cairo-cache-private.h       | 2 +-
>  src/cairo-hash.c                | 2 +-
>  src/cairo-scaled-font-subsets.c | 4 ++--
>  src/cairo-scaled-font.c         | 4 ++--
>  src/cairo-types-private.h       | 2 +-
>  5 files changed, 7 insertions(+), 7 deletions(-)
> 

In your "0/4"-mail, you write:

> I'm slightly less sure about the changes to the scaled fonts, but it
> appears that one of the pointers generated from casting an integer back is
> then dereferenced, so it would indeed be good to keep all the bits.
> However, this is where we introduce lots of overhead.

Since this talks about this patch, I'm replying here.

Where is this cast back to a pointer that you talk about? I haven't looked at
the actual code, but from the patch this looks like it just wants a hash and the
code can already handle hash collisions. Plus the high bits are likely the same
for all pointers anyway, so just using the low ones is The Right Thing(tm).

What am I missing?

> diff --git a/src/cairo-cache-private.h b/src/cairo-cache-private.h
> index 24b6d0b..275552c 100644
> --- a/src/cairo-cache-private.h
> +++ b/src/cairo-cache-private.h
> @@ -84,7 +84,7 @@
>   * not be initialized if so desired.
>   **/
>  typedef struct _cairo_cache_entry {
> -    unsigned long hash;
> +    intptr_t hash;
>      unsigned long size;
>  } cairo_cache_entry_t;
>  
> diff --git a/src/cairo-hash.c b/src/cairo-hash.c
> index 928c74b..8709918 100644
> --- a/src/cairo-hash.c
> +++ b/src/cairo-hash.c
> @@ -340,7 +340,7 @@ _cairo_hash_table_lookup (cairo_hash_table_t *hash_table,
>  {
>      cairo_hash_entry_t *entry;
>      unsigned long table_size, i, idx, step;
> -    unsigned long hash = key->hash;
> +    intptr_t hash = key->hash;
>  
>      entry = hash_table->cache[hash & 31];

The high bits look really important here :-)

>      if (entry && entry->hash == hash && hash_table->keys_equal (key, entry))
> diff --git a/src/cairo-scaled-font-subsets.c b/src/cairo-scaled-font-subsets.c
> index 74bfb9e..6080a41 100644
> --- a/src/cairo-scaled-font-subsets.c
> +++ b/src/cairo-scaled-font-subsets.c
> @@ -255,12 +255,12 @@ _cairo_sub_font_init_key (cairo_sub_font_t	*sub_font,
>  {
>      if (sub_font->is_scaled)
>      {
> -        sub_font->base.hash = (unsigned long) scaled_font;
> +        sub_font->base.hash = (intptr_t) scaled_font;
>          sub_font->scaled_font = scaled_font;
>      }
>      else
>      {
> -        sub_font->base.hash = (unsigned long) scaled_font->font_face;
> +        sub_font->base.hash = (intptr_t) scaled_font->font_face;
>          sub_font->scaled_font = scaled_font;
>      }
>  }
> diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
> index ac80c97..1a3483a 100644
> --- a/src/cairo-scaled-font.c
> +++ b/src/cairo-scaled-font.c
> @@ -639,7 +639,7 @@ _cairo_scaled_font_compute_hash (cairo_scaled_font_t *scaled_font)
>      hash = _hash_matrix_fnv (&scaled_font->ctm, hash);
>      hash = _hash_mix_bits (hash);
>  
> -    hash ^= (unsigned long) scaled_font->original_font_face;
> +    hash ^= (intptr_t) scaled_font->original_font_face;
>      hash ^= cairo_font_options_hash (&scaled_font->options);
>  
>      /* final mixing of bits */
> @@ -2852,7 +2852,7 @@ _cairo_scaled_font_allocate_glyph (cairo_scaled_font_t *scaled_font,
>      if (unlikely (page == NULL))
>  	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
>  
> -    page->cache_entry.hash = (unsigned long) scaled_font;
> +    page->cache_entry.hash = (intptr_t) scaled_font;
>      page->cache_entry.size = 1; /* XXX occupancy weighting? */
>      page->num_glyphs = 0;
>  
> diff --git a/src/cairo-types-private.h b/src/cairo-types-private.h
> index 3d15d96..8a540cc 100644
> --- a/src/cairo-types-private.h
> +++ b/src/cairo-types-private.h
> @@ -147,7 +147,7 @@ struct _cairo_observer {
>   * the entry need not be initialized if so desired.
>   **/
>  struct _cairo_hash_entry {
> -    unsigned long hash;
> +    intptr_t hash;
>  };
>  
>  struct _cairo_array {
>
Hi,

On 11.02.2016 21:00, Uli Schlachter wrote:

> Where is this cast back to a pointer that you talk about?

There are two instances in cairo-scaled-font.c, in lines 480 and 2827.
Both of them read

    scaled_font = (cairo_scaled_font_t *) page->cache_entry.hash;

and the pointer is dereferenced afterwards.

I agree with your assessment that this should really be a hash value
only, and using the lower bits might work for that (although the
bottommost bits are probably all zero due to alignment, so it is not a
very good hash), but fixing that requires more changes.

   Simon