bugGNU Octave - Bugs: bug #49515, Octave aborts on exit under some...

 
 

bug #49515: Octave aborts on exit under some conditions with --eval CODE

Submitter:  Mike Miller <mtmiller>
Submitted:  Wed 02 Nov 2016 04:08:25 PM UTC
   
 
Category:  Interpreter Severity:  4 - Important
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  mtmiller
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 24 Jan 2017 09:43:25 PM UTC, comment #21: 

Change bug report status to fixed

Avinoam Kalma <avinoam>
Group Member
Thu 03 Nov 2016 05:45:02 PM UTC, comment #20: 

Dan, thanks for your feedback and help diagnosing this.

Mike Miller <mtmiller>
Group Member
Thu 03 Nov 2016 05:40:29 PM UTC, comment #19: 

Pushed to stable and merged to default

http://hg.savannah.gnu.org/hgweb/octave/rev/d9bec44ffaff

Mike Miller <mtmiller>
Group Member
Thu 03 Nov 2016 01:30:24 AM UTC, comment #18: 

Here's the help for nanosleep:

"
nanosleep() suspends the execution of the calling thread until either at least the time specified in *req has elapsed, or the delivery of a signal that triggers the invocation of a handler in the calling thread or that terminates the process.

If the call is interrupted by a signal handler, nanosleep() returns -1, sets errno to EINTR, and writes the remaining time into the structure pointed to by rem unless rem is NULL. The value of *rem can then be used to call nanosleep() again and complete the specified pause (but see NOTES).
"

I assume that octave_nanosleep_wrapper is returning a -1:


void
octave_sleep (double seconds)
{
  if (seconds <= 0)
    return;

  double fraction = std::modf (seconds, &seconds);
  fraction = std::floor (fraction * 1000000000); // nanoseconds

  time_t sec = ((seconds > std::numeric_limits<time_t>::max ())
                ? std::numeric_limits<time_t>::max ()
                : static_cast<time_t> (seconds));

  struct timespec delay = { sec, static_cast<long> (fraction) };
  struct timespec remaining;
  octave_nanosleep_wrapper (&delay, &remaining);

  octave_quit ();
}



int
octave_nanosleep_wrapper (const struct timespec *requested,
                          struct timespec *remaining)
{
  return nanosleep (requested, remaining);
}


and something about this symbolic code is issuing a signal if not during sleep, then just before and the signal is unhandled.

Perhaps the answer is to first check for any unhandled signals before going into the sleep state.  Eh, but the while(true) achieves the same thing in the long run.  The changeset is fine.

Dan Sebald <sebald>
Thu 03 Nov 2016 01:14:20 AM UTC, comment #17: 

Well, the change is no worse than as the code currently exist.  But I don't understand why

-            octave_sleep (86400); // FIXME: really needed?

doesn't sit inactive for 86400 seconds, i.e., one day.

Dan Sebald <sebald>
Thu 03 Nov 2016 01:04:09 AM UTC, comment #16: 

OK, I follow now: this "--eval" mode doesn't even set up that octave_link to the GUI, so the handshaking sleep/awake doesn't take place at exit.  (Well, not 100% sure of that, more later.)

So we end up at 692 if "exit(0)" is not called:


        if (! options.persist ())
          {
            m_quitting_gracefully = true;

            clean_up_and_exit (parse_status);
          }


of which we aren't supposed to return from.

