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

 
 

bug #29491: recursive source() causes segfault

Submitted by:  None
Submitted on:  Fri 09 Apr 2010 10:09:04 AM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Segfault, Bus Error, etc.
Status: FixedAssigned to: John W. Eaton <jwe>
Originator Name: Matěj TýčOriginator Email: -unavailable-
Open/Closed: ClosedRelease: 3.2.4
Operating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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:

  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 File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 27 Apr 2010 04:09:33 PM UTCjweStatusIn Progress=>Fixed
      Assigned toNone=>jwe
      Open/ClosedOpen=>Closed
    Fri 09 Apr 2010 08:57:27 PM UTCjweStatusNone=>In Progress

    Back to the top


    Powered by Savane 3.1-cleanup1