Re: [Gnump3d-devel] files.pm

[Steve Kemp]

On Mon, Apr 10, 2006 at 10:03:14PM -0500, Aaron Brown wrote:

> Looking at the code I think I see a couple of potential pitfalls, 
> and I want to mention them here to see if:
> 1) I'm crazy
> 2) Someone's noticed this already
> 3) Someone's fixed this already

  You're not crazy, it hasn't been noticed, nobody has fixed it yet,
 and looking at this is much appreciated.

> I apologize if this doesn't quite follow the normal protocol for such
> things, but I don't often have code suggestions to submit so I'm not
> sure how exactly to go about it.  Anyway, here goes...

  You did good :)

> The code snippet above is the first step in spotting comments which
> contain special directives (like Echo, Include, etc.).  (Let's call it
> an "action comment" for the time being.)  I worry that the regular
> expression is too greedy: If I use a line like this:
> <!-- include="one.html" --> <!-- include="two.html" -->
> ...the results will be unpredictable.  The regex would be better like so:
>       $line =~ /(.*?)<!--\s+(.*?)\s+-->(.*)/

  Agreed.

> The whole thing would then be best put into a loop to catch cases like
> my example above, where a single line may contain more than one Action
> Comment.

  Agreed.

> This snippet is where the Action Comment processing becomes recursive,
> checking each included file for yet more Action Comments.  The problem
> is that there is no protection against infinite recursion.  There needs
> to be a variable outside this local scope or else another parameter
> which records all the filenames already processed (a simple hash of the
> names would do) to prevent one from being processed twice, creating a
> loop.  Right now, I think I could bring my system to its knees with a
> couple of files like this:

  Agreed too.

  I think this latter problem is one I could live with, since only
 a "trusted" user could modify the site templates, however it is
 something that should be fixed.

  For the moment I've written a simple test case and committed it
 to CVS along with a basic fixing of some uninitialised variables
 in files.

  If you update you should be able to run the test case by executing:

    perl tests/template-handling.t

  Right now the failing case contains a pair of inclusion directives
 on a single line - failing because each line is processed only once.

  If you want to try fixing the case so it passes and submitting a
 diff that would be appreciated.  If you don't have any luck then
 I'll try to fix it tomorrow evening.

> So, what sayeth the dev community?  Am I off base?  Are these really
> possible bugs or have they already been fixed?

  They are all bugs :)

  They are all bugs which are pretty much my own fault for trying to
 code a simple templating system - something I've been keen to correct
 with the migration to HTML::Template.

  However that transition is stalled since people have convinced me
 that my approach to i18n is flawed.  So fixing the code is definitely
 a good thing and wouldn't be a waste of time.

  There should probably be a new release with minor bugfixes and the
 file-types updates soonish.  Otherwise I'll lose interest.

Steve
-- 
Debian GNU/Linux System Administration
http://www.debian-administration.org/