[xml] Possible 64-bit issues in xml 2.9.2

2015-11-10 Thread Bruce Dawson
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.

-- 
Bruce Dawson
third_party\libxml\src\parser.c(11675): warning C4311: 'type cast': pointer 
truncation from 'void *' to 'long'
third_party\libxml\src\parser.c(1318): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\parser.c(1337): warning C4311: 'type cast': pointer 
truncation from 'void *' to 'long'
third_party\libxml\src\parser.c(1849): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\parser.c(9190): warning C4311: 'type cast': pointer 
truncation from 'void *' to 'long'
third_party\libxml\src\relaxng.c(4401): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\relaxng.c(4409): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\relaxng.c(4413): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\relaxng.c(4420): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\relaxng.c(4424): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\relaxng.c(9384): warning C4311: 'type cast': pointer 
truncation from 'void *' to 'long'
third_party\libxml\src\sax2.c(1911): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\tree.c(4579): warning C4311: 'type cast': pointer 
truncation from 'void *const ' to 'long'
third_party\libxml\src\xmlio.c(3006): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\xmlio.c(3112): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\xmlio.c(829): warning C4311: 'type cast': pointer 
truncation from 'void *' to 'long'
third_party\libxml\src\xmlio.c(850): warning C4311: 'type cast': pointer 
truncation from 'void *' to 'long'
third_party\libxml\src\xmlio.c(868): warning C4311: 'type cast': pointer 
truncation from 'void *' to 'long'
third_party\libxml\src\xmlmemory.c(468): warning C4311: 'type cast': pointer 
truncation from 'void *' to 'unsigned long'
third_party\libxml\src\xpath.c(170): warning C4311: 'type cast': pointer 
truncation from 'xmlChar *' to 'long'
third_party\libxml\src\xpath.c(171): warning C4311: 'type cast': pointer 
truncation from 'xmlChar *' to 'long'
third_party\libxml\src\xpath.c(174): warning C4311: 'type cast': pointer 
truncation from 'xmlChar *' to 'long'
third_party\libxml\src\xpath.c(175): warning C4311: 'type cast': pointer 
truncation from 'xmlChar *' to 'long'
third_party\libxml\src\xpath.c(220): warning C4311: 'type cast': pointer 
truncation from 'xmlChar *' to 'long'
third_party\libxml\src\xpath.c(270): warning C4311: 'type cast': pointer 
truncation from 'xmlChar *' to 'long'
third_party\libxml\src\xpath.c(3236): warning C4312: 'type cast': conversion 
from 'long' to 'void *' of greater size
third_party\libxml\src\xpath.c(3328): warning C4311: 'type cast': pointer 
truncation from 'xmlChar *' to 'long'
third_party\libxml\src\xpath.c(3329): warning C4311: 'type cast': pointer 
truncation from 'xmlChar *' to 'long'

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

2015-11-10 Thread Peter Kasting
On Tue, Nov 10, 2015 at 2:21 PM, Bruce Dawson 
wrote:

> On Tue, Nov 10, 2015 at 1:26 PM, Eric S. Eberhard 
> wrote:
>
>> I am not 100% sure this applies but I'd check into it.  I run AIX mostly
>> and it's compiler defaults to unsigned.  When moving my products to VC++ I
>> had to set some switches so that unspecified (e.g. char x; instead of
>> unsigned char x;) would default to unsigned.  Then my stuff compiles fine.
>> I did make my own projects for VC++ and libxml2 and they do have the
>> unsigned/signed switches set.  It is fairly current libxml2 on VS 2008.
>> Which you are welcome to the projects if you want I can drop a tarball onto
>> my public FTP site.
>>
>> My point is you  may be able to fix with compiler switches.
>>
>
The signedness of "char" is implementation-specific per the C standard,
hence the ability to change with switches.  The bugs in question here are
not related to the char type, so these sorts of switches don't apply.

As Bruce notes, it is not possible to address the actual issues here with
switches.

Another idea -- I believe that I also compiled them as 32 bit apps.
>> libxml2 has no need to be a 64 bit app.  It need to run on a 64 bit
>> computer which is no problem.  But libxml2 will never address that much
>> space and the O/S is decent in Windows at handling that (and great in AIX
>> and pretty good in Linux and OS/X and HP/UX).
>>
>
Unfortunately, because Chromium is 64-bit, we may not be passing a >2GB
buffer to libxml, but we definitely could be passing in virtual addresses
whose upper 32 bits are not zero- or sign-extensions of the lower 32 bits.
In that case it doesn't matter what amount of memory libxml needs to
address; the input addresses are sufficient to trigger problems even with
small buffer sizes.

PK
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml


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

2015-11-10 Thread Bruce Dawson
Thanks for the reply.

I work on Chromium and there many good reasons why it should be a 64-bit
application - security being one of them. Since libxml is being compiled
into Chromium we need it to support 64-bit.

