bugGNU Octave - Bugs: bug #29491, recursive source() causes segfault

 
 

bug #29491: recursive source() causes segfault

Submitter:  None
Submitted:  Fri 09 Apr 2010 10:09:04 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  jwe
Originator Name:  Matěj Týč Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 3.2.4
Operating System:  * GNU/Linux Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

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

John W. Eaton <jwe>
Group administrator
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.

Judd Storrs <judd>
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.

Jaroslav Hajek <highegg>
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.

Judd Storrs <judd>
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.

Jaroslav Hajek <highegg>
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?

John W. Eaton <jwe>
Group administrator
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().

Jaroslav Hajek <highegg>
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.

John W. Eaton <jwe>
Group administrator
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:


--- a/src/oct-parse.yy        Thu Apr 08 18:43:47 2010 -0400
+++ b/src/oct-parse.yy        Fri Apr 09 14:30:41 2010 -0400
@@ -118,6 +118,15 @@
 // we have nested functions or just implicitly ended subfunctions.
 static int max_function_depth = 0;

+// Maximum nesting level for source_file()
+static int max_source_file_depth = 256;
+
+// = 0 currently outside any sourced file.
+// = 1 inside the first sourced file.
+// > 1 means we are looking at a sourced file included by another
+//   sourced file.
+static int current_source_file_depth = 0;
+
 // FALSE if we are still at the primary function. Subfunctions can
 // only be declared inside function files.
 static int parsing_subfunctions = false;
@@ -3881,8 +3890,13 @@
         frame.add_fcn (octave_call_stack::pop);
     }

+  if ( current_source_file_depth == max_source_file_depth )
+    error ("source: too many nested source files");
+
   if (! error_state)
     {
+      current_source_file_depth++ ;
+
       octave_function *fcn = parse_fcn_file (file_full_name, "", true,
                                              require_file, warn_for);

@@ -3910,6 +3924,8 @@
       else
         error ("source: error sourcing file `%s'",
                file_full_name.c_str ());
+
+      current_source_file_depth-- ;
     }
 }


I'm not sure this follows octave's coding style and I need some pointers:

  1. Is frame_protect needed/desired on current_source_file_depth?
  2. What's a suggested maximum depth? I used Vmax_recursion_limit which is 256.
  3. Are sanity checks needed somewhere to verify that current_source_file_depth is 0 at certain entry points?
Judd Storrs <judd>
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.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by highegg (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by judd (Posted a comment)
  • -email is unavailable- added by None (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-04-27 jwe StatusIn Progress Fixed
        Assigned toNone jwe
        Open/ClosedOpen Closed
    2010-04-09 jwe StatusNone In Progress

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code