Re: [Fwd: [Mono-devel-list] Re: System.Xml patch]
I'll apply the patch attached after your commit. Actually, you don't have to add this nunit test (I am talking about EntityDeclarationNotWF ). It already exists in W3C, it's id is valid-sa-086. It's different from valid-sa-086. It tests *invalid* document (that one I pointed out why your previous patch was not fine). What does this mean? I guess you have in mind that there are cases that entities could be used inside attributes, but attribute content check is less prohibiting and '<' characters are checked anyways. Not only in attributes. One of the tests went through the following flow: DTD had entity declaration pointing to external xml, which had in turn it's own DTD. It is in valid-sa-100 test. So what happened is because current implementation makes early evaluation, it read the external source and failed because the context was Element, and Element can't have DTDs. So here we have a coincident of 2 things: early evaluation and Element context. So I understood that it's not always might be in Element context. Oh, I see. There's another reason to fix entity resolution stuff should be improved. Mhm, actually the testcase I've attached is a strong counterpart of temptation to change implementation to make it to take lazy evaluation way :( (BTW we won't understand what "-AS" means there ;-) svn blame will help curious hackers ;-) but if it's not acceptable, I'll remove it. That's exactly the reason why you *don't* need "-AS" there ;-) (and most of us won't know what AS means. At least "- Andrew" is much better (but we usually don't record our names. Yeah, it depends on the projects and organizations though). Besides the comment itself. In fact I also don't like that part of code. Actually, though replacing entities with a simple string is good for performance as compared to such code that loads entity possibly from external files every time, it causes incorrect BaseURI resolution (that results in incorrect not-wf error for XHTML 1.1 DTD; bug #51495). So, basically rewriting that entity expansion part is the best solution. But I haven't tried that since it will not be done as a quick hack and it will first result in several breakage in standalone tests. That's why I made this patch very carefully, to make minimal impact. I don't have plans to improve this code right now. That's part of why I don't love that enormous tests. Maybe no one including you will never fix those "big" problem, or make significant contributions. I'd even like to accept some regressions in front of such big improvements if any. XML standalone tests are so canonical pretty enough. Atsushi Eno ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Fwd: [Mono-devel-list] Re: System.Xml patch]
Atsushi Eno wrote: Please go ahead as I wrote before ;-) Ugh, I said you I miss sometimes messages from devlist. Sorry for inconvenience. This patch looks fine. Let's check it in svn. I'll apply the patch attached after your commit. Actually, you don't have to add this nunit test (I am talking about EntityDeclarationNotWF ). It already exists in W3C, it's id is valid-sa-086. What does this mean? I guess you have in mind that there are cases that entities could be used inside attributes, but attribute content check is less prohibiting and '<' characters are checked anyways. Not only in attributes. One of the tests went through the following flow: DTD had entity declaration pointing to external xml, which had in turn it's own DTD. It is in valid-sa-100 test. So what happened is because current implementation makes early evaluation, it read the external source and failed because the context was Element, and Element can't have DTDs. So here we have a coincident of 2 things: early evaluation and Element context. So I understood that it's not always might be in Element context. (BTW we won't understand what "-AS" means there ;-) svn blame will help curious hackers ;-) but if it's not acceptable, I'll remove it. Besides the comment itself. In fact I also don't like that part of code. Actually, though replacing entities with a simple string is good for performance as compared to such code that loads entity possibly from external files every time, it causes incorrect BaseURI resolution (that results in incorrect not-wf error for XHTML 1.1 DTD; bug #51495). So, basically rewriting that entity expansion part is the best solution. But I haven't tried that since it will not be done as a quick hack and it will first result in several breakage in standalone tests. That's why I made this patch very carefully, to make minimal impact. I don't have plans to improve this code right now. Cheers, Andrew. ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Fwd: [Mono-devel-list] Re: System.Xml patch]
Hi Andrew, Please go ahead as I wrote before ;-) http://www.mail-archive.com/mono-devel-list@lists.ximian.com/msg02075.html Andrew Skiba wrote: Hello Eno. Can you please look and give your opinion on this patch? Andrew. ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Fwd: [Mono-devel-list] Re: System.Xml patch]
Hello Eno.
Can you please look and give your opinion on this patch?
Andrew.
Original Message
Subject: [Mono-devel-list] Re: System.Xml patch
Date: Wed, 22 Jun 2005 12:48:06 +0300
From: Andrew Skiba <[EMAIL PROTECTED]>
To: Atsushi Eno <[EMAIL PROTECTED]>
CC: mono-devel mailing list
References: <[EMAIL PROTECTED]> <[EMAIL PROTECTED]>
Atsushi Eno wrote:
Hi Andrew,
Good catch :-) However the fix does not look correct. Your patch
also avoids entity content evaluation, that means it sometimes
results in incorrect well-formedness.
Good catch, too ;-)
How about this patch? To save iterations, I made the same check in
ResolveExternalEntityReplacementText, please tell me, if it's unnecessary.
Cheers,
Andrew Skiba.
Index: DTDReader.cs
===
--- DTDReader.cs(revision 46238)
+++ DTDReader.cs(working copy)
@@ -641,8 +641,9 @@
private void ResolveExternalEntityReplacementText
(DTDEntityBase decl)
{
if (decl.LiteralEntityValue.StartsWith ("___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Mono-devel-list] Re: System.Xml patch
Hi Andrew,
How about this patch? To save iterations, I made the same check in
ResolveExternalEntityReplacementText, please tell me, if it's unnecessary.
This patch looks fine. Let's check it in svn.
I'll apply the patch attached after your commit.
Index: DTDReader.cs
===
--- DTDReader.cs(revision 46238)
+++ DTDReader.cs(working copy)
@@ -641,8 +641,9 @@
private void ResolveExternalEntityReplacementText
(DTDEntityBase decl)
{
if (decl.LiteralEntityValue.StartsWith ("
What does this mean? I guess you have in mind that there are cases that
entities could be used inside attributes, but attribute content check
is less prohibiting and '<' characters are checked anyways.
(BTW we won't understand what "-AS" means there ;-)
Besides the comment itself.
In fact I also don't like that part of code. Actually, though
replacing entities with a simple string is good for performance
as compared to such code that loads entity possibly from external
files every time, it causes incorrect BaseURI resolution (that
results in incorrect not-wf error for XHTML 1.1 DTD; bug #51495).
So, basically rewriting that entity expansion part is the best
solution. But I haven't tried that since it will not be done as a
quick hack and it will first result in several breakage in
standalone tests.
Thanks,
Atsushi Eno
Index: Test/System.Xml/XmlTextReaderTests.cs
===
--- Test/System.Xml/XmlTextReaderTests.cs (revision 46370)
+++ Test/System.Xml/XmlTextReaderTests.cs (working copy)
@@ -1100,5 +1100,20 @@
AssertEquals ("#1", 0xf090, (int) r.Value [0]);
AssertEquals ("#1", 0x8080, (int) r.Value [1]);
}
+
+ [Test]
+ [ExpectedException (typeof (XmlException))]
+ public void EntityDeclarationNotWF ()
+ {
+ string xml = @"
+
+ '>
+ ]>
+ &e; ";
+ XmlTextReader xtr = new XmlTextReader (xml,
+ XmlNodeType.Document, null);
+ xtr.Read ();
+ }
}
}
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Mono-devel-list] Re: System.Xml patch
Atsushi Eno wrote:
Hi Andrew,
Good catch :-) However the fix does not look correct. Your patch
also avoids entity content evaluation, that means it sometimes
results in incorrect well-formedness.
Good catch, too ;-)
How about this patch? To save iterations, I made the same check in
ResolveExternalEntityReplacementText, please tell me, if it's unnecessary.
Cheers,
Andrew Skiba.
Index: DTDReader.cs
===
--- DTDReader.cs(revision 46238)
+++ DTDReader.cs(working copy)
@@ -641,8 +641,9 @@
private void ResolveExternalEntityReplacementText
(DTDEntityBase decl)
{
if (decl.LiteralEntityValue.StartsWith ("___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Mono-devel-list] Re: System.Xml patch
Hi Andrew, Good catch :-) However the fix does not look correct. Your patch also avoids entity content evaluation, that means it sometimes results in incorrect well-formedness. valid-sa-086 looks like this: "> ]> &e; If there is such xml like: "> ]> &e; Then it should be rejected. Note that there is '&' in the second ENTITY declaration. So we still have to invoke ResolveInternalEntityReplacementText(), but without replacing "replacement text". Am not sure I can fix it soon but would like to take care of it at some stage. Thanks Atsushi Eno Andrew Skiba wrote: Hi! Please review the attached patch. It fixes one of the regressions in W3C testsuite. I checked it with nunit tests and xsl standalone, it does not break anything. The idea of the patch is that "If the same entity is declared more than once, the first declaration encountered is binding" (citation from http://www.w3.org/TR/xml11/#sec-entity-decl ). In this test (valid-sa-086) they redefine an entity with unclosed tag, and current implementation crashes. I made the minimal necessary change, but may be you will want to include other flows as well. Regards, Andrew. ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