> I run AIX mostly and it's compiler defaults to unsigned.

I'm not sure that's relevant. The only problem I'm hitting is with the
assumption that sizeof(long) == sizeof(void*), which is not a portable
assumption.

I can't fix with compiler switches because there is no option to change the
size of 'long'. I can suppress the warnings, but since some of the warnings
appear to be real issues, that would just be ignoring the problem, not
solving it. I don't know how serious the issues are since it's not clear to
me what the pointer comparisons are for.



On Tue, Nov 10, 2015 at 1:26 PM, Eric S. Eberhard  wrote:

> I am not 100% sure this applies but I'd check into it.  I run AIX mostly
> and it's compiler defaults to unsigned.  When moving my products to VC++ I
> had to set some switches so that unspecified (e.g. char x; instead of
> unsigned char x;) would default to unsigned.  Then my stuff compiles fine.
> I did make my own projects for VC++ and libxml2 and they do have the
> unsigned/signed switches set.  It is fairly current libxml2 on VS 2008.
> Which you are welcome to the projects if you want I can drop a tarball onto
> my public FTP site.
>
> My point is you  may be able to fix with compiler switches.
>
> Another idea -- I believe that I also compiled them as 32 bit apps.
> libxml2 has no need to be a 64 bit app.  It need to run on a 64 bit
> computer which is no problem.  But libxml2 will never address that much
> space and the O/S is decent in Windows at handling that (and great in AIX
> and pretty good in Linux and OS/X and HP/UX).  Windows simply puts 32 bit
> apps at the bottom of memory and 64 bit at the top ... which is fine if you
> don't run out of space.  I heard that later Windows does it more like
> Unix.  AIX actually intercepts addresses on 32 bit apps and makes a
> translation -- so I may or may not be using 64 bit addresses and it does
> not matter, it is always translated to a 32 bit.  Again, I don't need to
> access a pointer more than 32 bits (excessive in all but the largest of
> systems).  And horsepower on these boxes is so high that the translation is
> not hurting.  I have sites that literally exchange 1.5 million - 2.0
> million XML files per day (most in 10 hours) on a weenie little IBM using
> libxml2 to parse and create XML.
>
> My point on this is you may be able to fix by making it a 32 bit
> application.
>
> E
>
> On 11/10/2015 2:04 PM, 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.
>
> --
> Bruce Dawson
>
>
> ___
> xml mailing list, project page  
> http://xmlsoft.org/xml@gnome.orghttps://mail.gnome.org/mailman/listinfo/xml
>
>
> --
> Eric S. Eberhard
> VICS
> 2933 W Middle Verde Road
> Camp Verde, AZ  86322
> 928-567-3727  work  928-301-7537  cell
> http://www.vicsmba.com/index.html (our 
> work)http://www.vicsmba.com/ourpics/index.html (fun pictures)
>
>


-- 
Bruce Dawson

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

2015-11-10 Thread Bruce Dawson
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 
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
> veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
> http://veillard.com/ | virtualization library  http://libvirt.org/
>



-- 
Bruce Dawson
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml


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

2015-11-10 Thread Daniel Veillard
On Tue, Nov 10, 2015 at 05:34:45PM -0800, Bruce Dawson wrote:
> Sure. I'll do that.

  okay, thanks

> 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.

  the problem is to know when that type is not defined, will require some
configure.ac magic I'm afraid

Daniel

> On Tue, Nov 10, 2015 at 5:32 PM, Daniel Veillard 
> 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
> > veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
> > http://veillard.com/ | virtualization library  http://libvirt.org/
> >
> 
> 
> 
> -- 
> Bruce Dawson

-- 
Daniel Veillard  | Open Source and Standards, Red Hat
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml


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

2015-11-10 Thread Peter Kasting
On Tue, Nov 10, 2015 at 3:38 PM, Eric S. Eberhard  wrote:

> But I also found a /clr compiler switch which DOES allow a 64 bit
> application to call a 32 bit dll application ... with the implied
> assumption that the addressing will be taken care of:
>
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa384231(v=vs.85).aspx
> https://msdn.microsoft.com/en-us/library/ms973190.aspx
>
> I am pretty I did not "solve" your problem but I may have given you some
> leads ... both of those sites are over my Windows knowledge threshold.  But
> if you solve it I would be curious to know the answer :-)
>

It's not feasible to run libxml as a separate service, wrapped using COM to
marshal calls to it.

This is not the sort of problem that is solvable with simple compiler
switches or the like.  The libxml code is fundamentally buggy.  It makes
assumptions that sizeof(long) >= sizeof(intptr_t), which is not guaranteed
by the standard and is in fact violated in practice on 64-bit Windows
builds.  The way to fix these is to change the code.

PK
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml