Re: [Bluez-devel] hcid patch (patch 00.13)

[Steven Singer <steven.singer@csr.com>]

Marcel Holtmann wrote:
> Johan Hedberg wrote:
>> Yeah, I suspected that could cause problems since
>> sizeof(int) != sizeof(void *) on those machines. I don't think those
>> compiler warnings are dangerous though, since they are probably meant to
>> warn about possible loss of data which shouldn't happen to us since our
>> values always fit within 2 bytes.

Compiler warning are always dangerous.

At the very least, they can obscure the warnings you care about.


>>                                   I don't have a 64bit machine so I
>> can't verify this fix, but I think that sizeof(long) should be the same
>> as sizeof(void *) on both 32 and 64 bit machines. If that isn't the
>> case, the last resort could be to make some automake makro to check the
>> actual size of pointer types and add #defines to the code acordingly.

If you, really, really, really want to stuff integers into pointers then
you should be using the type uintptr_t defined in <inttypes.h>, but even
this isn't safe.

uintptr_t is guaranteed only to be an integer type large enough to hold
all object pointer types (this excludes function pointers). However,
it's not guaranteed that a void * can hold all uintptr_t values or even
that it will hold any consecutive subset. That is, your assertion that
you're safe because all your values fit in 2 bytes is not strictly true
(although it may be true on many architectures).

There's another problem in that even referring to a pointer value that
points to freed memory leads you into undefined behaviour even if you
don't dereference the pointer. This means that code like this:

    free(p);
    if (q != p)
       free(q);

isn't safe. See:

   http://mail-index.netbsd.org/current-users/2005/07/30/0005.html

This makes it safe for a compiler author targetting an architecture
with separate address and data registers to load pointer values into
an address register even if the act of loading an invalid pointer into
a register causes an address exception without the address being
dereferenced.

(If you think about it, this would be quite handy for authors of
debug tools like electric fence - they could generate an exception
as soon as you though about using an invalid pointer rather than
several days later when you finally get round to dereferencing it).

So, if one of the integers you've cast into a void * happens to
match memory that's been freed then you're in trouble.

Basically, this:

  void *p;
  uintptr_t tmp; /* or any suitable integer type such as long */

  /* ... */

  tmp = (uintptr_t) p;

  /* ... */

  p = (void *) tmp;

is safe, but:

  void *tmp;
  uintptr_t *i; /* or any suitable integer type such as long */

  /* ... */

  tmp = (void *) i;

  /* ... */

  i = (uintptr_t) tmp;

Takes you into a world of undefined behaviour.

>> Anyway, the attached patch uses long for the pointer<->integer
>> typecasts. Let me know if it doesn't work.
> 
> I applied your patch and the compiler on my 64-bit machine is much
> happier now, but I think that we need to allocate a structure and pass
> the pointer to it around instead of some casting tricks. Would you mind
> fixing it?

This is the right solution.

Speaking from experience, saying "I only need to store an integer so I
can save myself a malloc and, instead, cast it to a pointer" is a false
economy. Next week you'll suddenly find the implementation gets easier
if you can store two integers, then the week after, an integer and a
char * and so on.

On a resource constrained embedded system where the cost of the extra
allocation may be excessive, the only solution is to redesign the
library from the ground up replacing the void * with a union of a
void * and a long.

	- Steven
-- 


This message has been scanned for viruses by BlackSpider MailControl - www.blackspider.com


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel