Patch: FYI: disable XML service files

Patch: FYI: disable XML service files

[Tom Tromey]

I'm checking this in on the trunk and the RH 4.1 branch.

This removes the XML service files from libgcj.so.  This lets us
override things properly again.

Andrew, I tried your test case from ifoox, but I can only see the
failure if I back out some of the latest XML bug fixes.  However a
simpler test case shows which thing we're actually loading:

    import javax.xml.parsers.*;
    public class q {
      public static void main(String[] args) throws Throwable{
        DocumentBuilderFactory tf = DocumentBuilderFactory.newInstance();
        System.out.println(tf.getClass());
      }
    }

Eg, here's the bug in action:

opsy. gij -Djava.class.path=/usr/share/java/xerces-j2.jar:. q
class gnu.xml.dom.DomDocumentBuilderFactory

Tom

Index: ChangeLog
from  Tom Tromey  <tromey@redhat.com>

	* sources.am, Makefile.in: Rebuilt.
	* scripts/makemake.tcl (scan_directory): Allow service files to be
	omitted.
	Omit all XML-related service files.

Index: scripts/makemake.tcl
===================================================================
--- scripts/makemake.tcl	(revision 122761)
+++ scripts/makemake.tcl	(working copy)
@@ -123,6 +123,14 @@
 # We haven't merged locale resources yet.
 set properties_map(gnu/java/locale) _
 
+# We want to be able to load xerces if it is on the class path.  So,
+# we have to avoid compiling in the XML-related service files.
+set properties_map(META-INF/services/javax.xml.parsers.DocumentBuilderFactory) _
+set properties_map(META-INF/services/javax.xml.parsers.SAXParserFactory) _
+set properties_map(META-INF/services/javax.xml.parsers.TransformerFactory) _
+set properties_map(META-INF/services/org.relaxng.datatype.DatatypeLibraryFactory) _
+set properties_map(META-INF/services/org.w3c.dom.DOMImplementationSourceList) _
+set properties_map(META-INF/services/org.xml.sax.driver) _
 
 # List of all properties files.
 set properties_files {}
@@ -223,8 +231,10 @@
     } elseif {[file isdirectory $file]} {
       lappend subdirs $subdir/$file
     } elseif {$subdir == "META-INF/services"} {
-      # All service files are included as properties.
-      lappend properties_files $basedir/$subdir/$file
+      # Service files are generally included as properties.
+      if {! [info exists properties_map($subdir/$file)]} {
+	lappend properties_files $basedir/$subdir/$file
+      }
     }
   }
   cd $here

Re: Patch: FYI: disable XML service files

[Andrew Haley]

Tom Tromey writes:
 > I'm checking this in on the trunk and the RH 4.1 branch.
 > 
 > This removes the XML service files from libgcj.so.  This lets us
 > override things properly again.
 > 
 > Andrew, I tried your test case from ifoox, but I can only see the
 > failure if I back out some of the latest XML bug fixes.

Well, yeah.  The important think to make sure is that we really can
override the built-in classes.

 > However a
 > simpler test case shows which thing we're actually loading:
 > 
 >     import javax.xml.parsers.*;
 >     public class q {
 >       public static void main(String[] args) throws Throwable{
 >         DocumentBuilderFactory tf = DocumentBuilderFactory.newInstance();
 >         System.out.println(tf.getClass());
 >       }
 >     }
 > 
 > Eg, here's the bug in action:
 > 
 > opsy. gij -Djava.class.path=/usr/share/java/xerces-j2.jar:. q
 > class gnu.xml.dom.DomDocumentBuilderFactory

Looks good.  Is there some easy way to make sure the fallbacks still
work with these servie files gone?

Andrew.

Re: Patch: FYI: disable XML service files

[Tom Tromey]

>>>>> "Andrew" == Andrew Haley <aph@redhat.com> writes:

>> opsy. gij -Djava.class.path=/usr/share/java/xerces-j2.jar:. q
>> class gnu.xml.dom.DomDocumentBuilderFactory

Andrew> Looks good.  Is there some easy way to make sure the fallbacks still
Andrew> work with these servie files gone?

Yeah, just omit the classpath setting on the command line above.
Also I hand-checked all the places using these service files to make
sure that there actually is a hard-coded fallback.

Tom

Re: Patch: FYI: disable XML service files

[Gary Benson]

