bugGNU Octave - Bugs: bug #47837, Segmentation fault with missing...

 
 

bug #47837: Segmentation fault with missing warning state "all"

Submitter:  Guillaume <gyom>
Submitted:  Tue 03 May 2016 03:01:18 PM UTC
   
 
Category:  Octave Function Severity:  4 - Important
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  lachlan
Originator Name:  Guillaume Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 15 Jul 2016 10:11:52 PM UTC, comment #15: 

I verified the fix works.  Closing report.

Rik <rik5>
Group administrator
Fri 15 Jul 2016 05:23:02 PM UTC, comment #14: 

I pushed Lachlan's change for bug #47543:

http://hg.savannah.gnu.org/hgweb/octave/rev/879f297f3194

I think that fixes this problem too.  Can this report be closed now?

John W. Eaton <jwe>
Group administrator
Fri 15 Jul 2016 03:48:02 PM UTC, comment #13: 

As far as I understand it, the behavior of throwing an exception from a signal handler is not specified, so I don't think that is something we should do.

John W. Eaton <jwe>
Group administrator
Thu 05 May 2016 07:38:49 AM UTC, comment #12: 

Rik, I agree that segfaults are to be avoided whenever possible.

I have been toying with the idea of causing the signal handler to throw an exception that can be caught, instead of exiting.  This would allow a clean solution for bug #36067 ("exist" on a buggy mex/oct file causing a crash), and would also make it cleaner for a general seg-fault to drop back to the command prompt with a warning to save whatever is possible and then restart.

What do you think?

Lachlan Andrew <lachlan>
Wed 04 May 2016 03:45:39 PM UTC, comment #11: 

I added bug #47543 as a dependency.  Since Mike did most of the review over on the other report, I'll let him decide whether it is acceptable to push it now.  In general, I hate segfaults since user's can lose valuable work when they unexpectedly happen.  My vote would be to plug the hole, and then figure out the best real repair.

Rik <rik5>
Group administrator
Wed 04 May 2016 03:37:00 PM UTC, comment #10: 

Thanks Lachlan. Would be great if you could apply your patch as it is now.

Guillaume <gyom>
Wed 04 May 2016 02:03:31 AM UTC, comment #9: 

Mike was right.  The reason "all" is being removed is the same as bug #47543, and the patch there fixes this.

Its behaviour isn't ideal -- doing 'warning ("off", "all")' only changes the "all" state, and doesn't clear warnings that were set explicitly.  However, it is better than crashing, and I think it would be good to apply that patch now, and implement the proper "all" behaviour in the new framework I mentioned.

Lachlan Andrew <lachlan>
Tue 03 May 2016 11:02:16 PM UTC, comment #8: 

I'll look into this.

I am planning an overhaul of the handling of warnings, which will include the ability to warn of non-existing warnings in the Octave: namespace, though not in other name spaces for the reason Rik mentions.  I posted a request for comments on the maintainers' mailing list, and got no response.  Since it will require a substantial overhaul, I'd really like to make sure that it will be approved once it is written.

Lachlan Andrew <lachlan>
Tue 03 May 2016 05:54:20 PM UTC, comment #7: 

To my surprise:


>> warning ('off', 'not-existing')
Error using warning
Unknown setting or incorrect message identifier 'not-existing'.

>> warning ('query', 'not-existing')
Error using warning
Unknown setting or incorrect message identifier 'not-existing'.


but, actually, the error is due to the identifier itself: according to Matlab's doc, it can only contain alphanumerics (A–Z, a–z, 0–9) and underscores, starting with a letter. It then works as I would expect:


>> warning ('off', 'Octave:not_existing')
>> warning ('query', 'Octave:not_existing')
The state of warning 'Octave:not_existing' is 'off'.


So you can ignore my comment #5.

Guillaume <gyom>
Tue 03 May 2016 05:41:41 PM UTC, comment #6: 

Unfortunately, maintaining a list of possible warnings to verify against would be horribly difficult and almost always out-of-date.  What happens if a programmer wants to make use of their own special warning ID?  The Octave core would never know about it and issue a warning.

Matlab might try to finesse the issue by only issuing an error if the warning ID has not already been used.  What does Matlab do for the following?


warning ('off', 'not-existing')
warning ('query', 'not-existing')



Rik <rik5>
Group administrator
Tue 03 May 2016 04:59:59 PM UTC, comment #5: 

Thanks for looking into this. Just to add that Matlab's behavior with warning('query',...) and an unknown identifier is to return an error:


>> warning('query','not-existing');
Unknown setting or incorrect message identifier 'not-existing'.


In the development branch, when the 'all' identifier exists, Octave returns:


ans =

  scalar structure containing the fields:

    identifier = not-existing
    state = on


Guillaume <gyom>
Tue 03 May 2016 04:40:47 PM UTC, comment #4: 

I think bug #47543 is different.  It was introduced on the change from 4.0.0 to 4.0.1.  On the other hand, this report deals with a change from 4.0.2 to default.

Rik <rik5>
Group administrator
Tue 03 May 2016 04:14:13 PM UTC, comment #3: 

May be related to bug #47543, which is also on default but not stable, there is some discussion there.

Mike Miller <mtmiller>
Group Member
Tue 03 May 2016 04:04:40 PM UTC, comment #2: 

Confirmed.  This "works" with version 4.0.2 where it only produces an error message.  But on the development branch it hits an assert statement which causes a panic.

The default branch code in the function warning_query in error.cc looks like this:


  if (found)
    {
      retval.assign ("identifier", id);
      retval.assign ("state", val);
    }
  else
    error ("warning: unable to find default warning state!");

  return retval;


On the development branch the code now looks like this:


  // The warning state "all" is always supposed to remain in the list,
  // so we should always find a state, either explicitly or by using the
  // state for "all".

  assert (found);

  retval.assign ("identifier", id);
  retval.assign ("state", val);


According to 'hg blame' the cset that made the change was 20535:b70cc4bd8109 by jwe.  I've added him to the CC list for this bug.  The easiest solution would be to revert the code, but it's also true that one shouldn't be able to get rid of the 'all' identifier.  That just sounds like a harder fix.

Rik <rik5>
Group administrator
Tue 03 May 2016 03:12:34 PM UTC, comment #1: 

A faster way to reproduce the error (that does not involve object comparison):


ws = warning;
warning(ws);
warning('query','Octave:not-existing')


Guillaume <gyom>
Tue 03 May 2016 03:01:18 PM UTC, original submission:  

The following lines of code remove the warning state "all":


ws = warning;
ws.identifier
warning(ws);
ws = warning;
ws.identifier


This generates a segmentation fault when comparing objects with isequal/isequaln:


>> isequal(inputParser,inputParser)
octave-gui: ../libinterp/corefcn/error.cc:1275: octave_scalar_map warning_query(const string&): Assertion `found' failed.
panic: Aborted -- stopping myself...


According to the comment in error.cc, the warning state "all" should always be present.


  // The warning state "all" is always supposed to remain in the list,
  // so we should always find a state, either explicitly or by using the
  // state for "all".


Guillaume <gyom>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by gyom (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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-15 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2016-07-15 jwe StatusPatch Submitted Ready For Test
    2016-07-01 rik5 Severity3 - Normal 4 - Important
    2016-05-04 rik5 Assigned toNone lachlan
        Dependencies- Depends on bugs #47543
    2016-05-04 lachlan StatusConfirmed Patch Submitted
    2016-05-03 rik5 StatusNone Confirmed
        Carbon-Copy- Added jwe

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code