bugGNU Octave - Bugs: bug #53209, inputdlg crashes if prompt and...

 
 

bug #53209: inputdlg crashes if prompt and default cell arrays are different sizes

Submitter:  Jake <epy>
Submitted:  Fri 23 Feb 2018 02:10:56 AM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 4.2.1 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 28 Feb 2018 07:49:48 PM UTC, comment #11: 

I recall checking when I created the patch and didn't see any other problematic cases in which the loop limit was determined from something other than what is indexed within the loop.  The inputdlg is unique in that there is multiple lists whereas the rest typically have a single list.

Dan Sebald <sebald>
Wed 28 Feb 2018 07:40:05 PM UTC, comment #10: 

Dan's patch works for MXE as well so I committed it to the development branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/abfd4fc1b977).

I'm going to mark this bug as fixed and close this report, but are any of the other dialogs (ListDialog, MessageDialog, etc.) in dialog.cc also vulnerable to out-of-bound errors?

Rik <rik5>
Group administrator
Wed 28 Feb 2018 05:57:17 PM UTC, comment #9: 

This bug is reported against the development branch, but the code causes a segfault on stable as well.  Changing the release in this bug report to 4.2.1.

I applied Philip's patch on the stable branch as a quick fix.  See http://hg.savannah.gnu.org/hgweb/octave/rev/e098b0fb11ab.

I'm testing Dan's patch with MXE, but that will take an hour or two.

Rik <rik5>
Group administrator
Sat 24 Feb 2018 08:53:06 PM UTC, comment #8: 

Until early next week I think I have no opportunity to test your patch (as it requires a double build - Linux & MXE) while the .m-file fix is easily tested.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 24 Feb 2018 08:15:27 PM UTC, comment #7: 

OK, sounds good.  I'd say apply both changesets; shouldn't be able to crash a program by altering script code.

Dan Sebald <sebald>
Sat 24 Feb 2018 07:25:16 PM UTC, comment #6: 

Yes my fix makes inputdlgÅ› error checking Matlab-compatible, below is what Matlab r2018a prerelease makes of the original example and a fixed one:


>> h = inputdlg ({'foo', 'bar'}, 'title', 1, {'123'})
Index exceeds array bounds.

Error in inputdlg (line 260)
  if ~ischar(DefAns{lp})

>> h = inputdlg ({'foo', 'bar'}, 'title', 1, {'123', '456'})

% (a neat inputdlg windows pops up with default values 123 and 456)

h =

  2×1 cell array

    {'123'}
    {'456'}


Philip Nienhuis <philipnienhuis>
Group Member
Fri 23 Feb 2018 10:18:14 AM UTC, comment #5: 

Oh, I didn't see your changeset.  Are we sure that forcing the array lengths to be the same is the thing to do?  I'm fine with that (it obviously didn't cross my mind at the time that they'd be different), but I mean from a compatibility perspective.

Dan Sebald <sebald>
Fri 23 Feb 2018 09:48:27 AM UTC, comment #4: 

@Dan:
My patch was needed anyway to just catch wrong input, but you're absolutely right that further down the road Octave shouldn't crash.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 23 Feb 2018 09:31:34 AM UTC, comment #3: 

I wrote the Qt inputdlg-related code so took a look at this.  Attached is a fix; please test.

I also removed this comment that someone put in:


    // FIXME: Why define and then immediately test value?
#define LINE_EDIT_FOLLOWS_PROMPT 0

#if LINE_EDIT_FOLLOWS_PROMPT


It shouldn't be too much of a quandary why that was done.  First, I didn't know what layout these input dialogs should have, so I put two versions in there that someone who cares could pick the appropriate layout.  Second, John suggested limiting definitions in files to not clutter the space.  So, this definition is very limited in scope, as it is undefined about a dozen lines later.  So the defines without the code are:


#define LINE_EDIT_FOLLOWS_PROMPT 0

#if LINE_EDIT_FOLLOWS_PROMPT
#else
#endif

#if LINE_EDIT_FOLLOWS_PROMPT
#else
#endif

#undef LINE_EDIT_FOLLOWS_PROMPT


Because the definition is used in two places, if we were to do the following:


#if 0
#else
#endif

#if 0
#else
#endif


both of the zeros would have to be changed to 1 to accomplish the equivalent of changing LINE_EDIT_FOLLOWS_PROMPT to 1.  Third, the definition gives meaning to what the pre-processor switch does.

If the definition isn't desired, let's pick the layout that has been used for a while and discard the other code.

(file #43389)

Dan Sebald <sebald>
Fri 23 Feb 2018 08:51:27 AM UTC, comment #2: 

Here is a very simple cset. Something for stable as well?

(file #43388)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 23 Feb 2018 04:21:14 AM UTC, comment #1: 

Confirmed.  And the issue is still present on the development branch of Octave.

Rik <rik5>
Group administrator
Fri 23 Feb 2018 02:10:56 AM UTC, original submission:  

GUI crashes immediately if the prompt and defaults cell arrays are different sizes. Try:

inputdlg({'foo','bar'},'title',1,{'123'})

to reproduce.

Jake <epy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43388:  bug53209.cset added by philipnienhuis (822B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by sebald (Updated the item)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by epy (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-02-28 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2018-02-28 rik5 StatusReady For Test Patch Reviewed
        Releasedev 4.2.1
    2018-02-24 philipnienhuis StatusConfirmed Ready For Test
        Release4.2.1 dev
    2018-02-23 sebald Attached File- Added octave-qt_inputdlg_crash_fix-djs2018feb23.patch, #43389
    2018-02-23 philipnienhuis Attached File- Added bug53209.cset, #43388
    2018-02-23 rik5 CategoryOctave Function GUI
        Item GroupMissed Error or Warning Segfault, Bus Error, etc.
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code