bugGNU Octave - Bugs: bug #45947, savepath can create a ~/.octaverc...

 
 

bug #45947: savepath can create a ~/.octaverc that crashes Octave at startup

Submitter:  Mike Miller <mtmiller>
Submitted:  Sat 12 Sep 2015 07:31:28 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  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

Mon 16 May 2016 04:18:22 AM UTC, comment #10: 

No comments, so I went ahead and applied the second solution from comment #9.  See http://hg.savannah.gnu.org/hgweb/octave/rev/764e586dca4b.

Fixed, closing report.

Rik <rik5>
Group administrator
Thu 12 May 2016 12:46:30 AM UTC, comment #9: 

It isn't so hard to do.  One way to eliminate the variable trailing context is to have explicit rules.  This would catch the majority of instances, and is probably good enough given the 80/20 rule.  For example, catching "get|set" followed by 0,1,2 spaces and a then a parenthesis would be:


(set|get)/\( {
    HANDLE_IDENTIFIER ("(set|get)/\\(", true);
  }

(set|get)/{S}\( {
    HANDLE_IDENTIFIER ("(set|get)/{S}\\(", true);
  }

(set|get)/{S}{S}\( {
    HANDLE_IDENTIFIER ("(set|get)/{S}{S}\\(", true);
  }


I've tested this and it works.

Alternatively, remove the trailing context from the Flex pattern, but then you need to strip out the whitespace yourself.

This pattern works to stop the segfault.


(set|get){S}*\( {
    HANDLE_IDENTIFIER ("(set|get){S}*\\(", true);
  }


However, the HANDLE_IDENTIFIER macro needs to be changed since the whitespace is now included in yytext.


else \
{ \
  if (get_set) \
    curr_lexer->maybe_classdef_get_set_method = false; \
\
  int id_tok = curr_lexer->handle_identifier (); \
\
  if (id_tok >= 0) \
    return curr_lexer->count_token_internal (id_tok); \
} \


I changed the code clause where get_set is true to this


  if (get_set) \
    { \
      yyless (3); \
      curr_lexer->maybe_classdef_get_set_method = false; \
    } \


and it works ("make check" passes).

Rik <rik5>
Group administrator
Tue 10 May 2016 09:51:11 PM UTC, comment #8: 

The REJECT feature is not used explicitly, but is triggered by this rule in lex.ll:


// Identifiers.

// Don't allow get and set to be recognized as keywords if they are
// followed by "(".
%}

(set|get)/{S}*\( {
    HANDLE_IDENTIFIER ("(set|get)/{S}*\\(", true);
  }


The problem is the variable-length trailing context (the whitespace followed by an open paren).

If we can change the way that get and set are handled for classdef methods like "get.property_name" to avoid using this trailing context, then this problem should go away.

There is one more use of trailing context in the lexer, but it doesn't involve variable length matching, so it isn't a problem.

Probably we just need a completely different way of recognizing these get/set identifiers in this context.

John W. Eaton <jwe>
Group administrator
Mon 14 Sep 2015 08:02:59 AM UTC, comment #7: 

If our scanner/lexer can't be modified to parse larger strings (e.g. not using REJECT as explained here [1]), at least (1) parsing a too large string should not lead to a segfault, (2) the manual should mention this limitation.

[1] http://flex.sourceforge.net/manual/Diagnostics.html#Diagnostics

Pantxo Diribarne <pantxo>
Group Member
Mon 14 Sep 2015 05:55:58 AM UTC, comment #6: 

Savepath is easy to fix (see comment #1) but the segfault whith long strings is more serious. From a quick search, I found that this is a lexer issue that e.g. Doxygen also has (same limit of 16000 characters see [1]).



[1] see item 13: http://www.star.bnl.gov/public/comp/sofi/doxygen/faq.html

Pantxo Diribarne <pantxo>
Group Member
Sun 13 Sep 2015 10:59:53 PM UTC, comment #5: 

Yes, see bug #45308 for my motivation for opening this bug. A novice user may not even know about the existence of ~/.octaverc, and may not know exactly what path is set to when savepath is called, but it could inadvertently "break" their Octave installation without any indication of what went wrong.

Mike Miller <mtmiller>
Group Member
Sun 13 Sep 2015 09:31:07 PM UTC, comment #4: 

Does anybody really use this function anyways?  We have to have it for Matlab compatibility, but I don't think we need to spend much time perfecting it.  A path of over 26,000 characters seems to me to be a problem the user should solve, rather than Octave.

Rik <rik5>
Group administrator
Sun 13 Sep 2015 09:05:48 PM UTC, comment #3: 

A simpler way to reproduce the same segfault:


n = 1e7;
aa = repmat ("a", 1, n);
eval (sprintf ("str = '%s';", aa))


For me it crashes above n of order 1e7.

Pantxo Diribarne <pantxo>
Group Member
Sun 13 Sep 2015 08:35:32 PM UTC, comment #2: 

And for the segfault, I tried to copy the monolithic path in .octaverc from an editor, and paste it (around 26400 characters according to LibreOffice) in the terminal between two quotes and it also crashed Octave with the same error messages.

So it looks like long string inputs are not well treated in the terminal either.

Pantxo Diribarne <pantxo>
Group Member
Sun 13 Sep 2015 10:41:54 AM UTC, comment #1: 

I can confirm this behavior.

Why not add each path entry individually (something like the attached patch)? Even though startup time is large, it works.

(file #34868)

Pantxo Diribarne <pantxo>
Group Member
Sat 12 Sep 2015 07:31:28 PM UTC, original submission:  

The savepath() function stores the current load path in ~/.octaverc, but it stores the path as one long command. If the user's currently configured load path is huge, this will crash Octave on startup.

Test case:


>> d = tempname ();
>> mkdir (d);
>> cd (d);
>> names = char (randi ([64, 90], 1000, 8));
>> for i = 1:rows(names), mkdir (names(i,:)); endfor
>> addpath (genpath (pwd ()));
>> savepath
warning: savepath: current path saved to ~/.octaverc


Now exit and start Octave again. Octave crashes when trying to execute the long savepath command line with:


error: input buffer overflow, can't enlarge buffer because scanner uses REJECT
input buffer overflow, can't enlarge buffer because scanner uses REJECT
*** Error in `/home/mike/src/octave/default/+build-default/src/.libs/lt-octave-cli': corrupted double-linked list: 0x0000000001060c80 ***
panic: Aborted -- stopping myself...


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 #34868:  savepath_test.patch added by pantxo (2KiB - text/x-diff)

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-05-16 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2016-05-08 pantxo Dependencies- bugs #47876 is dependent
    2015-10-19 pantxo Dependencies- bugs #46245 is dependent
    2015-09-13 pantxo Attached File- Added savepath_test.patch, #34868
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code