This following (from what I'm recalling now):


    if (octave_link::exit (status))
      {
        if (safe_to_return)
          return;
        else
          {
            // What should we do here?  We might be called from some
            // location other than the end of octave_execute_interpreter,
            // so it might not be safe to return.

            // We have nothing else to do at this point, and the
            // octave_link::exit function is supposed to take care of
            // exiting for us.  Assume that job won't take more than a
            // day...

            octave_sleep (86400); // FIXME: really needed?
          }
      }
    else
      {
        if (octave_exit)
          (*octave_exit) (status);
      }


was done so that the GUI could launch Octave core.  But if it is true that the octave_link isn't fully set up, then octave_link::exit, i.e.,

bool
octave_qt_link::do_exit (int status)
{
  emit exit_app_signal (status);

  // Could wait for a while and then timeout, but for now just
  // assume the GUI application exit will be without problems.
  return true;
}

is emitting a signal to nowhere.  If I recall correctly, the issue was that if Octave exits directly (rather than having the GUI shut down its thread) then the main GUI thread displays an error.

Here's the odd thing.  octave_sleep() has a octave_quit() at the end of it.


void
octave_sleep (double seconds)
{
  if (seconds <= 0)
    return;

  double fraction = std::modf (seconds, &seconds);
  fraction = std::floor (fraction * 1000000000); // nanoseconds

  time_t sec = ((seconds > std::numeric_limits<time_t>::max ())
                ? std::numeric_limits<time_t>::max ()
                : static_cast<time_t> (seconds));

  struct timespec delay = { sec, static_cast<long> (fraction) };
  struct timespec remaining;
  octave_nanosleep_wrapper (&delay, &remaining);

  octave_quit ();
}


Could it be that clean_up_and_exit() isn't returning but going to this octave_quit(), because the timer is timing out somehow?  Or have you confirmed the return from clean_up_and_exit() in the debugger?

Seems there should be a better way.  The slots for a QThread are as follows:

void quit()
void start(Priority priority = InheritPriority)
void terminate()

The core can emit signals.  I wonder if it is possible to emit a signal from within the QThread that is intended to terminate.

But still, I'm a bit confused about whether the octave_link is actually working in the eval scenario.

Dan Sebald <sebald>
Thu 03 Nov 2016 12:35:01 AM UTC, comment #15: 

I'm planning on pushing the attached patch to the stable branch unless I hear any objections. I'm testing with this patch applied currently to make sure it solves all the problems I have seen while testing this bug and octsympy.

(file #38872)

Mike Miller <mtmiller>
Group Member
Thu 03 Nov 2016 12:13:22 AM UTC, comment #14: 

Like I said, with or without the explicit call to Octave's exit() function, the only difference is where the interpreter crashes.

I don't think you've looked at octave:;interpreter yet, there are 7 calls to clean_up_and_exit() from different code paths depending on whether Octave was called with --eval or not, whether Octave was called with a script file or not, whether --persist was given or not, whether it's running interactively or not, whether the code that was in a script or --eval called exit or not.

If you run


$ octave --eval "exit(0);"


then octave::interpreter::clean_up_and_exit is called from within the octave::interpreter::execute_eval_option_code method (line 795).

If you run


$ octave --eval "true;"


then octave::interpreter::clean_up_and_exit is called from the octave::interpreter::execute method (line 692) instead.

If either of these calls to clean_up_and_exit returns, bad things happen.

I think I've answered your question, whether or not the --eval script calls exit() explicitly, Octave attempts to exit by signaling the octave_link, and then the interpreter continues to run and accesses invalid memory because the main thread has exited.

Both of these should end in a single call to clean_up_and_exit that never returns.

Mike Miller <mtmiller>
Group Member
Wed 02 Nov 2016 11:51:43 PM UTC, comment #13: 

Yeah, heisenbug is about right.

There is an "exit(0)" in the script you are running.  If that is not present, does the process just hang in a suspended mode?  Could that be the source of the problem?  I.e., that in this "--eval" mode octave is meant to run the code and then force an exit, but the extra "exit(0)" is causing some kind of dual shutdown?


Dan Sebald <sebald>
Wed 02 Nov 2016 11:35:37 PM UTC, comment #12: 

Yes, you are right, all of the confirm_shutdown handshaking comes way before the code that I'm looking at. I don't see any relationship to the state of the interpreter or the GUI to this part of the code that you are pointing at.

The confirm_shutdown back-and-forth happens when the quit() function is called or when the user tells the GUI to exit.

In the condition that I am debugging, that has already passed, there is no GUI running so there is nothing to check, the signal is immediately sent back, the interpreter thread is awakened, and the clean_up_and_exit function is called.

At the state of the program I am debugging, the interpreter thread has done all of its flush output, close open file descriptors, run all atexit hooks, and is either supposed to call exit() itself or tell the Qt main thread to call exit() (by calling QApplication::exit()).

I don't know exactly what is causing the interpreter thread to wake up or break out of its nanosleep early, because every time I try to use gdb to set a breakpoint on the sleep function, the race condition does not appear and the program exits cleanly, it's a heisenbug.

I'm not sure it matters now, I think the takeaway is that we should do whatever is necessary to make sure that the clean_up_and_exit method never returns or else the interpreter will be dereferencing an invalid object (itself).

Mike Miller <mtmiller>
Group Member
Wed 02 Nov 2016 11:09:20 PM UTC, comment #11: 

What you described sounds correct.  The handshake is done in the


bool
octave_qt_link::do_confirm_shutdown (void)
{
  // Lock the mutex before emitting signal.
  mutex.lock ();

  emit confirm_shutdown_signal ();

  // Wait while the GUI shuts down.
  waitcondition.wait (&mutex);

  // The GUI has sent a signal and the thread has been awakened.

  mutex.unlock ();

  return _shutdown_confirm_result;
}


call.

The GUI thread is responding to confirm_shutdown here:


  // Wait for link thread to go to sleep state.
  _octave_qt_link->mutex.lock ();

  _octave_qt_link->shutdown_confirmation (closenow);

  _octave_qt_link->mutex.unlock ();

  // Awake the worker thread so that it continues shutting down (or not).
  _octave_qt_link->waitcondition.wakeAll ();


So there is a sleep/awake happening, but it is supposed to be before reaching this octave_sleep (86400); line.  So I don't know what is waking the core once it gets to this line.

Dan Sebald <sebald>
Wed 02 Nov 2016 10:55:02 PM UTC, comment #10: 

I don't think your description matches the reality of the octave::interpreter class and the clean_up_and_exit method.

You are suggesting that the sleep is kind of like a select() or a semaphore, wait here until some other thread says it's ok to continue execution. I don't think that is correct at all.

The parameter to the clean_up_and_exit function is called safe_to_return (which I understand as "safe to return at all"). The comment before the sleep seems to confirm that the point of the sleep is that the function blocks at this point forever until the process exits completely.

Note that there are quite a few places where clean_up_and_exit is called inside the interpreter class, and I think that all of them are essentially supposed to be treated as dead end paths that will never return control.

In fact, if the GUI is not running, the clean_up_and_exit function just calls the C exit() function instead of sending a signal over the octave_link and sleeping. So to me it seems deliberate that the function is supposed to block indefinitely until the process exits completely.

If I am wrong, then please show me where the GUI main thread is intentionally waking up the interpreter thread and what the unwinding of the interpreter thread's stack at that point is supposed to look like.

Mike Miller <mtmiller>
Group Member
Wed 02 Nov 2016 10:20:43 PM UTC, comment #9: 

The idea with the sleep is a sort of handshaking with the GUI.  I'm fairly certain other problems were run into, at the time, if the core just shut down the GUI.  There might be open files, for example, or something else that created timing issues.  So, I know that the sleep was put in so that the GUI did its thing and then awoke the core process when it was done with all its cleanup.

That is probably where the comment "assume it doesn't take a day" comes from, i.e., it shouldn't take a day for the GUI user to answer the question "Save this file".

Anyway, it sounds like there might be more than one thread waking up the core thread.  That of course doesn't work in this scenario.  It would need to be something more sophisticated like:

Confirm Shutdown Python Thread
Sleep until Python thread awakes core
Confirm Shutdown GUI Thread
Sleep until GUI thread awakes core

Or have some flags so that the core keeps going into a sleep state and upon waking checks that all flags have been set, otherwise go back to sleep.

Dan Sebald <sebald>
Wed 02 Nov 2016 10:15:32 PM UTC, comment #8: 

Correspondingly, if I completely delete the octave_sleep, then I can more readily reproduce this error even without running Octave in gdb.

Mike Miller <mtmiller>
Group Member
Wed 02 Nov 2016 10:01:22 PM UTC, comment #7: 

Absolutely, I would not call this a solution. All I've done is confirm that the persistent open pipe to the python executable that octsympy opens helps to provoke the condition that leads to this error.

I guess the expectation is that the clean_up_and_exit function is supposed to never return if the safe_to_return flag is false (which it is in all of my testing). However, the method does seem to be returning from its supposed-infinite sleep and causing the crash I am running into.

I made the following quick-n-dirty hack


diff --git a/libinterp/corefcn/interpreter.cc b/libinterp/corefcn/interpreter.cc
--- a/libinterp/corefcn/interpreter.cc
+++ b/libinterp/corefcn/interpreter.cc
@@ -1078,7 +1078,11 @@ namespace octave
             // exiting for us.  Assume that job won't take more than a
             // day...

-            octave_sleep (86400); // FIXME: really needed?
+            do
+              {
+                octave_sleep (86400); // FIXME: really needed?
+              }
+            while (true);
           }
       }
     else


and now I no longer get any abort or segmentation fault with any of the cases that did reliably crash on me without this change.

The answer to the FIXME seems obvious, yes, something is needed in this function to prevent it from returning when it has told the GUI thread to exit the application. What's in there now doesn't seem to be sufficient.

Cc jwe, you added the code to relay the exit over the octave_qt_link back in 2013 (http://hg.savannah.gnu.org/hgweb/octave/rev/8b783661e03f), any thoughts?

Mike Miller <mtmiller>
Group Member
Wed 02 Nov 2016 08:42:03 PM UTC, comment #6: 

OK, good that you found a solution, in principle.  Does it seem, though, that the clean up should be waiting on all processes to finish before moving forward with the GUI shutdown (or vice versa)?

Dan Sebald <sebald>
Wed 02 Nov 2016 08:37:10 PM UTC, comment #5: 

Yes, adding "sympref reset;" to the --eval code string forces the Python pipe subprocess to be closed, and this bug no longer happens.

Mike Miller <mtmiller>
Group Member
Wed 02 Nov 2016 08:34:05 PM UTC, comment #4: 

I have now reduced the test case to as simple as


$ ./run-octave --eval "addpath ~/src/octave/octsympy/inst; sym(1); exit(0);"
(gdb) r
...
pure virtual method called
terminate called without an active exception

Thread 10 "QThread" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:58


I thought I had tried this simple test before and it didn't trigger the error, but now it does.

If I set a breakpoint on octave::interpreter::clean_up_and_exit, it appears to be called twice. The first time it is called when the eval block calls exit(). This sends the signal over the octave_qt_link to cause the Qt window class and main application to delete and exit itself.

However, at this point the octave::interpreter::execute function is still running on the main_thread QThread loop. In fact, by the time the octave::interpreter::execute_eval_option_code function returns up one stack frame to octave::interpreter::execute, the main function has already exited and the main_window, application, interpreter, and octave_qt_link objects have already deleted themselves.

The next step in the execute function calls the clean_up_and_exit method a second time, and this is where the abort() occurs.

If I take out the `exit(0)` from the --eval code string, then clean_up_and_exit is only called after execute_eval_option_code returns, but again by the time the next routines are in play, the Qt main application and main function have already exited. The segfault in this case occurs when the interpreter tries to see if have_script_file() is true, and make copies of the argv array. Since the application object has been destroyed, it incorrectly enters this code block and crashes on trying to store argv into Octave string objects.

Maybe the key to this bug is the popen2 that is left open pointing at the Python interpreter process. It's possible that the delay from closing the Python process is slowing down the clean_up_and_exit part of the interpreter to the point that this bug is exposed.

Mike Miller <mtmiller>
Group Member
Wed 02 Nov 2016 07:10:25 PM UTC, comment #3: 

Oh yeah, I keep forgetting that --no-gui isn't the same as --no-gui-libs.

So more delay (a breakpoint) avoids the problem?  That rules out my hypothesis.

Quoting:

"the octave-gui executable's main function has already returned and cleaned up its state."

there is this hunk


  if (! octave_link::confirm_shutdown ())
    return ovl ();


which is supposed to confirm that the GUI is OK to shutdown.  Quoting:

"octave::interpreter::execute function is still running on a QThread"

so this QThread is part of the GUI?  Or just a mechanism that the core is using to create another thread?  I seems to me that all of this should be under the confirm_shutdown() umbrella.

Dan Sebald <sebald>
Wed 02 Nov 2016 06:57:40 PM UTC, comment #2: 

That is not a safe assumption. Specifically, when running octave (or the octave-gui executable directly), the --eval option implies the same thing as the --no-gui option (unless the --force-gui or --persist options are also given).

If you actually meant octave-cli (or octave --no-gui-libs), then yes you are correct, this problem does not occur under the octave-cli executable.

Could be, might be, either one of those things.

It is odd that if I set a gdb breakpoint on or inside the octave_qt_link destructor, the error does not occur.

If I get rid of the explicit call to `exit()`, the error changes from a SIGABRT (due to a pure virtual method being called) to a SIGSEGV, but with the same general state: the octave::interpreter::execute function is still running on a QThread while the octave-gui executable's main function has already returned and cleaned up its state.

Mike Miller <mtmiller>
Group Member
Wed 02 Nov 2016 06:47:08 PM UTC, comment #1: 

I assume there are no such problems when adding "--no-gui" to the command line invocation.

Could it be that by running the symbolic package there is a lot of complicated stuff in memory, i.e., tables, etc. that takes a while to clean up so there might be a longer delay between shutting down the GUI thread and something else (in the core thread) a bit later that uses octave_qt_link?

Might it simply be a case of having to set octave_qt_link to NULL after shutting down the GUI thread so that it isn't used from that point forward (assuming the core thread checks that sort of thing already)?

Dan Sebald <sebald>
Wed 02 Nov 2016 04:08:25 PM UTC, original submission:  

I have a reproducible case for this error on exit, but unfortunately it is the entire symbolic package test suite.


$ ./run-octave -g --eval "addpath ~/src/octave/octsympy/inst; exit (octsympy_tests);"
(gdb) r
...
[Thread 0x7fffc9657700 (LWP 25243) exited]
pure virtual method called
terminate called without an active exception

Thread 10 "QThread" received signal SIGABRT, Aborted.


The abort occurs when `octave_link::exit (status)` is called by the interpreter in the `clean_up_and_exit` method. The problem is that the octave_link has been partially destroyed, it is no longer an octave_qt_link, but the `instance` pointer still points to the same address. The `exit` method is not implemented in the base class.

I have not yet figured out how the interpreter is still in `clean_up_and_exit` while the GUI main function has already exited and the octave_qt_link has already been destroyed.

The symbolic unit test suite exercises a lot of functionality, including plotting figures, opening and closing files, keeping a persistent popen2 running in the background, so I haven't reduced this to a smaller reproducible example yet. Just running one or two of the `@sym` method unit tests is not enough to trigger this.

Mike Miller <mtmiller>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #38872:  exit-delay.diff added by mtmiller (2KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by mtmiller
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by mtmiller (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-01-24 avinoam StatusPatch Submitted Fixed
    2016-11-03 mtmiller Open/ClosedOpen Closed
    2016-11-03 mtmiller Attached File- Added exit-delay.diff, #38872
        StatusNeed Info Patch Submitted
    2016-11-02 mtmiller Carbon-Copy- Added jwe

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code