Tom Tromey wrote:
> I'm checking this in on the trunk and the RH 4.1 branch.
> 
> This removes the XML service files from libgcj.so.  This lets us
> override things properly again.
> 
> Andrew, I tried your test case from ifoox, but I can only see the
> failure if I back out some of the latest XML bug fixes.  However a
> simpler test case shows which thing we're actually loading:
> 
>     import javax.xml.parsers.*;
>     public class q {
>       public static void main(String[] args) throws Throwable{
>         DocumentBuilderFactory tf = DocumentBuilderFactory.newInstance();
>         System.out.println(tf.getClass());
>       }
>     }
> 
> Eg, here's the bug in action:
> 
> opsy. gij -Djava.class.path=/usr/share/java/xerces-j2.jar:. q
> class gnu.xml.dom.DomDocumentBuilderFactory

This is actually the correct behaviour: xerces should only be loaded
if it is endorsed, and merely being in the classpath should have no
effect.  Check this test case with Sun or IBM java:

  import javax.xml.parsers.DocumentBuilderFactory;
  public class Test {
    public static void main(String[] args) throws Throwable{
      DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
      f.setAttribute("bad_attribute", "hello");
    }
  }
	    
You can't just print the DocumentBuilderFactory here because Sun and
IBM both use xerces internally, but causing an exception lets you see
the line numbers and figure out where it's loading from:

  mambo:[~]$ ibmjava Test
  Exception in thread "main" java.lang.IllegalArgumentException: bad_attribute
          at org.apache.xerces.jaxp.DocumentBuilderFactoryImpl.setAttribute(Unknown Source)
          at Test.main(Test.java:7)

  mambo:[~]$ ibmjava -cp /usr/share/java/xerces-j2.jar:. Test
  Exception in thread "main" java.lang.IllegalArgumentException: bad_attribute
          at org.apache.xerces.jaxp.DocumentBuilderFactoryImpl.setAttribute(Unknown Source)
          at Test.main(Test.java:7)

  mambo:[~]$ ibmjava -Djava.endorsed.dirs=/var/lib/tomcat5/common/endorsed Test
  Exception in thread "main" java.lang.IllegalArgumentException: Property 'bad_attribute' is not recognized.
          at org.apache.xerces.jaxp.DocumentBuilderFactoryImpl.setAttribute(DocumentBuilderFactoryImpl.java:112)
          at Test.main(Test.java:7)

Just being in the classpath has the same behaviour as not being there
at all.  gcj had the correct behaviour before the service files were
removed.

Cheers,
Gary

Re: Patch: FYI: disable XML service files

[Andrew Haley]

Gary Benson writes:
 > Tom Tromey wrote:
 > > I'm checking this in on the trunk and the RH 4.1 branch.
 > > 
 > > This removes the XML service files from libgcj.so.  This lets us
 > > override things properly again.
 > > 
 > > Andrew, I tried your test case from ifoox, but I can only see the
 > > failure if I back out some of the latest XML bug fixes.  However a
 > > simpler test case shows which thing we're actually loading:
 > > 
 > >     import javax.xml.parsers.*;
 > >     public class q {
 > >       public static void main(String[] args) throws Throwable{
 > >         DocumentBuilderFactory tf = DocumentBuilderFactory.newInstance();
 > >         System.out.println(tf.getClass());
 > >       }
 > >     }
 > > 
 > > Eg, here's the bug in action:
 > > 
 > > opsy. gij -Djava.class.path=/usr/share/java/xerces-j2.jar:. q
 > > class gnu.xml.dom.DomDocumentBuilderFactory
 > 
 > This is actually the correct behaviour: xerces should only be loaded
 > if it is endorsed, and merely being in the classpath should have no
 > effect.  Check this test case with Sun or IBM java:
 > 
 >   import javax.xml.parsers.DocumentBuilderFactory;
 >   public class Test {
 >     public static void main(String[] args) throws Throwable{
 >       DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
 >       f.setAttribute("bad_attribute", "hello");
 >     }
 >   }

IMO that's the wrong test.  What matters is this not the
DocumentBuilderFactory returned, but the DocumentBuilder, and here
what we do is clearly wrong:

import javax.xml.parsers.DocumentBuilderFactory;
public class Test1 {
  public static void main(String[] args) throws Throwable{
    DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
    System.out.println(f.newDocumentBuilder());
    }
}

 $ ~/jdk1.5.0_09/bin/java Test1
com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderImpl@18a47e0

 $ ~/jdk1.5.0_09/bin/java -classpath /usr/share/java/xerces-j2.jar:. Test1
org.apache.xerces.jaxp.DocumentBuilderImpl@7d8483


The old gcj did The Right Thing:

 $ gij Test1
gnu.xml.dom.DomDocumentBuilder@3181dc15

 $ gij -classpath /usr/share/java/xerces-j2.jar:. Test1
org.apache.xerces.jaxp.DocumentBuilderImpl@2cfe98d5


Without tromey's patch the new gcj does the Wrong Thing:

 $ ~/gcc/trunk/install/bin/gij Test1
gnu.xml.dom.DomDocumentBuilder@318eaa95
 $ ~/gcc/trunk/install/bin/gij -classpath /usr/share/java/xerces-j2.jar:. Test1
gnu.xml.dom.DomDocumentBuilder@31992c35


And with his patch, the Right Thing:

 $ ~/gcc/trunk/install/bin/gij Test1
gnu.xml.dom.DomDocumentBuilder@318ebff5

 $ ~/gcc/trunk/install/bin/gij -classpath /usr/share/java/xerces-j2.jar:. Test1
org.apache.xerces.jaxp.DocumentBuilderImpl@31918a55


We don't use Xerces so we can't be compatible with all behaviour in
every possible case, but this is far more like to affect
user-observable behaviour.

What we had before did the right thing for the wrong reason.
	    
 > You can't just print the DocumentBuilderFactory here because Sun and
 > IBM both use xerces internally, but causing an exception lets you see
 > the line numbers and figure out where it's loading from:
 > 
 >   mambo:[~]$ ibmjava Test
 >   Exception in thread "main" java.lang.IllegalArgumentException: bad_attribute
 >           at org.apache.xerces.jaxp.DocumentBuilderFactoryImpl.setAttribute(Unknown Source)
 >           at Test.main(Test.java:7)
 > 
 >   mambo:[~]$ ibmjava -cp /usr/share/java/xerces-j2.jar:. Test
 >   Exception in thread "main" java.lang.IllegalArgumentException: bad_attribute
 >           at org.apache.xerces.jaxp.DocumentBuilderFactoryImpl.setAttribute(Unknown Source)
 >           at Test.main(Test.java:7)
 > 
 >   mambo:[~]$ ibmjava -Djava.endorsed.dirs=/var/lib/tomcat5/common/endorsed Test
 >   Exception in thread "main" java.lang.IllegalArgumentException: Property 'bad_attribute' is not recognized.
 >           at org.apache.xerces.jaxp.DocumentBuilderFactoryImpl.setAttribute(DocumentBuilderFactoryImpl.java:112)
 >           at Test.main(Test.java:7)
 > 
 > Just being in the classpath has the same behaviour as not being there
 > at all.  gcj had the correct behaviour before the service files were
 > removed.

I don't think this is true.  

The logic in GNU Classpath's DocumentBuilderFactory.newInstance() that
searches for an available parser can't possibly return anything other
than the built-in one if the XML property files are included, because
the class loader logic will always find the built-in one first.

With IBM, Sun, etc, all that a user has to do to use their own version
of Xerces is put it into the classpath, and that's what people do.  It
isn't necessary to put the it in endorsed, although that would be good
practice.

Andrew.

Re: Patch: FYI: disable XML service files

[Gary Benson]

Andrew Haley wrote:
> Gary Benson writes:
> > Tom Tromey wrote:
> > > I'm checking this in on the trunk and the RH 4.1 branch.
> > > 
> > > This removes the XML service files from libgcj.so.  This lets us
> > > override things properly again.
> > > 
> > > Andrew, I tried your test case from ifoox, but I can only see the
> > > failure if I back out some of the latest XML bug fixes.  However a
> > > simpler test case shows which thing we're actually loading:
> > > 
> > >     import javax.xml.parsers.*;
> > >     public class q {
> > >       public static void main(String[] args) throws Throwable{
> > >         DocumentBuilderFactory tf = DocumentBuilderFactory.newInstance();
> > >         System.out.println(tf.getClass());
> > >       }
> > >     }
> > > 
> > > Eg, here's the bug in action:
> > > 
> > > opsy. gij -Djava.class.path=/usr/share/java/xerces-j2.jar:. q
> > > class gnu.xml.dom.DomDocumentBuilderFactory
> > 
> > This is actually the correct behaviour: xerces should only be loaded
> > if it is endorsed, and merely being in the classpath should have no
> > effect.  Check this test case with Sun or IBM java:
> > 
> >   import javax.xml.parsers.DocumentBuilderFactory;
> >   public class Test {
> >     public static void main(String[] args) throws Throwable{
> >       DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
> >       f.setAttribute("bad_attribute", "hello");
> >     }
> >   }
> 
> IMO that's the wrong test.  What matters is this not the
> DocumentBuilderFactory returned, but the DocumentBuilder, and here
> what we do is clearly wrong:
> 
> import javax.xml.parsers.DocumentBuilderFactory;
> public class Test1 {
>   public static void main(String[] args) throws Throwable{
>     DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
>     System.out.println(f.newDocumentBuilder());
>     }
> }
> 
>  $ ~/jdk1.5.0_09/bin/java Test1
> com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderImpl@18a47e0
> 
>  $ ~/jdk1.5.0_09/bin/java -classpath /usr/share/java/xerces-j2.jar:. Test1
> org.apache.xerces.jaxp.DocumentBuilderImpl@7d8483

Ok, this is where I went wrong.  IBM's JRE does not do this.
We should definitly follow Sun here.

> We don't use Xerces so we can't be compatible with all behaviour
> in every possible case, but this is far more like to affect
> user-observable behaviour.

We should be okay so long as we have either all service files or no
service files.  The bug we had before was because we had all of the
service fiiles except one.  However, making xerces work in the
classpath like this will mean a whole load of things that have both
xml-commons-apis and xerces in the classpath but not endorsed (eg
most everything in Fedora) will now be running xerces with libgcj's
javax.xml, and ISTR libgcj has a newer javax.xml with more interface
methods than most versions of xerces expect.

Cheers,
Gary

Re: Patch: FYI: disable XML service files

[Andrew Haley]

Gary Benson writes:
 > Andrew Haley wrote:

 > > import javax.xml.parsers.DocumentBuilderFactory;
 > > public class Test1 {
 > >   public static void main(String[] args) throws Throwable{
 > >     DocumentBuilderFactory f = DocumentBuilderFactory.newInstance();
 > >     System.out.println(f.newDocumentBuilder());
 > >     }
 > > }
 > > 
 > >  $ ~/jdk1.5.0_09/bin/java Test1
 > > com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderImpl@18a47e0
 > > 
 > >  $ ~/jdk1.5.0_09/bin/java -classpath /usr/share/java/xerces-j2.jar:. Test1
 > > org.apache.xerces.jaxp.DocumentBuilderImpl@7d8483
 > 
 > Ok, this is where I went wrong.  IBM's JRE does not do this.

Interesting.

 > We should definitly follow Sun here.

Right, we're on the same page.

 > > We don't use Xerces so we can't be compatible with all behaviour
 > > in every possible case, but this is far more like to affect
 > > user-observable behaviour.
 > 
 > We should be okay so long as we have either all service files or no
 > service files.  The bug we had before was because we had all of the
 > service files except one.

Out of interest, which one were we missing?

 > However, making xerces work in the classpath like this will mean a
 > whole load of things that have both xml-commons-apis and xerces in
 > the classpath but not endorsed (eg most everything in Fedora) will
 > now be running xerces with libgcj's javax.xml, 

Which, unless I am very much mistaken, is what has always happened.

 > and ISTR libgcj has a newer javax.xml with more interface methods
 > than most versions of xerces expect.

Sure, but presumably this is also true for everyone else's version 5
libraries, isn't it?

Andrew.

Re: Patch: FYI: disable XML service files

[Gary Benson]

Andrew Haley wrote:
> Gary Benson writes:
> > Andrew Haley wrote:
> > > We don't use Xerces so we can't be compatible with all behaviour
> > > in every possible case, but this is far more like to affect
> > > user-observable behaviour.
> > 
> > We should be okay so long as we have either all service files or
> > no service files.  The bug we had before was because we had all of
> > the service files except one.
> 
> Out of interest, which one were we missing?

The one I added, org.w3c.dom.DOMImplementationSourceList.

> > However, making xerces work in the classpath like this will mean a
> > whole load of things that have both xml-commons-apis and xerces in
> > the classpath but not endorsed (eg most everything in Fedora) will
> > now be running xerces with libgcj's javax.xml,
> 
> Which, unless I am very much mistaken, is what has always happened.

No.  Prior to the service files being removed the only way to get
xerces to be used was to endorse it.  Until now everything has been
using libgcj for both the interface and the implementation.

> > and ISTR libgcj has a newer javax.xml with more interface methods
> > than most versions of xerces expect.
> 
> Sure, but presumably this is also true for everyone else's version
> 5 libraries, isn't it?

Presumably, but we always seemed to have build failures here.  I've
been trying to build some stuff with the RH4.1 branch (which should
flush them out if they exist) but I'm struggling to get a working
setup.

Cheers,
Gary

