You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Mark Thomas <ma...@apache.org> on 2010/01/08 12:36:31 UTC
EL issues and 6.0.x release
I think I have got to the bottom of why the EL has been so fragile and
why the fixes to the various edge case bug fixes have invariably created
new edge cases. It is probably easier to explain by considering an
example. Using the echo tag from the newly added EL test cases consider
the following JSP extract:
<tags:echo echo="${1+1}" />
<tags:echo echo="\${1+1}" />
<tags:echo echo="\\${1+1}" />
The JSP and EL specs are a little fuzzy/inconsistent but the guidance we
received from the expert group was:
- the whole attribute should be unquoted as per the rules for quoting
JSP attributes
- everything outside an actual expression after unquoting should be
treated as a literal
The issue is that we performed JSP attribute unquoting and EL parsing in
two independent steps. This creates an ambiguity. If the attribute
values above are unquoted then we get the following:
${1+1}
${1+1}
\${1+1}
The first is an EL expression and will be treated as such.
The second is a literal. However, the EL parser will treat it as an
expression.
The third is a literal '\' followed by an expression but the EL parser
will see an escaped literal.
My proposed solution is to modify the output of the attribute unquoting
to ensure the EL Parser correctly interprets the result. ie:
${1+1}
${'$'}{1+1}
${'\'}${1+1}
In writing this I think a few of my test cases might still be wrong.
I'll take a look and update them as necessary.
I have some rough code that implements this scheme. I am in the process
of integrating it into Jasper for trunk. I can see this taking little
while to iron out the wrinkles before I'll be ready t propose a
back-port for 6.0.x. I don't think we should hold up the 6.0.x release
for this. I think it is more important to get the next 6.0.x release out
than to wait to fix something that has been broken for quite a while anyway.
One area where help would be appreciated is in the test cases. If folks
see an edge case that isn't covered by the current set of tests please
do add it.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: EL issues and 6.0.x release
Posted by Mark Thomas <ma...@apache.org>.
On 08/01/2010 11:53, Remy Maucherat wrote:
> On Fri, 2010-01-08 at 11:36 +0000, Mark Thomas wrote:
>> I have some rough code that implements this scheme. I am in the process
>> of integrating it into Jasper for trunk. I can see this taking little
>> while to iron out the wrinkles before I'll be ready t propose a
>> back-port for 6.0.x. I don't think we should hold up the 6.0.x release
>> for this. I think it is more important to get the next 6.0.x release out
>> than to wait to fix something that has been broken for quite a while anyway.
>
> I don't think making major changes to 6.x to fix edge cases nobody knows
> about is a good idea ...
Unfortunately, as can be seen in Bugzilla [1]-[7] to list a few of them,
plenty of people do know about them. Unfortunately most of the attempts
to fix these issues have usually introduced another one. The current
problem is [8].
The change is likely to be on the larger side of what we are normally
comfortable with in 6.0.x but lets wait and see exactly how big the
change is and what benefits we get (in terms of less fragility, code
with test cases for regression testing etc) before deciding how good or
bad an idea this porting this will be. At the moment the discussion is
rather academic as we don't have a concrete backport to discuss.
Mark
[1] https://issues.apache.org/bugzilla/show_bug.cgi?id=42565
[2] https://issues.apache.org/bugzilla/show_bug.cgi?id=44994
[3] https://issues.apache.org/bugzilla/show_bug.cgi?id=45427
[4] https://issues.apache.org/bugzilla/show_bug.cgi?id=45451
[5] https://issues.apache.org/bugzilla/show_bug.cgi?id=45511
[6] https://issues.apache.org/bugzilla/show_bug.cgi?id=46596
[7] https://issues.apache.org/bugzilla/show_bug.cgi?id=47413
[8] https://issues.apache.org/bugzilla/show_bug.cgi?id=48112
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: EL issues and 6.0.x release
Posted by Remy Maucherat <re...@apache.org>.
On Fri, 2010-01-08 at 11:36 +0000, Mark Thomas wrote:
> I have some rough code that implements this scheme. I am in the process
> of integrating it into Jasper for trunk. I can see this taking little
> while to iron out the wrinkles before I'll be ready t propose a
> back-port for 6.0.x. I don't think we should hold up the 6.0.x release
> for this. I think it is more important to get the next 6.0.x release out
> than to wait to fix something that has been broken for quite a while anyway.
I don't think making major changes to 6.x to fix edge cases nobody knows
about is a good idea ...
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: EL issues and 6.0.x release
Posted by Konstantin Kolinko <kn...@gmail.com>.
2010/1/31 Mark Thomas <ma...@apache.org>:
> On 30/01/2010 17:41, Mark Thomas wrote:
>> On 30/01/2010 07:33, Konstantin Kolinko wrote:
>>> Regarding the implementation, AttributeParser.java class. I think
>>> that, based on the above, we can fix it to solve bug 48627. Other
>>> parts of the new implementation will remain unchanged.
>>
>> I'll take another look at this. I thought that this wouldn't work but
>> that may because I was doing my testing before I fixed the EL parsing.
>> If this doesn't work I have an alternative plan.
>
> Looks like it will work. Just running the TCK to be sure.
>
>>> 1. In JSP 2.1 spec there is an option to selectively disable '#'
>>> expressions when '$' ones are still enabled. The name of that option
>>> is "deferredSyntaxAllowedAsLiteral".
>>>
>>> As of now, AttributeParser takes care of isELIgnored option, but does
>>> not know about deferredSyntaxAllowedAsLiteral one.
>>
>> Probably a bug. We should write some test cases for this first though to
>> check.
>
> Yep bug. Test cases written. Fixed. Just running the TCK to be sure.
>
>>> 3. EL spec (ch.1.2.3 of EL 2.1 spec) says that "It is illegal to mix
>>> ${} and #{} constructs in a composite expression." though followed by
>>> "This restriction may be lifted in future versions".
>>>
>>> AttributeParser#parseLiteral() has the following clause:
>>>
>>> } else if (ch == type){
>>>
>>> I think it has to process '#' and '$' expressions in the same way, and
>>> the "mix ${} and #{}" rule should be checked either explicitly here,
>>> or elsewhere. I have not researched the question where it is actually
>>> checked.
>>
>> More tests cases required.
>
> Test case added. This is already handled by the EL impl.
>
If we have "#{foo}${bar}" then parseEL() won't be triggered for the
${bar} part, and so won't correctly parse quotes etc.
Though probably that makes not much of a difference. I think
${foo}#{bar will be treated as an error regardless of what garbage or
not that bar is .
As of now
<tags:echo echo="${foo}#{bar}" />
<tags:echo echo="${foo}#{bar" />
or "legal"
<tags:echo echo="#{foo}" />
all result in the same compile-time exception: "According to TLD or
attribute directive in tag file, attribute echo does not accept any
expressions"
The "any expressions" part of the message text is wrong.
All that is a minor/cosmetic issue. I won't care much if it is
postponed for later or not fixed in a while at all.
>>> Lastly,
>>> when Mark was testing TC7 with JSP 2.2 TCK, he caught several minor EL
>>> evaluation issues. Those are fixed in TC7, and I think some of them
>>> have to be backported to TC6.
>>
>> They all need back-porting. I didn't propose them at the time since the
>> issues had existing for all of the 6.0.x release and no-one had
>> complained. I didn't want to hold up the 6.0.24 release.
>
> Proposed.
>
> Assuming the TCK passes I'll have a fix for 46827 and
> deferredSyntaxAllowedAsLiteral shortly.
>
One error there (for AttributeParser.java of trunk at rev.904949):
In parseEL() the literalQuote variable should be moved outside the
while (i < size && !endEL) loop. Currently its value is not preserved
among iterations.
E.g.
<tags:echo echo="${\"foo\"}\\${'bar'}" />
<tags:echo echo="${'foo'}\\${\"bar\"}" />
should both evaluate to foo\bar, but now the first one evaluates to foo${'bar'}
The rest is OK. I do not see any other errors in the implementation.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: EL issues and 6.0.x release
Posted by Mark Thomas <ma...@apache.org>.
On 30/01/2010 17:41, Mark Thomas wrote:
> On 30/01/2010 07:33, Konstantin Kolinko wrote:
>> Regarding the implementation, AttributeParser.java class. I think
>> that, based on the above, we can fix it to solve bug 48627. Other
>> parts of the new implementation will remain unchanged.
>
> I'll take another look at this. I thought that this wouldn't work but
> that may because I was doing my testing before I fixed the EL parsing.
> If this doesn't work I have an alternative plan.
Looks like it will work. Just running the TCK to be sure.
>> 1. In JSP 2.1 spec there is an option to selectively disable '#'
>> expressions when '$' ones are still enabled. The name of that option
>> is "deferredSyntaxAllowedAsLiteral".
>>
>> As of now, AttributeParser takes care of isELIgnored option, but does
>> not know about deferredSyntaxAllowedAsLiteral one.
>
> Probably a bug. We should write some test cases for this first though to
> check.
Yep bug. Test cases written. Fixed. Just running the TCK to be sure.
>> 3. EL spec (ch.1.2.3 of EL 2.1 spec) says that "It is illegal to mix
>> ${} and #{} constructs in a composite expression." though followed by
>> "This restriction may be lifted in future versions".
>>
>> AttributeParser#parseLiteral() has the following clause:
>>
>> } else if (ch == type){
>>
>> I think it has to process '#' and '$' expressions in the same way, and
>> the "mix ${} and #{}" rule should be checked either explicitly here,
>> or elsewhere. I have not researched the question where it is actually
>> checked.
>
> More tests cases required.
Test case added. This is already handled by the EL impl.
>> Lastly,
>> when Mark was testing TC7 with JSP 2.2 TCK, he caught several minor EL
>> evaluation issues. Those are fixed in TC7, and I think some of them
>> have to be backported to TC6.
>
> They all need back-porting. I didn't propose them at the time since the
> issues had existing for all of the 6.0.x release and no-one had
> complained. I didn't want to hold up the 6.0.24 release.
Proposed.
Assuming the TCK passes I'll have a fix for 46827 and
deferredSyntaxAllowedAsLiteral shortly.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: EL issues and 6.0.x release
Posted by Mark Thomas <ma...@apache.org>.
On 30/01/2010 07:33, Konstantin Kolinko wrote:
> Regarding the implementation, AttributeParser.java class. I think
> that, based on the above, we can fix it to solve bug 48627. Other
> parts of the new implementation will remain unchanged.
I'll take another look at this. I thought that this wouldn't work but
that may because I was doing my testing before I fixed the EL parsing.
If this doesn't work I have an alternative plan.
> 1. In JSP 2.1 spec there is an option to selectively disable '#'
> expressions when '$' ones are still enabled. The name of that option
> is "deferredSyntaxAllowedAsLiteral".
>
> As of now, AttributeParser takes care of isELIgnored option, but does
> not know about deferredSyntaxAllowedAsLiteral one.
Probably a bug. We should write some test cases for this first though to
check.
> 2. There are several places in AttributeParser#parseLiteral() where
>
> if (type == 0) {
> type = '$';
> }
>
> That is where the bug 48627 lies. The above code turns non-dynamic
> attribute in a dynamic one.
Agreed.
> 3. EL spec (ch.1.2.3 of EL 2.1 spec) says that "It is illegal to mix
> ${} and #{} constructs in a composite expression." though followed by
> "This restriction may be lifted in future versions".
>
> AttributeParser#parseLiteral() has the following clause:
>
> } else if (ch == type){
>
> I think it has to process '#' and '$' expressions in the same way, and
> the "mix ${} and #{}" rule should be checked either explicitly here,
> or elsewhere. I have not researched the question where it is actually
> checked.
More tests cases required.
> 4. I have not researched this question, and I *can be wrong* in this
> point, but I think that when EL is evaluated, the engine does not know
> about isELIgnored and deferredSyntaxAllowedAsLiteral options.
The parser does know. Parsing is done in two phases. The first phase
parses directives, the second phase parses everything else. This ensures
when everything else is parsed, the parser knows the correct way to
handle stuff that might be an expression.
See http://svn.apache.org/viewvc?view=revision&revision=708165
> Lastly,
> when Mark was testing TC7 with JSP 2.2 TCK, he caught several minor EL
> evaluation issues. Those are fixed in TC7, and I think some of them
> have to be backported to TC6.
They all need back-porting. I didn't propose them at the time since the
issues had existing for all of the 6.0.x release and no-one had
complained. I didn't want to hold up the 6.0.24 release.
Thanks for the review.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: EL issues and 6.0.x release
Posted by Konstantin Kolinko <kn...@gmail.com>.
2010/1/8 Mark Thomas <ma...@apache.org>:
> I think I have got to the bottom of why the EL has been so fragile and
> why the fixes to the various edge case bug fixes have invariably created
> new edge cases. It is probably easier to explain by considering an
> example. Using the echo tag from the newly added EL test cases consider
> the following JSP extract:
>
> <tags:echo echo="${1+1}" />
> <tags:echo echo="\${1+1}" />
> <tags:echo echo="\\${1+1}" />
>
> The JSP and EL specs are a little fuzzy/inconsistent but the guidance we
> received from the expert group was:
> - the whole attribute should be unquoted as per the rules for quoting
> JSP attributes
> - everything outside an actual expression after unquoting should be
> treated as a literal
>
> The issue is that we performed JSP attribute unquoting and EL parsing in
> two independent steps. This creates an ambiguity. If the attribute
> values above are unquoted then we get the following:
>
> ${1+1}
> ${1+1}
> \${1+1}
>
> The first is an EL expression and will be treated as such.
> The second is a literal. However, the EL parser will treat it as an
> expression.
> The third is a literal '\' followed by an expression but the EL parser
> will see an escaped literal.
>
> My proposed solution is to modify the output of the attribute unquoting
> to ensure the EL Parser correctly interprets the result. ie:
> ${1+1}
> ${'$'}{1+1}
> ${'\'}${1+1}
>
Some more notes regarding this, as I am reviewing docs and
implementation in view of issues reported for 6.0.24.
Especially, I was looking at chapter JSP.10.1.11 (XML View /
"Request-Time Attribute Expressions") and especially the table Table
JSP.10-2 (XML View of an Escaped EL Expression) in that chapter of JSP
2.1 specification. The question was how it avoids the pitfall of bug
48627 [1]. More on that below.
[1] https://issues.apache.org/bugzilla/show_bug.cgi?id=48627
First, for reference: r899148 is the revision when the new
implementation was applied to the 6.0 branch [2].
[2] http://svn.apache.org/viewvc?view=revision&revision=899148
Highlights for that implementation:
#1. It fixes several EL parsing issues.
#2. It implements '\' -> ${'\\'} or #{'\\'} (depending on the
expression type) replacement
#1. is mainly implemented by the changes in ELParser.jjt file
#2. is mainly implemented by the new AttributeParser.java class
Especially #1. fixes the issue, where '\' in a literal EL expression
(ch. 1.2.2 of EL 2.1 spec) was interpreted as an escape character
(actually, it is not an escape character there, only \${ and \#{ ).
E.g., evaluation of "\\\\" as an EL expression should give "\\\\" and
not "\\". It gave '\\' before r899148. I mentioned that issue in [3].
The catch point here is that if that EL parsing issue is fixed in #1.,
then total unconditional replacement '\' -> ${'\\'} or #{'\\'} of #2.
is not needed. That is, with #1 being fixed a single '\' is correctly
evaluated as '\' by EL and does not need escaping.
Look at Table JSP.10-2 inchapter JSP.10.1.11 of JSP 2.1 spec, that I
mentioned. The "XML View" column there is always a valid EL
expression, with all necessary EL escaping. The rule there is that if
the resulting expression is a literal (all ${foo} are escaped as
\${foo}), then the '\'s are remaining as they are. If the resulting
expression is a composite EL expression (there is an unescaped ${foo}
there), only in that case '\'s are replaced with ${'\\'}.
That is, in that table the only lines where ${'\\'} is there are those
where the result is an evaluatable EL expression.
That avoids the pitfall of BZ 48627 (where "\\" value of an attribute
became an EL expression in a tag where dynamic expressions where not
allowed): all non-dynamic attributes remain as non-dynamic.
That also avoids the uncertainty of whether '\' should be escaped as
${'\\'} or as #{'\\'}. As eval EL-expression is always there in those
cases, we always know what type of expression ($ or #) it is.
[3] http://marc.info/?t=126261733500006&r=1&w=2
Regarding the implementation, AttributeParser.java class. I think
that, based on the above, we can fix it to solve bug 48627. Other
parts of the new implementation will remain unchanged.
I have several comments on the AttributeParser class.
1. In JSP 2.1 spec there is an option to selectively disable '#'
expressions when '$' ones are still enabled. The name of that option
is "deferredSyntaxAllowedAsLiteral".
As of now, AttributeParser takes care of isELIgnored option, but does
not know about deferredSyntaxAllowedAsLiteral one.
2. There are several places in AttributeParser#parseLiteral() where
if (type == 0) {
type = '$';
}
That is where the bug 48627 lies. The above code turns non-dynamic
attribute in a dynamic one.
3. EL spec (ch.1.2.3 of EL 2.1 spec) says that "It is illegal to mix
${} and #{} constructs in a composite expression." though followed by
"This restriction may be lifted in future versions".
AttributeParser#parseLiteral() has the following clause:
} else if (ch == type){
I think it has to process '#' and '$' expressions in the same way, and
the "mix ${} and #{}" rule should be checked either explicitly here,
or elsewhere. I have not researched the question where it is actually
checked.
4. I have not researched this question, and I *can be wrong* in this
point, but I think that when EL is evaluated, the engine does not know
about isELIgnored and deferredSyntaxAllowedAsLiteral options.
If that is the truth, then in AttributeParser#parseLiteral()
if (isELIgnored) ${ should be escaped as \${
if (isELIgnored || deferredSyntaxAllowedAsLiteral) #{ should be escaped as \#{
It will result in expression that can be directly evaluated where all
those unsupported expressions are escaped. Are we already doing the
same escaping elsewhere when those options are in effect, or do we
carry those isELIgnored, deferredSyntaxAllowedAsLiteral options all
along with unescaped expressions until evaluation time?
Lastly,
when Mark was testing TC7 with JSP 2.2 TCK, he caught several minor EL
evaluation issues. Those are fixed in TC7, and I think some of them
have to be backported to TC6. Though, I have not reviewed them in
detail yet. For reference, here are those revision numbers:
899635 <- I've already proposed this one for backport
899653
899769
899770
899783
899788
899792
899916
899918
899919
899935
899949
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: EL issues and 6.0.x release
Posted by Mark Thomas <ma...@apache.org>.
On 08/01/2010 12:18, Tim Funk wrote:
> +1 to trunk
> -0 for backport to 6.0.x
>
> Is this legal? ${'\'}${1+1}
> Shouldn't it be: ${'\\'}${1+1}
Yes it should.
and the Java code to append the \ is:
result.append("${'\\\\'}");
This is part of what makes this so tricky to get right. The quoting
rules vary between Java, JSPs and EL. A number of the initial failures
with my tests were because I had quoted stuff incorrectly in the test case.
The good news is that one of my test cases was failing because of
exactly this error. As you'll see which my next commit (hopefully later
today) things are a lot easier to test and there are a lot more test
cases. In fact the majority of the changes will be test cases rather
than changes to Jasper/EL.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: EL issues and 6.0.x release
Posted by Tim Funk <fu...@apache.org>.
+1 to trunk
-0 for backport to 6.0.x
Is this legal? ${'\'}${1+1}
Shouldn't it be: ${'\\'}${1+1}
Or phrased another way, what should I see on the screen with this:
${1+1}${'\t'}${1+1}
"2 2" or "2\t2"
-Tim
On 1/8/2010 6:36 AM, Mark Thomas wrote:
>
> One area where help would be appreciated is in the test cases. If folks
> see an edge case that isn't covered by the current set of tests please
> do add it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org