Tue 27 Apr 2010 04:09:33 PM UTC, comment #9:
I checked in the following change for this problem:
http://hg.savannah.gnu.org/hgweb/octave/rev/cb0883127251
|
Mon 12 Apr 2010 08:35:35 PM UTC, comment #8:
That's interesting. I'm guessing that what happens is that each time tt is reparsed a new function object is created--the new object is distinct from the prior one for stack popping. So tt.m blows past the 255 limit. I don't think this is intentional? Should replacing/reparsing an already-executed function be forbidden when the function is already on the stack? That may get get difficult. Relative recursion limits are always going to open things it up to gaming.
On the other hand, the advantage of the per-function stuff is that it always fails the same way no matter how deep you were when you started the nested/recursive call. Imagine you have a function that goes 254 deep before starting to return. From the prompt it will never fail but when included in a function users will get a recursion error. I think that could be confusing to users.
|
Mon 12 Apr 2010 07:18:34 PM UTC, comment #7:
I'd like to note that to make things absolutely bulletproof, we'd need to have an extra total limit on call stack anyway. I can imagine a script dynamically creating another script using tmpnam, then sourcing it (and fooling the checks).
For another (completely braindead) example how to forge an infinite recursion, consider this stupid example:
function tt()
fdisp (stderr, length (dbstack));
system ("touch ./tt.m");
pause (0.2);
rehash ();
tt();
endfunction
I'm not sure whether we should attempt to "fix" this nonsense, maybe we should just have a limit (say, 100*max_recursion_depth) on the total size of the call stack for safety.
The original issue would be fixed as a side effect.
|
Mon 12 Apr 2010 03:50:50 PM UTC, comment #6:
I'm currently working on implementing a by-script version, but I could go back to the original approach if that is preferred. I don't have a strong preference either way. I think that doing it the by-script way benefits me personally in terms of becoming more familiar with octave's sources. That said, I'm somewhat busier now and I can't project when I'll have a patch. If it is urgent someone else should feel free to take over. I'm happy to keep working on this.
|
Mon 12 Apr 2010 01:22:26 PM UTC, comment #5:
I just thought the added complexity is not worth the added benefit, because probably nobody will care whether it's consistent or not. max_recursion_limit is mainly there to prevent infinite recursion, and if I really need to increase it, I do so on an ad hoc basis anyway. And source() is probably never intentionally used with deep recursion.
But that's subjective, feel free to do it consistently if you do care.
|
Mon 12 Apr 2010 12:51:27 PM UTC, comment #4:
Yes, you could just count recursive invocations of the source function itself. The reason I proposed the other solution is because it would be more consistent with what currently happens with script files that are recursive. For a script file that calls itself, the recursion limit is Vmax_recursion_limit. For a pair of mutually recursive scripts, it is twice that number. If you just count calls to source, you will get Vmax_recursion_limit calls to source itself, which will mean half as many allowed calls for a pair of mutually recursive scripts that call each other via source as would happen if they call each other directly using the script file execution feature of Octave. I agree that this is not likely to be a widely used feature, but the solution I'm proposing isn't all that complex, so why not make the two cases behave the same?
|
Mon 12 Apr 2010 10:03:16 AM UTC, comment #3:
That's an overkill IMHO. I'd say just use Vmax_recursion_limit, and document it to be also a maximum value for recursive calls of source().
|
Fri 09 Apr 2010 08:57:27 PM UTC, comment #2:
I think the limit should be Vmax_recursion_limit, not just the same value. That way, if a user sets max_recursion_limit, it will also affect recursive uses of the source function.
To work the same as other recursion counts, we should be counting on a per-file basis, not just recursive calls to the source function. I would do that with a map::<std::string, int> object so you can keep track of the recursion count for a given full file name.
|
Fri 09 Apr 2010 06:41:32 PM UTC, comment #1:
One way to fix this would be to limit the depth of source file nesting. This would prevent other runaway circular sourcing as well as the recursive example. Something like this works for me with limited testing so far:
I'm not sure this follows octave's coding style and I need some pointers:
- Is frame_protect needed/desired on current_source_file_depth?
- What's a suggested maximum depth? I used Vmax_recursion_limit which is 256.
- Are sanity checks needed somewhere to verify that current_source_file_depth is 0 at certain entry points?
|
Fri 09 Apr 2010 10:09:04 AM UTC, original submission:
If you have "crash.m" file in your current directory containing this line:
source('crash.m');
and you call source('crash.m') from the interpreter, you get a segmentation fault instead of a warning or something else.
|