Re: [xml] Possible 64-bit issues in xml 2.9.2

[Bruce Dawson <brucedawson google com>]

Sure. I'll do that.

On old architectures it would be easy enough to use a typedef to 'create' intptr_t, so any compatibility problems that such a patch introduces will be easily fixed.

On Tue, Nov 10, 2015 at 5:32 PM, Daniel Veillard <veillard redhat com> wrote:

On Tue, Nov 10, 2015 at 01:04:51PM -0800, Bruce Dawson wrote:
> Resending now that I've joined the mailing list...
>
> While building 64-bit Chromium with VC++ 2015 Update 1 I noticed a
> significant number of pointer truncation warnings in libxml, especially in
> xpath.c. A typical warning is:
>
> warning C4311: 'type cast': pointer truncation from 'xmlChar *' to 'long'
>
> which triggers on the last two lines of this block:
>
> case XML_ELEMENT_NODE:
>     if (node2->type == XML_ELEMENT_NODE) {
> if ((0 > (long) node1->content) && /* TODO: Would a != 0 suffice here? */
>     (0 > (long) node2->content) &&
>
> The intent is not entirely clear but if these are supposed to be NULL
> checks then they could easily give the wrong result. In the VC++ world
> 'long' is always a 32-bit type, so converting a pointer to a long both
> throws away the top 32 bits and also makes it a signed type. That means
> that pointers to the first 2 GB of address space will behave as expected,
> and pointers beyond that will have a 50% chance of behaving incorrectly!
>
> Another example is these two conversions. The code is apparently trying to
> sort by content address, but on 64-bit Windows builds it will just be
> sorting by the bottom 32 bits of the content address.
> l1 = -((long) node1->content);
> l2 = -((long) node2->content);
> if (l1 < l2)
>    return(1);
> if (l1 > l2)
>    return(-1);
>     }
>
> This is not a problem with gcc or clang because their 64-bit builds have
> 64-bit long, but the C/C++ standard do not mandate and that is not the case
> with VC++.
>
> I think that the correct type would be ptrdiff_t, or size_t, or intptr_t,
> or uintptr_t. I've attached the full set of warnings in case that is of any
> assistance. Even though some of these warnings do not indicate actual bugs
> it would still be best to use the correct type in order to avoid confusion
> and warnings.
>

  Sounds like intptr_t would be the proper type to use for those, I just
wonder what kind of old arches will break if they don't have it.

Can you come up with a patch which just change the casts and possibly the target
types ?

Daniel

--
Daniel Veillard      | Open Source and Standards, Red Hat
veillard redhat com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

--

Bruce Dawson