You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Will Glass-Husain <wg...@gmail.com> on 2007/06/01 06:06:56 UTC
macros - max recursion depth
(changing the subject line for better tracking)
What are your preliminary thoughts on this -- how are you thinking of
tracking the recursion level for each macro?
WILL
On 5/31/07, Supun Kamburugamuva <su...@gmail.com> wrote:
>
> > My suggestion is to run with the macro max recursion depth issue while
> we
> > argue this out.
> >
>
> I'm in the process of implementing max recursion depth issue.
>
> Supun.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com
Re: macros - max recursion depth
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Supin,
Supun Kamburugamuva wrote:
> I think the first challenge is to discover recursion. I was thinking
> about discovering recursion when a macro call is being executed.
This probably won't work. Just because a macro calls itself doesn't mean
that infinite recursion will result. I have several macros that are
self-calling, but I take care to avoid infinite recursion.
It's the infinite case (or, nearly infinite... let's say 100 levels)
that you want to avoid instead of throwing a StackOverflowError.
- -chris
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFGYGIK9CaO5/Lv0PARAkJpAJ9OmlFBKmlU6UT+u1jT0GuKPHcp0QCeMLIK
igG06kPto/AsFTO1NzrmdrM=
=kzdS
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Nathan Bubna <nb...@gmail.com>.
On 6/1/07, Christopher Schultz <ch...@christopherschultz.net> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Nathan,
>
> Nathan Bubna wrote:
> >> 2. Throw another exception (MacroDepthExceededException?)
> >>
> >> The way I see it, neither of these options is any better than simply
> >> allowing the stack overflow to occur.
> >
> > Stack overflows can be caused by many things. Throwing a
> > MacroDepthException is much more informative, and in the case of 3rd
> > party templates being introduced to a running system, can prevent DOS
> > type stuff.
>
> Yeah... as I was typing that question, I was thinking "well, stack
> overflow could mean many things", although I immediately assume that my
> template has infinite recursion in these cases ;)
:)
> I hasn't really thought about 3rd-party templates. Does anyone have any
> data on the impact of a stack overflow on a running app server? I would
> imagine that a better way to perform a DOS would be to concatenate
> strings forever in an endless loop. There's really no checking that can
> be done against that.
still plugging what holes we can ain't a bad thing :)
> Okay. Enough nay-saying from me ;)
>
> - -chris
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.7 (MingW32)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFGYGY39CaO5/Lv0PARAm9iAJ0cYAW0Rs6h5yfVwefQkvPcMnUmPgCgjnkV
> IG5pXk8OVJY+44SHv+mr/i0=
> =9F0i
> -----END PGP SIGNATURE-----
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Will Glass-Husain <wg...@gmail.com>.
Great question from Claude.
Yes, I think this is an untrusted 3rd party template issue. (or perhaps
just a defense against sloppy template design).
Looking at the original issue (VELOCITY-297), the stated intent is to avoid
StackOverflowExceptions. I see twin objectives here of of providing a more
meaningful exception and to ensure that recursive macro calls have
consistent behavior regardless of JVM settings and internal method
structure. In otherwords, I prefer to see a Velocity related exception
after 20 macro calls, rather than a stack overflow exception after umpteen
method calls.
As a corollary to this issue, maybe the exception message can contain a list
of macro calls? Something like:
org.apache.velocity.exception MacroOverflowException:
message: "Exceed maximum 20 macro calls. Call stack: macro1 -> macro2 ->
macro3 -> macro1 -> macro1 -> macro1"
(obviously, with 20 calls in the list above)
To implement this, you'd have to track the stack of macro calls during page
rendering.
WILL
On 6/1/07, Christopher Schultz <ch...@christopherschultz.net> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Nathan,
>
> Nathan Bubna wrote:
> >> 2. Throw another exception (MacroDepthExceededException?)
> >>
> >> The way I see it, neither of these options is any better than simply
> >> allowing the stack overflow to occur.
> >
> > Stack overflows can be caused by many things. Throwing a
> > MacroDepthException is much more informative, and in the case of 3rd
> > party templates being introduced to a running system, can prevent DOS
> > type stuff.
>
> Yeah... as I was typing that question, I was thinking "well, stack
> overflow could mean many things", although I immediately assume that my
> template has infinite recursion in these cases ;)
>
> I hasn't really thought about 3rd-party templates. Does anyone have any
> data on the impact of a stack overflow on a running app server? I would
> imagine that a better way to perform a DOS would be to concatenate
> strings forever in an endless loop. There's really no checking that can
> be done against that.
>
> Okay. Enough nay-saying from me ;)
>
> - -chris
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.7 (MingW32)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFGYGY39CaO5/Lv0PARAm9iAJ0cYAW0Rs6h5yfVwefQkvPcMnUmPgCgjnkV
> IG5pXk8OVJY+44SHv+mr/i0=
> =9F0i
> -----END PGP SIGNATURE-----
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com
Re: macros - max recursion depth
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Nathan,
Nathan Bubna wrote:
>> 2. Throw another exception (MacroDepthExceededException?)
>>
>> The way I see it, neither of these options is any better than simply
>> allowing the stack overflow to occur.
>
> Stack overflows can be caused by many things. Throwing a
> MacroDepthException is much more informative, and in the case of 3rd
> party templates being introduced to a running system, can prevent DOS
> type stuff.
Yeah... as I was typing that question, I was thinking "well, stack
overflow could mean many things", although I immediately assume that my
template has infinite recursion in these cases ;)
I hasn't really thought about 3rd-party templates. Does anyone have any
data on the impact of a stack overflow on a running app server? I would
imagine that a better way to perform a DOS would be to concatenate
strings forever in an endless loop. There's really no checking that can
be done against that.
Okay. Enough nay-saying from me ;)
- -chris
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFGYGY39CaO5/Lv0PARAm9iAJ0cYAW0Rs6h5yfVwefQkvPcMnUmPgCgjnkV
IG5pXk8OVJY+44SHv+mr/i0=
=9F0i
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Nathan Bubna <nb...@gmail.com>.
On 6/1/07, Christopher Schultz <ch...@christopherschultz.net> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Will,
>
> Will Glass-Husain wrote:
> > I'd be happy with the latter item. If this is set to a sufficiently high
> > number (20?) this prevents runaway macros yet will not limit typical use.
>
> Can I ask a stupid question: what is the problem we're trying to solve?
> Are we trying to avoid getting a stack overflow from runaway recursion?
> If so, what are we going to replace it with? I see we have two options:
>
> 1. Simply deny the macro call
> or
> 2. Throw another exception (MacroDepthExceededException?)
>
> The way I see it, neither of these options is any better than simply
> allowing the stack overflow to occur.
Stack overflows can be caused by many things. Throwing a
MacroDepthException is much more informative, and in the case of 3rd
party templates being introduced to a running system, can prevent DOS
type stuff.
> Am I crazy?
not at all. this is a really good thing to ask before putting effort
into something like this!
> - -chris
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.7 (MingW32)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFGYGKj9CaO5/Lv0PARAq/rAKCjkbaIRIYas8AysrzDO+hQora+twCfSxix
> 7OuLJWdY58pbW8hLR6z8A6Y=
> =hWSQ
> -----END PGP SIGNATURE-----
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Will,
Will Glass-Husain wrote:
> I'd be happy with the latter item. If this is set to a sufficiently high
> number (20?) this prevents runaway macros yet will not limit typical use.
Can I ask a stupid question: what is the problem we're trying to solve?
Are we trying to avoid getting a stack overflow from runaway recursion?
If so, what are we going to replace it with? I see we have two options:
1. Simply deny the macro call
or
2. Throw another exception (MacroDepthExceededException?)
The way I see it, neither of these options is any better than simply
allowing the stack overflow to occur.
Am I crazy?
- -chris
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFGYGKj9CaO5/Lv0PARAq/rAKCjkbaIRIYas8AysrzDO+hQora+twCfSxix
7OuLJWdY58pbW8hLR6z8A6Y=
=hWSQ
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Supun Kamburugamuva <su...@gmail.com>.
Hi,
Thanks a lot for fixing those minor issues. I have configured JDK 1.4 -).
I would like to start on another issue. If there is any idea from the
community, please let me know.
Supun.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Will Glass-Husain <wg...@gmail.com>.
Hi Supun,
Great work! I've gone ahead and committed your patch. I especially
appreciate your attention to detail in including a good unit test and
following our coding style.
I found some minor issues, which I changed before committing. I list them
below for future reference. Probably in the future I may bounce some of
these back to you for tweaks on your end.
(1) I changed the property name to "velocimacro.max.depth" to match similar
property "directive.parse.max.depth".
(2) Since the patch deals with calling depth, not recursion, I changed all
references in comments from "recursion" to "calling depth".
(3) In VelocimacroTestCase, there was a subtle reliance on JDK 1.5.
(Remember, we require max JDK 1.4 for compiling and 1.3 for running). An
int was passed as a parameter when an Integer was expected. Recommendation:
set your IDE to enforce JDK 1.4 syntax.
(4) In VelocimacroTestCase, one of the tests was repeated. (deleted the
extra)
(5) You can't change the class Directive, it's one of the public APIs.
(yes, this is vague). Basically, any class that can be extended or an
interface that can be implemented by changing velocity.properties has to
keep the same API. Luckily, since MacroOverflowException is a subclass of
RuntimeException you don't need to declare it in the Directive.render()
method.
WILL
On 6/6/07, Will Glass-Husain <wg...@gmail.com> wrote:
>
> sure, please do!
>
> On 6/6/07, Supun Kamburugamuva <su...@gmail.com> wrote:
> >
> > Hi,
> >
> > +1 for velocimacro.depth.max.
> >
> > I have alomost completed the implementation. Can I submit a patch? I'm
> > sure there are lot of things to consider.
> >
> > Supun.
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> > For additional commands, e-mail: dev-help@velocity.apache.org
> >
> >
>
>
> --
> Forio Business Simulations
>
> Will Glass-Husain
> wglass@forio.com
> www.forio.com
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com
Re: macros - max recursion depth
Posted by Will Glass-Husain <wg...@gmail.com>.
sure, please do!
On 6/6/07, Supun Kamburugamuva <su...@gmail.com> wrote:
>
> Hi,
>
> +1 for velocimacro.depth.max.
>
> I have alomost completed the implementation. Can I submit a patch? I'm
> sure there are lot of things to consider.
>
> Supun.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com
Re: macros - max recursion depth
Posted by Supun Kamburugamuva <su...@gmail.com>.
Hi,
+1 for velocimacro.depth.max.
I have alomost completed the implementation. Can I submit a patch? I'm
sure there are lot of things to consider.
Supun.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Will Glass-Husain <wg...@gmail.com>.
Hi Supun,
You can see the current list of properties in RuntimeConstants.java
How about
velocimacro.depth.max?
I wouldn't use the word "recursion" since this applies to macro calls in
general.
WILL
On 6/4/07, Supun Kamburugamuva <su...@gmail.com> wrote:
>
> Hi,
>
> What should be the property name for defining max number of macro calls.
>
> Supun.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com
Re: macros - max recursion depth
Posted by Supun Kamburugamuva <su...@gmail.com>.
Hi,
What should be the property name for defining max number of macro calls.
Supun.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Will Glass-Husain <wg...@gmail.com>.
Hi Supun,
I think that makes sense. It's probably broader than is actually supported
(for example, the Velocimacros can be configured so that a macro in one page
does not overwrite the global macros), but it satisfies the constraint of
uniqueness.
Keeping track of this in the factory is fine. I was thinking you could add
a method to the internal context object, but your suggestion seems better.
When you have something ready to share, post a patch on JIRA and we can talk
about it. Remember that ultimately you'll need:
--> code to implement issue
--> new test for issue
--> old tests (e.g. "ant test") must pass
--> code should follow standard style guidelines documented on the Wiki.
WILL
On 6/3/07, Supun Kamburugamuva <su...@gmail.com> wrote:
>
> > I'm not sure what is the correct place to keep track of macros.
> >
>
> I'm keeping track of Macro execution in VelociMacroFactory. Is it
> sufficient to use the current template name and the macro name to
> uniquely identify a macro in the runtime?
>
> Supun.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com
Re: macros - max recursion depth
Posted by Supun Kamburugamuva <su...@gmail.com>.
> I'm not sure what is the correct place to keep track of macros.
>
I'm keeping track of Macro execution in VelociMacroFactory. Is it
sufficient to use the current template name and the macro name to
uniquely identify a macro in the runtime?
Supun.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Supun Kamburugamuva <su...@gmail.com>.
On 6/1/07, Will Glass-Husain <wg...@gmail.com> wrote:
> I agree with Claude. It seems there are two approaches
>
> --> track max recursion: for each macro call in a sequence, track the number
> of times you call it. (basically, maintain a map of macros with a counter
> for each)
>
> --> track embedded macro calls: track a single number of the macro call
> depth (forget recursion, just track macro calls within macro calls)
>
I'm not sure what is the correct place to keep track of macros.
Supun.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Will Glass-Husain <wg...@gmail.com>.
I agree with Claude. It seems there are two approaches
--> track max recursion: for each macro call in a sequence, track the number
of times you call it. (basically, maintain a map of macros with a counter
for each)
--> track embedded macro calls: track a single number of the macro call
depth (forget recursion, just track macro calls within macro calls)
I'd be happy with the latter item. If this is set to a sufficiently high
number (20?) this prevents runaway macros yet will not limit typical use.
WILL
On 6/1/07, Claude Brisson <cl...@renegat.net> wrote:
>
> If it's easier to implement, it may be pertinent to just implement a
> "max macro stack depth".
>
> Claude
>
> Le vendredi 01 juin 2007 à 09:21 -0700, Supun Kamburugamuva a écrit :
> > I think the first challenge is to discover recursion. I was thinking
> > about discovering recursion when a macro call is being executed.
> >
> > When the node tree is rendered lets say at some point it reaches a
> > macro. At this point, if I can keep track of whether children of this
> > node calls the same macro again before the original call finishes, I
> > will be able to discover recursion.
> >
> > I have tried this method in several places i.e. ASTDirectve.render and
> > VelocimacroProxy.render by keeping track of the state in ASTDirective
> > class and VelocimacroProxy class. First time render method is called
> > by a Macro invocation I kept the status in a private variable and when
> > the next call occurs I checked this variable to see weather the
> > previous macro call is completed. But then I realized that objects
> > from these classes are created every time a call occurs to a macro. So
> > I cannot keep track of the macro calling state in these classes.
> >
> > Supun.
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> > For additional commands, e-mail: dev-help@velocity.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com
Re: macros - max recursion depth
Posted by Claude Brisson <cl...@renegat.net>.
If it's easier to implement, it may be pertinent to just implement a
"max macro stack depth".
Claude
Le vendredi 01 juin 2007 à 09:21 -0700, Supun Kamburugamuva a écrit :
> I think the first challenge is to discover recursion. I was thinking
> about discovering recursion when a macro call is being executed.
>
> When the node tree is rendered lets say at some point it reaches a
> macro. At this point, if I can keep track of whether children of this
> node calls the same macro again before the original call finishes, I
> will be able to discover recursion.
>
> I have tried this method in several places i.e. ASTDirectve.render and
> VelocimacroProxy.render by keeping track of the state in ASTDirective
> class and VelocimacroProxy class. First time render method is called
> by a Macro invocation I kept the status in a private variable and when
> the next call occurs I checked this variable to see weather the
> previous macro call is completed. But then I realized that objects
> from these classes are created every time a call occurs to a macro. So
> I cannot keep track of the macro calling state in these classes.
>
> Supun.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: macros - max recursion depth
Posted by Supun Kamburugamuva <su...@gmail.com>.
I think the first challenge is to discover recursion. I was thinking
about discovering recursion when a macro call is being executed.
When the node tree is rendered lets say at some point it reaches a
macro. At this point, if I can keep track of whether children of this
node calls the same macro again before the original call finishes, I
will be able to discover recursion.
I have tried this method in several places i.e. ASTDirectve.render and
VelocimacroProxy.render by keeping track of the state in ASTDirective
class and VelocimacroProxy class. First time render method is called
by a Macro invocation I kept the status in a private variable and when
the next call occurs I checked this variable to see weather the
previous macro call is completed. But then I realized that objects
from these classes are created every time a call occurs to a macro. So
I cannot keep track of the macro calling state in these classes.
Supun.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org