[1/4] mempool: use wide enough type for pointer arithmetic

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

Details

Message ID 1455137348-22597-2-git-send-email-Simon.Richter@hogyros.de
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) 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 just 32 bits wide, so converting
from and to a pointer is unsafe.

Replace this with intptr_t, which is guaranteed to be wide enough. It would
be better to use uintptr_t here, but this is not available in several MSVC
versions.
---
 src/cairo-mempool.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/cairo-mempool.c b/src/cairo-mempool.c
index 751ede3..d177bcc 100644
--- a/src/cairo-mempool.c
+++ b/src/cairo-mempool.c
@@ -284,19 +284,19 @@  _cairo_mempool_init (cairo_mempool_t *pool,
 		      void *base, size_t bytes,
 		      int min_bits, int num_sizes)
 {
-    unsigned long tmp;
+    intptr_t tmp;
     int num_blocks;
     int i;
 
     /* Align the start to an integral chunk */
-    tmp = ((unsigned long) base) & ((1 << min_bits) - 1);
+    tmp = ((intptr_t) base) & ((1 << min_bits) - 1);
     if (tmp) {
 	tmp = (1 << min_bits) - tmp;
 	base = (char *)base + tmp;
 	bytes -= tmp;
     }
 
-    assert ((((unsigned long) base) & ((1 << min_bits) - 1)) == 0);
+    assert ((((intptr_t) base) & ((1 << min_bits) - 1)) == 0);
     assert (num_sizes < ARRAY_LENGTH (pool->free));
 
     pool->base = base;

Comments

On Wed, 10 Feb 2016 21:49:05 +0100, Simon Richter wrote:

> The "unsigned long" type on Windows is just 32 bits wide, so
> converting from and to a pointer is unsafe.

“unsigned long long” should work.

> Replace this with intptr_t, which is guaranteed to be wide enough. It
> would be better to use uintptr_t here, but this is not available in
> several MSVC versions.

What about size_t?
Hi,

On 10.02.2016 22:10, Lawrence D'Oliveiro wrote:

>> The "unsigned long" type on Windows is just 32 bits wide, so
>> converting from and to a pointer is unsafe.

> “unsigned long long” should work.

Yes, but that is 64 bits wide even on 32 bit architectures.

>> Replace this with intptr_t, which is guaranteed to be wide enough. It
>> would be better to use uintptr_t here, but this is not available in
>> several MSVC versions.

> What about size_t?

That doesn't come with a guarantee either.

The cleanest solution IMO would be to introduce a few "#ifdef"s, and
just add "typedef UINTPTR_T uintptr_t;" on MSVC (MSYS has a definition
for uintptr_t), but doing that in a way that doesn't break anything else
will be more involved.

   Simon
On Wed, 10 Feb 2016 22:28:59 +0100, Simon Richter wrote:

> On 10.02.2016 22:10, Lawrence D'Oliveiro wrote:
>
>> “unsigned long long” should work.  
> 
> Yes, but that is 64 bits wide even on 32 bit architectures.

Could that cause any problems with the code in question?

>> What about size_t?  
> 
> That doesn't come with a guarantee either.

It’s big enough to represent the size of any object that can occupy
memory. Therefore, it’s big enough to represent the address of any
object that can occupy memory.
2016-02-11 0:10 GMT+01:00 Lawrence D'Oliveiro <ldo@geek-central.gen.nz>:
> On Wed, 10 Feb 2016 22:28:59 +0100, Simon Richter wrote:
>>> What about size_t?
>>
>> That doesn't come with a guarantee either.
>
> It’s big enough to represent the size of any object that can occupy
> memory. Therefore, it’s big enough to represent the address of any
> object that can occupy memory.

This is not correct. See the various answers to this question in SO:
http://stackoverflow.com/a/1464194/450398

Regards,

Guillermo Rodriguez
On Thu, 11 Feb 2016 08:52:38 +0100, Guillermo Rodriguez Garcia wrote:

> 2016-02-11 0:10 GMT+01:00 Lawrence D'Oliveiro
> <ldo@geek-central.gen.nz>:
>
>> On Wed, 10 Feb 2016 22:28:59 +0100, Simon Richter wrote:  
>>>> What about size_t?  
>>>
>>> That doesn't come with a guarantee either.  
>>
>> It’s big enough to represent the size of any object that can occupy
>> memory. Therefore, it’s big enough to represent the address of any
>> object that can occupy memory.  
> 
> This is not correct. See the various answers to this question in SO:
> http://stackoverflow.com/a/1464194/450398

Lol
Am 10.02.2016 um 21:49 schrieb Simon Richter:
> The "unsigned long" type on Windows is just 32 bits wide, so converting
> from and to a pointer is unsafe.
> 
> Replace this with intptr_t, which is guaranteed to be wide enough. It would
> be better to use uintptr_t here, but this is not available in several MSVC
> versions.
> ---
>  src/cairo-mempool.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/cairo-mempool.c b/src/cairo-mempool.c
> index 751ede3..d177bcc 100644
> --- a/src/cairo-mempool.c
> +++ b/src/cairo-mempool.c
> @@ -284,19 +284,19 @@ _cairo_mempool_init (cairo_mempool_t *pool,
>  		      void *base, size_t bytes,
>  		      int min_bits, int num_sizes)
>  {
> -    unsigned long tmp;
> +    intptr_t tmp;
>      int num_blocks;
>      int i;
>  
>      /* Align the start to an integral chunk */
> -    tmp = ((unsigned long) base) & ((1 << min_bits) - 1);
> +    tmp = ((intptr_t) base) & ((1 << min_bits) - 1);
>      if (tmp) {
>  	tmp = (1 << min_bits) - tmp;
>  	base = (char *)base + tmp;
>  	bytes -= tmp;
>      }

Correct me if I'm wrong, but this code doesn't care about the high bits of the
pointer. As long as sizeof(unsigned long) > min_bits (="always"), the old code
already did the right thing (= get the low min_bits bits of the pointer and use
that to align base as needed) and this patch just needs to silence the warning.

Assuming no one proves me wrong on the above, let's not bikeshed this too much:

Reviewed-by: Uli Schlachter <psychon@znc.in>

>  
> -    assert ((((unsigned long) base) & ((1 << min_bits) - 1)) == 0);
> +    assert ((((intptr_t) base) & ((1 << min_bits) - 1)) == 0);
>      assert (num_sizes < ARRAY_LENGTH (pool->free));
>  
>      pool->base = base;
>