Re: Patch: FYI: disable XML service files

[Andrew Haley]

Gary Benson writes:
 > Andrew Haley wrote:
 > > Gary Benson writes:
 > > > Andrew Haley wrote:
 > > > > We don't use Xerces so we can't be compatible with all behaviour
 > > > > in every possible case, but this is far more like to affect
 > > > > user-observable behaviour.
 > > > 
 > > > We should be okay so long as we have either all service files or
 > > > no service files.  The bug we had before was because we had all of
 > > > the service files except one.
 > > 
 > > Out of interest, which one were we missing?
 > 
 > The one I added, org.w3c.dom.DOMImplementationSourceList.
 > 
 > > > However, making xerces work in the classpath like this will mean a
 > > > whole load of things that have both xml-commons-apis and xerces in
 > > > the classpath but not endorsed (eg most everything in Fedora) will
 > > > now be running xerces with libgcj's javax.xml,
 > > 
 > > Which, unless I am very much mistaken, is what has always happened.
 > 
 > No.  Prior to the service files being removed the only way to get
 > xerces to be used was to endorse it.  Until now everything has been
 > using libgcj for both the interface and the implementation.

No, that is certainly not true.  See my prior postings in this thread,
and in particular the test using gcj Version 4.2.  Until this latest
classpath import, we haven't been using the service files in libgcj at
all, so if Xerces was in the classpath we used it.

Andrew.

Re: Patch: FYI: disable XML service files

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> This is actually the correct behaviour: xerces should only be loaded
Gary> if it is endorsed, and merely being in the classpath should have no
Gary> effect.

I do think this behavior is fishy (despite later stuff in this
thread), but I think removing the service files is the best thing to
do anyway.  Using the service files to mask some other bug is a bad
approach IMO; if we're using the wrong class loader (which doesn't
seem to be the case anyhow) we should fix that directly.

Tom

Re: Patch: FYI: disable XML service files

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> This is actually the correct behaviour: xerces should only be
> Gary> loaded if it is endorsed, and merely being in the classpath
> Gary> should have no effect.
> 
> I do think this behavior is fishy

Not just fishy but actually wrong :)

> I think removing the service files is the best thing to do anyway.
> Using the service files to mask some other bug is a bad approach IMO

Having the service files is the right thing to do if you want JAXP
parsers to be loaded only if they're endorsed.  Having no service
files is the right thing if you want them to be loaded regardless.

I think it all comes down to understanding what the endorsed standard
override thing is supposed to be overriding, which is the classes in
javax.xml, the interface classes.  The implementation is not the
standard, so implementations don't need endorsing to make them work.
I misunderstood that.  It didn't help that whoever wrote that bit of
the IBM class library also misunderstood it ;)

Cheers,
Gary

Re: Patch: FYI: disable XML service files

[Gary Benson]

Andrew Haley wrote:
> Gary Benson writes:
> > Andrew Haley wrote:
> > > Gary Benson writes:
> > > > However, making xerces work in the classpath like this will
> > > > mean a whole load of things that have both xml-commons-apis
> > > > and xerces in the classpath but not endorsed (eg most
> > > > everything in Fedora) will now be running xerces with
> > > > libgcj's javax.xml,
> > > 
> > > Which, unless I am very much mistaken, is what has always
> > > happened.
> > 
> > No.  Prior to the service files being removed the only way to get
> > xerces to be used was to endorse it.  Until now everything has
> > been using libgcj for both the interface and the implementation.
> 
> No, that is certainly not true.

It is true in the sense that immediately prior to the service files
being removed the only way to get xerces to be used was to endorse it.
Which is the sense I meant it.

> See my prior postings in this thread, and in particular the test
> using gcj Version 4.2.  Until this latest classpath import, we
> haven't been using the service files in libgcj at all, so if Xerces
> was in the classpath we used it.

Ok, this makes me a lot happier about this patch going into Fedora.

Cheers,
Gary

Re: Patch: FYI: disable XML service files

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

>> I think removing the service files is the best thing to do anyway.
>> Using the service files to mask some other bug is a bad approach IMO

Gary> Having the service files is the right thing to do if you want JAXP
Gary> parsers to be loaded only if they're endorsed.  Having no service
Gary> files is the right thing if you want them to be loaded regardless.

It seems to me that if we want to ensure that this code in our XML
implementation only loads things when they are endorsed, then it ought
to be using the bootstrap class loader and not the system class
loader, as it currently does.  Using the service files to work around
this seems hackish.

I'm probably really confused though.  I certainly feel confused.

Tom